Issue 5630329503088640: Fixed miscellaneous issues with anti-adblock notification port (Closed)
|
Created:
March 26, 2014, 10:30 a.m. by Thomas Greiner Modified:
March 28, 2014, 6:50 p.m. Reviewers:
Felix Dahlke Visibility:
Public. |
DescriptionThis review includes the following issues:
- ActiveFilter is no longer needed in background.js
- notification isn't shown again if closed or clicked on
- added proper Safari support for URL-specific notifications
In addition to that I also allowed for chrome.notifications to be used on Linux
for Chrome 35+ since I was able to verify that it is indeed working now on
Chrome's dev channel.
Patch Set 1 #
Total comments: 7
Patch Set 2 : #
Total comments: 5
MessagesTotal messages: 7
After breaking the latest build I decided to do some in-depth testing of the complete featureset included in the anti-adblock notification port. This review fixes all issues that I found and also adds preliminary support for Safari. Keep in mind that we don't receive main_frame requests in onBeforeRequest on Safari which is, however, necessary to fully support URL-specific notifications.
Looks good, some comments. http://codereview.adblockplus.org/5630329503088640/diff/5629499534213120/back... File background.js (right): http://codereview.adblockplus.org/5630329503088640/diff/5629499534213120/back... background.js:358: // Chrome on Linux does not fully support chrome.notifications until version 35 Since this is executed immediately, I would have expected it at the bottom. How about putting this in some "initChromeNotifications" function (or whatever) and calling that at the bottom? http://codereview.adblockplus.org/5630329503088640/diff/5629499534213120/back... background.js:360: var canUseChromeNotifications = (require("info").platform == "chromium" && "notifications" in chrome && (navigator.platform.indexOf("Linux") == -1 || parseInt(require("info").applicationVersion) > 34)); The outer parentheses are redundant here. And I'd be in favor of wrapping this at the &&s, super long line :D http://codereview.adblockplus.org/5630329503088640/diff/5629499534213120/webr... File webrequest.js (right): http://codereview.adblockplus.org/5630329503088640/diff/5629499534213120/webr... webrequest.js:65: if (platform != "chromium" && type == "sub_frame") Sounds like we should check for Safari here, instead of for !Chromium. Can we?
http://codereview.adblockplus.org/5630329503088640/diff/5629499534213120/back... File background.js (right): http://codereview.adblockplus.org/5630329503088640/diff/5629499534213120/back... background.js:358: // Chrome on Linux does not fully support chrome.notifications until version 35 On 2014/03/28 13:10:41, Felix H. Dahlke wrote: > Since this is executed immediately, I would have expected it at the bottom. How > about putting this in some "initChromeNotifications" function (or whatever) and > calling that at the bottom? Done. http://codereview.adblockplus.org/5630329503088640/diff/5629499534213120/back... background.js:360: var canUseChromeNotifications = (require("info").platform == "chromium" && "notifications" in chrome && (navigator.platform.indexOf("Linux") == -1 || parseInt(require("info").applicationVersion) > 34)); On 2014/03/28 13:10:41, Felix H. Dahlke wrote: > The outer parentheses are redundant here. And I'd be in favor of wrapping this > at the &&s, super long line :D Done. http://codereview.adblockplus.org/5630329503088640/diff/5629499534213120/webr... File webrequest.js (right): http://codereview.adblockplus.org/5630329503088640/diff/5629499534213120/webr... webrequest.js:65: if (platform != "chromium" && type == "sub_frame") On 2014/03/28 13:10:41, Felix H. Dahlke wrote: > Sounds like we should check for Safari here, instead of for !Chromium. Can we? Should we? It's running in onHeadersReceived on "chromium" so doesn't it make sense to have it run in onBeforeRequest for all other platforms? To clarify that, we could also replace |platform == "chromium"| with a variable |canUseOnHeadersReceived| and use that one instead of the platform checks. If you disagree, I don't mind changing it to |platform == "safari"| though. http://codereview.adblockplus.org/5630329503088640/diff/5639274879778816/back... File background.js (right): http://codereview.adblockplus.org/5630329503088640/diff/5639274879778816/back... background.js:384: clearActiveNotification(notificationId); I noticed that even notifications that were dismissed by clicking on one of the buttons were only hidden in the notification center and not cleared so I added this here.
http://codereview.adblockplus.org/5630329503088640/diff/5629499534213120/webr... File webrequest.js (right): http://codereview.adblockplus.org/5630329503088640/diff/5629499534213120/webr... webrequest.js:65: if (platform != "chromium" && type == "sub_frame") On 2014/03/28 15:57:08, Thomas Greiner wrote: > On 2014/03/28 13:10:41, Felix H. Dahlke wrote: > > Sounds like we should check for Safari here, instead of for !Chromium. Can we? > > Should we? It's running in onHeadersReceived on "chromium" so doesn't it make > sense to have it run in onBeforeRequest for all other platforms? > To clarify that, we could also replace |platform == "chromium"| with a variable > |canUseOnHeadersReceived| and use that one instead of the platform checks. > > If you disagree, I don't mind changing it to |platform == "safari"| though. Hm, you've got a point, alright! http://codereview.adblockplus.org/5630329503088640/diff/5639274879778816/back... File background.js (right): http://codereview.adblockplus.org/5630329503088640/diff/5639274879778816/back... background.js:371: if (activeNotification && activeNotification.type != "question") Why check for the notification type here? Seems to me this should be done for all notifications that trigger a chrome.notification to be shown.
Sorry for the lengthy comment. :) http://codereview.adblockplus.org/5630329503088640/diff/5639274879778816/back... File background.js (right): http://codereview.adblockplus.org/5630329503088640/diff/5639274879778816/back... background.js:371: if (activeNotification && activeNotification.type != "question") On 2014/03/28 16:16:39, Felix H. Dahlke wrote: > Why check for the notification type here? Seems to me this should be done for > all notifications that trigger a chrome.notification to be shown. What this function does is remove the notification from the notification center - not close the notification if that's what you thought. It'd be nice to use the notification center for non-interactive notifications but we can also go with your suggestion for now. Additional context: While it's no problem that regular notifications are being kept in there, in case users want to check for them, it's problematic for URL-specific notifications because we can only show a notification again if it's not hidden in the notification center. In addition to that, we only save the last notification which means that interacting with old notifications in the notificiation center wouldn't do anything. So actually, we have three options: 1) Don't show any notifications in the notification center. 2) Change the check to the one below to only show notifications in the notification center which do not offer any interaction. if (activeNotification && (activeNotification.type == "question" || "links" in activeNotification)) 3) Keep all notifications in memory until they are properly closed to make use of the notification center. While I chose to go with (2) to at least keep non-interactive notifications in the notification center, I could also live with (1). (3) would require more effort to implement and is therefore not an option for now IMHO.
LGTM http://codereview.adblockplus.org/5630329503088640/diff/5639274879778816/back... File background.js (right): http://codereview.adblockplus.org/5630329503088640/diff/5639274879778816/back... background.js:371: if (activeNotification && activeNotification.type != "question") On 2014/03/28 17:26:51, Thomas Greiner wrote: > On 2014/03/28 16:16:39, Felix H. Dahlke wrote: > > Why check for the notification type here? Seems to me this should be done for > > all notifications that trigger a chrome.notification to be shown. > > What this function does is remove the notification from the notification center > - not close the notification if that's what you thought. It'd be nice to use the > notification center for non-interactive notifications but we can also go with > your suggestion for now. > > Additional context: > > While it's no problem that regular notifications are being kept in there, in > case users want to check for them, it's problematic for URL-specific > notifications because we can only show a notification again if it's not hidden > in the notification center. > > In addition to that, we only save the last notification which means that > interacting with old notifications in the notificiation center wouldn't do > anything. So actually, we have three options: > > 1) Don't show any notifications in the notification center. > > 2) Change the check to the one below to only show notifications in the > notification center which do not offer any interaction. > > if (activeNotification && (activeNotification.type == "question" || "links" in > activeNotification)) > > 3) Keep all notifications in memory until they are properly closed to make use > of the notification center. > > While I chose to go with (2) to at least keep non-interactive notifications in > the notification center, I could also live with (1). (3) would require more > effort to implement and is therefore not an option for now IMHO. Ah, no I get it. No it's fine let's go with 2 if you prefer that.
http://codereview.adblockplus.org/5630329503088640/diff/5639274879778816/back... File background.js (right): http://codereview.adblockplus.org/5630329503088640/diff/5639274879778816/back... background.js:371: if (activeNotification && activeNotification.type != "question") For completeness' sake, what I meant for (2) was: if (activeNotification && activeNotification.type != "question" && !("links" in activeNotification)) 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||
