Issue 5767063142400000: notification fixes (Closed)
|
Created:
March 3, 2014, 3:50 p.m. by saroyanm Modified:
March 6, 2014, 11:06 a.m. Visibility:
Public. |
DescriptionThis issue is related to the discussion under closed notification issue:
http://codereview.adblockplus.org/6098518317989888/diff2/5728116278296576:5651874166341632/chrome/ext/background.js
Patch Set 1 #
Total comments: 2
Patch Set 2 : platform information from info module #Patch Set 3 : removed import of ext/background.js from notification.html #
Total comments: 11
Patch Set 4 : notification.html relocation, safari changes revert and notification.js update #Patch Set 5 : removed ext/common.js import from notification.html #
Total comments: 2
Patch Set 6 : Nit fix for long lines #
MessagesTotal messages: 18
In case I'll also update the patch after conclusion on current discussion: http://codereview.adblockplus.org/6098518317989888/diff2/5733935958982656:565...
http://codereview.adblockplus.org/5767063142400000/diff/5629499534213120/back... File background.js (right): http://codereview.adblockplus.org/5767063142400000/diff/5629499534213120/back... background.js:351: if (typeof chrome != "undefined" && "notifications" in chrome && navigator.platform.indexOf("Linux") == -1) You can find the platform information in the info module. Use that instead of checking for the global "chrome" variable.
Updated to use info module to get platform information. http://codereview.adblockplus.org/5767063142400000/diff/5629499534213120/back... File background.js (right): http://codereview.adblockplus.org/5767063142400000/diff/5629499534213120/back... background.js:351: if (typeof chrome != "undefined" && "notifications" in chrome && navigator.platform.indexOf("Linux") == -1) On 2014/03/04 09:48:48, Thomas Greiner wrote: > You can find the platform information in the info module. Use that instead of > checking for the global "chrome" variable. Thanks for pointing to the module.
Guys I've uploaded a new patch to remove ext/background.js import from notification.html after aligning with Thomas on this, please let me know if you are okey with It. I've used window.open to open a link in the active window, that is how window.open works in chrome extensions beside opening a new window. Also moved ext.backgroundPage implementation from safari/ext/background.js to safari/ext/common.js (had no chance to test with safari while have no mac device).
On 2014/03/04 16:35:42, saroyanm wrote: > Guys I've uploaded a new patch to remove ext/background.js import from > notification.html after aligning with Thomas on this, please let me know if you > are okey with It. > I've used window.open to open a link in the active window, that is how > window.open works in chrome extensions beside opening a new window. Also moved > ext.backgroundPage implementation from safari/ext/background.js to > safari/ext/common.js (had no chance to test with safari while have no mac > device). As already pointed out in the other review, notification.html and notification.js is only used on Chrome. Therefore there is no need to bother about Safari. Please get rid of both includes for ext/common.js and ext/background.js in notification.html. And since this code is only for Chrome, you don't even need ext, but can just use native Chrome APIs. You also might want to move notification.html and notification.js into the chrome subfolder and prevent it from being included in the Safari build.
http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... File notification.html (right): http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... notification.html:29: <script src="ext/common.js"></script> We don't need ext/common.js anymore if we remove notification.html from Safari and get rid of references to ext.*. http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... File notification.js (right): http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... notification.js:18: var backgroundPage = ext.backgroundPage.getWindow(); This can be changed to use the Chrome API. http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... notification.js:88: window.open(link.href); You can just use chrome.tabs.create for that. http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/safa... safari/ext/common.js:215: backgroundPage: { ext.backgroundPage is already defined in ext/popup.js. Seems like we can leave it where it was.
Guys sorry for late answer, but actually seams like we should rethink about implementation while notification.js also imported in popup.html and used by safari. http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... File notification.html (right): http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... notification.html:29: <script src="ext/common.js"></script> On 2014/03/05 13:24:47, Thomas Greiner wrote: > We don't need ext/common.js anymore if we remove notification.html from Safari > and get rid of references to ext.*. notification.js also imported in popup.html, so we can't use only chrome API there I guess, maybe we can still have this import here, what you think ? http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... File notification.js (right): http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... notification.js:18: var backgroundPage = ext.backgroundPage.getWindow(); On 2014/03/05 13:24:47, Thomas Greiner wrote: > This can be changed to use the Chrome API. Thomas actually seams like we forgot about popup.html, notification.js also imported into popup.html, so maybe we don't need to change it yet. http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... notification.js:88: window.open(link.href); On 2014/03/05 13:24:47, Thomas Greiner wrote: > You can just use chrome.tabs.create for that. Again, connected to the comment for ext.backgroundPage.getWindow(), seams like this file will be used also by safari.
http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... File notification.html (right): http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... notification.html:29: <script src="ext/common.js"></script> On 2014/03/05 13:44:28, saroyanm wrote: > On 2014/03/05 13:24:47, Thomas Greiner wrote: > > We don't need ext/common.js anymore if we remove notification.html from Safari > > and get rid of references to ext.*. > > notification.js also imported in popup.html, so we can't use only chrome API > there I guess, maybe we can still have this import here, what you think ? Good point. In that case I'd be in favor of keeping common.js in here. @Sebastian What do you think?
http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... File notification.html (right): http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... notification.html:29: <script src="ext/common.js"></script> On 2014/03/05 13:51:48, Thomas Greiner wrote: > On 2014/03/05 13:44:28, saroyanm wrote: > > On 2014/03/05 13:24:47, Thomas Greiner wrote: > > > We don't need ext/common.js anymore if we remove notification.html from > Safari > > > and get rid of references to ext.*. > > > > notification.js also imported in popup.html, so we can't use only chrome API > > there I guess, maybe we can still have this import here, what you think ? > > Good point. In that case I'd be in favor of keeping common.js in here. > @Sebastian What do you think? Don't we also need ext.windows in order to open a new page? That can't be moved to common.js. However in popup.html we already have ext. And in notification.html (which is still only used on Chrome) you can retrieve ext via chrome.extension.getBackgroundPage().ext.
http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... File notification.html (right): http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... notification.html:29: <script src="ext/common.js"></script> On 2014/03/05 14:11:56, Sebastian Noack wrote: > On 2014/03/05 13:51:48, Thomas Greiner wrote: > > On 2014/03/05 13:44:28, saroyanm wrote: > > > On 2014/03/05 13:24:47, Thomas Greiner wrote: > > > > We don't need ext/common.js anymore if we remove notification.html from > > Safari > > > > and get rid of references to ext.*. > > > > > > notification.js also imported in popup.html, so we can't use only chrome API > > > there I guess, maybe we can still have this import here, what you think ? > > > > Good point. In that case I'd be in favor of keeping common.js in here. > > @Sebastian What do you think? > Don't we also need ext.windows in order to open a new page? That can't be moved > to common.js. However in popup.html we already have ext. And in > notification.html (which is still only used on Chrome) you can retrieve ext via > chrome.extension.getBackgroundPage().ext. In that case maybe we can still import ext/common.js into notification.html and use: backgroundPage.ext.windows.getLastFocused(function(win) { win.openTab(link.href); }); to open new tab, not to make changes in notification.js while both safari and chrome use it in popup.html ?
Guys, I've uploaded a new patch, Please have a look and let me know if we are Okey with it.
http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... File notification.html (right): http://codereview.adblockplus.org/5767063142400000/diff/5639274879778816/noti... notification.html:29: <script src="ext/common.js"></script> On 2014/03/05 14:36:26, saroyanm wrote: > On 2014/03/05 14:11:56, Sebastian Noack wrote: > > On 2014/03/05 13:51:48, Thomas Greiner wrote: > > > On 2014/03/05 13:44:28, saroyanm wrote: > > > > On 2014/03/05 13:24:47, Thomas Greiner wrote: > > > > > We don't need ext/common.js anymore if we remove notification.html from > > > Safari > > > > > and get rid of references to ext.*. > > > > > > > > notification.js also imported in popup.html, so we can't use only chrome > API > > > > there I guess, maybe we can still have this import here, what you think ? > > > > > > Good point. In that case I'd be in favor of keeping common.js in here. > > > @Sebastian What do you think? > > Don't we also need ext.windows in order to open a new page? That can't be > moved > > to common.js. However in popup.html we already have ext. And in > > notification.html (which is still only used on Chrome) you can retrieve ext > via > > chrome.extension.getBackgroundPage().ext. > In that case maybe we can still import ext/common.js into notification.html and > use: > backgroundPage.ext.windows.getLastFocused(function(win) { > win.openTab(link.href); }); > to open new tab, not to make changes in notification.js while both safari and > chrome use it in popup.html ? I would still prefer something like this at the top of notification.js: if (!("ext" in window)) window.ext = chrome.extension.getBackgroundPage().ext; Until now in all JavaScript files we have either the content script part or the background page part of the extension API exposed by ext. And having only the common subset available in notification.html is just not consistent, but confusing and might rather quickly lead to bugs when working on that code in the future.
After discussion with Sebastian the new patch is uploaded, Please have a look guys when you will have time.
LGTM, with the nit addressed. http://codereview.adblockplus.org/5767063142400000/diff/5643440998055936/back... File background.js (right): http://codereview.adblockplus.org/5767063142400000/diff/5643440998055936/back... background.js:351: if (require("info").platform == "chromium" && "notifications" in chrome && navigator.platform.indexOf("Linux") == -1) Nit: Those are two very long lines. We usually try to keep lines up to 80 characters wide. I would suggest to wrap the if-statement like following: if (require("info").platform == "chromium" && "notifications" in chrome && navigator.platform.indexOf("Linux") == -1) { ... }
Nit fixed. http://codereview.adblockplus.org/5767063142400000/diff/5643440998055936/back... File background.js (right): http://codereview.adblockplus.org/5767063142400000/diff/5643440998055936/back... background.js:351: if (require("info").platform == "chromium" && "notifications" in chrome && navigator.platform.indexOf("Linux") == -1) On 2014/03/06 09:58:31, Sebastian Noack wrote: > Nit: Those are two very long lines. We usually try to keep lines up to 80 > characters wide. I would suggest to wrap the if-statement like following: > > if (require("info").platform == "chromium" && > "notifications" in chrome && > navigator.platform.indexOf("Linux") == -1) > { > ... > } Will keep in mind,thanks. Done.
Even more LGTM ;)
LGTM
:) Finally, thanks guys. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
