Issue 5538776168267776: Implemented anti-adblock message notification (Closed)
|
Created:
Feb. 7, 2014, 5:08 p.m. by Thomas Greiner Modified:
Feb. 13, 2014, 1:14 p.m. Reviewers:
Felix Dahlke Visibility:
Public. |
DescriptionImplementation overview:
- On first startup: Download filterlist but don't enable it yet.
- On startup: Add notification with domains mentioned in filterlist.
- On new frame: Show notification with following options:
- yes: enable filterlist, don't show notification again
- no: disable filterlist, don't show notification again
- cancel: show notification again
- Handling filterlist changes.
Extended notification mechanism as follows:
- "message" property can now also be a string
- renamed "severity" into "type"
- added "question" type
- added methods for adding, removing and marking notifications as shown
- getNextToShow can now be passed an URL
Patch Set 1 #
Total comments: 29
Patch Set 2 : #
Total comments: 13
Patch Set 3 : #
Total comments: 13
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
MessagesTotal messages: 12
To be honest I'm not too happy with the matcher approach but all alternatives seem to have quite a few drawbacks (see comment) and I want to avoid manually checking the domains so let me know if you have a good idea for that specific problem. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... File lib/notification.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:189: let matcher = new Matcher(); Let me know if you don't like this approach. However, if we want to create the matcher beforehand we'd need to store it somewhere and I couldn't find a good location for that: - attaching it to the notification would be problematic due to it being stored in serialized form) - having the data separately means that we'd need to keep it synchronized with the notification
Almost done with the review, but I didn't get to lib/ui.js yet. Some of my comments would probably disappear once I've reviewed that too, but I figured I'd give you something to work with for now, since I won't be able to finish this before the afternoon today. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... File chrome/content/ui/overlay.xul (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... chrome/content/ui/overlay.xul:127: <html:p id="abp-notification-message"/> Didn't we discuss that questions should be shown in a notification box in Firefox? Note that the notification panel disappears automatically after some time, people can simply ignore it, which we don't want. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... File chrome/locale/en-US/global.properties (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... chrome/locale/en-US/global.properties:58: notification_antiadblock_message=This site is known to show targeted messages to Adblock Plus users. Do you want Adblock Plus to hide these everywhere? I suggest some modifications: 1. s/is known/has been known/ (I think the former isn't really correct here) 2. s/to Adblock Plus users/to users of ad blockers/ (Arguably somewhat controversial. Do we want to point out that there are other ad blockers? But on the other hand, do we want to suggest that the sites are targeting us specifically? Just a suggestion, don't have a strong opinion. Feel free to go with what you prefer here.) 3. s/hide these everywhere/hide targeted messages/ - possibly even "hide targeted messages like these" (I think it makes sense to be very clear here.) http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... File chrome/locale/en-US/overlay.dtd (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... chrome/locale/en-US/overlay.dtd:40: <!ENTITY notification.button.close "Close"> I think these should all have an access key. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... File lib/notification.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:42: return (notification.type in TYPES ? TYPES[notification.type] : TYPES.information); I think we should be backwards compatible here, we won't change the name of this field on the server right away, and if we do, we can't show notifications to all users in the migration period. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:181: if ((typeof notification.type === "undefined" || notification.type === "information" || notification.type === "question") I think this would make more sense now: if ((typeof notification.type === "undefined" || notification.type !== "warning") http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:185: if (typeof url === "string" || notification.domains instanceof Array) notification.domains are not necessarily domains if it's arbitrary filters, so I think it needs another name. notification.urls? notification.urlFilters? http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:189: let matcher = new Matcher(); On 2014/02/07 17:29:17, Thomas Greiner wrote: > Let me know if you don't like this approach. However, if we want to create the > matcher beforehand we'd need to store it somewhere and I couldn't find a good > location for that: > - attaching it to the notification would be problematic due to it being stored > in serialized form) > - having the data separately means that we'd need to keep it synchronized with > the notification Pretty fine to me, wouldn't like to complicate this without any real performance issues. And I don't expect any here, from what I see, there's not much overhead. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:196: continue; So if the url parameter is passed, we ignore all notifications that don't have notification.domains? Seems wrong to me. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:216: activeNotifications.push(notification); Why did you change this logic? It looks to me like we're only ever interested in the notification with the highest priority anyway. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:294: if (localData.indexOf(notification) > -1) Nit: Save the index to a temp instead of calling indexOf twice. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/ui.js:847: if (Services.vc.compare(prevVersion, "2.5") < 0) I think we should show them to every user once actually.
http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... File chrome/content/ui/overlay.xul (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... chrome/content/ui/overlay.xul:127: <html:p id="abp-notification-message"/> On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > Didn't we discuss that questions should be shown in a notification box in > Firefox? Note that the notification panel disappears automatically after some > time, people can simply ignore it, which we don't want. Yes, but saying the extent of its constraints (see https://developer.mozilla.org/en/docs/Web/API/notification) it seems like we should stick with the existing panel in Firefox which allows us to add buttons. In ui.js I'm setting the "noautohide" attribute which makes it stay open even after it loses focus. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... File chrome/locale/en-US/global.properties (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... chrome/locale/en-US/global.properties:58: notification_antiadblock_message=This site is known to show targeted messages to Adblock Plus users. Do you want Adblock Plus to hide these everywhere? On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > I suggest some modifications: > > 1. s/is known/has been known/ (I think the former isn't really correct here) > 2. s/to Adblock Plus users/to users of ad blockers/ (Arguably somewhat > controversial. Do we want to point out that there are other ad blockers? But on > the other hand, do we want to suggest that the sites are targeting us > specifically? Just a suggestion, don't have a strong opinion. Feel free to go > with what you prefer here.) > 3. s/hide these everywhere/hide targeted messages/ - possibly even "hide > targeted messages like these" (I think it makes sense to be very clear here.) 2. I don't think we need to change that. It does say that the message is shown to ABP users but it doesn't say that it's not showing it to other people as well. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... File chrome/locale/en-US/overlay.dtd (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... chrome/locale/en-US/overlay.dtd:40: <!ENTITY notification.button.close "Close"> On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > I think these should all have an access key. Done. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... File lib/notification.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:42: return (notification.type in TYPES ? TYPES[notification.type] : TYPES.information); On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > I think we should be backwards compatible here, we won't change the name of this > field on the server right away, and if we do, we can't show notifications to all > users in the migration period. Done. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:181: if ((typeof notification.type === "undefined" || notification.type === "information" || notification.type === "question") On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > I think this would make more sense now: > > if ((typeof notification.type === "undefined" || notification.type !== > "warning") Done. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:185: if (typeof url === "string" || notification.domains instanceof Array) On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > notification.domains are not necessarily domains if it's arbitrary filters, so I > think it needs another name. notification.urls? notification.urlFilters? Done. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:196: continue; On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > So if the url parameter is passed, we ignore all notifications that don't have > notification.domains? Seems wrong to me. This means that some notifications could be shown for any URL. Can you think of a good use-case for that? http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:216: activeNotifications.push(notification); On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > Why did you change this logic? It looks to me like we're only ever interested in > the notification with the highest priority anyway. Done. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:294: if (localData.indexOf(notification) > -1) On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > Nit: Save the index to a temp instead of calling indexOf twice. Done. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/ui.js:847: if (Services.vc.compare(prevVersion, "2.5") < 0) On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > I think we should show them to every user once actually. This is just the initial download of the filterlist so this happens in the background and is not intended to be shown to the user.
LGTM http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... File chrome/content/ui/overlay.xul (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... chrome/content/ui/overlay.xul:127: <html:p id="abp-notification-message"/> On 2014/02/11 16:53:31, Thomas Greiner wrote: > On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > > Didn't we discuss that questions should be shown in a notification box in > > Firefox? Note that the notification panel disappears automatically after some > > time, people can simply ignore it, which we don't want. > > Yes, but saying the extent of its constraints (see > https://developer.mozilla.org/en/docs/Web/API/notification) it seems like we > should stick with the existing panel in Firefox which allows us to add buttons. > > In ui.js I'm setting the "noautohide" attribute which makes it stay open even > after it loses focus. I meant a notification box, not a desktop notification. These things: https://developer.mozilla.org/en-US/docs/XUL/notificationbox?redirectlocale=e... http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... File chrome/locale/en-US/global.properties (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... chrome/locale/en-US/global.properties:58: notification_antiadblock_message=This site is known to show targeted messages to Adblock Plus users. Do you want Adblock Plus to hide these everywhere? On 2014/02/11 16:53:31, Thomas Greiner wrote: > On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > > I suggest some modifications: > > > > 1. s/is known/has been known/ (I think the former isn't really correct here) > > 2. s/to Adblock Plus users/to users of ad blockers/ (Arguably somewhat > > controversial. Do we want to point out that there are other ad blockers? But > on > > the other hand, do we want to suggest that the sites are targeting us > > specifically? Just a suggestion, don't have a strong opinion. Feel free to go > > with what you prefer here.) > > 3. s/hide these everywhere/hide targeted messages/ - possibly even "hide > > targeted messages like these" (I think it makes sense to be very clear here.) > > 2. I don't think we need to change that. It does say that the message is shown > to ABP users but it doesn't say that it's not showing it to other people as > well. Alright, let's leave it then. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... File lib/notification.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/notification.js:196: continue; On 2014/02/11 16:53:31, Thomas Greiner wrote: > On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > > So if the url parameter is passed, we ignore all notifications that don't have > > notification.domains? Seems wrong to me. > > This means that some notifications could be shown for any URL. Can you think of > a good use-case for that? Ah, I thought I'd change my mind on this after reviewing lib/ui.js. Indeed, this function is called twice: Once with and once without URL. So yeah, fine.
Oh crap, was in the middle of reviewing this still, wrong window... it's _not_ yet LGTM. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/ui.js:554: if (subscription.url in FilterStorage.knownSubscriptions) Shouldn't we add it here if it's not in the knownSubscriptions?
Everything's reviewed now, sorry for the confusion! http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... File chrome/content/ui/overlay.xul (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... chrome/content/ui/overlay.xul:127: <html:p id="abp-notification-message"/> On 2014/02/11 17:11:12, Felix H. Dahlke wrote: > On 2014/02/11 16:53:31, Thomas Greiner wrote: > > On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > > > Didn't we discuss that questions should be shown in a notification box in > > > Firefox? Note that the notification panel disappears automatically after > some > > > time, people can simply ignore it, which we don't want. > > > > Yes, but saying the extent of its constraints (see > > https://developer.mozilla.org/en/docs/Web/API/notification) it seems like we > > should stick with the existing panel in Firefox which allows us to add > buttons. > > > > In ui.js I'm setting the "noautohide" attribute which makes it stay open even > > after it loses focus. > > I meant a notification box, not a desktop notification. These things: > https://developer.mozilla.org/en-US/docs/XUL/notificationbox?redirectlocale=e... I could live with doing this in a follow-up though. This is a big, important change, and we need to have it in the dev builds for a while before it's released. So feel free to leave things as they are for now and create a Trello card for that. http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/lib/... lib/ui.js:554: if (subscription.url in FilterStorage.knownSubscriptions) On 2014/02/11 17:19:44, Felix H. Dahlke wrote: > Shouldn't we add it here if it's not in the knownSubscriptions? Actually not, it's fine :) http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:533: // Add "anti-adblock messages" notification I think all the anti adblock message specific code should go to its own function (initAntiAdblockMessageBlocking?) - it's a lot of code. http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:551: onApproved: function() I really don't think we should be able to put code directly into the notification - it's a huge security issue. How about this: We put a map of notification listeners in notification.js, and add addListener/removeListener functions, maybe like this: Notification.addListener("antiadblock", function(approved) { ... }); http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:577: antiadblockNotification.urlFilters = getURLFiltersFromSubscription(value); This is repeated above. How about having a createAntiAdblockBlockingNotification(subscription) function instead? http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:586: var httpRequestObserver = { Shouldn't it be called documentCreatingObserver or something? This reacts to a page being loaded effectively, not individual HTTP requests, doesn't it? http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:847: if (Services.vc.compare(prevVersion, "2.5") < 0) I think we should do this for everyone, no matter what version. http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:1913: while (!link || link === messageElement || link.localName !== "a") Shouldn't this be an "if"? Anyway, why aren't we bubbling up anymore?
http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... File chrome/content/ui/overlay.xul (right): http://codereview.adblockplus.org/5538776168267776/diff/5629499534213120/chro... chrome/content/ui/overlay.xul:127: <html:p id="abp-notification-message"/> On 2014/02/11 17:57:17, Felix H. Dahlke wrote: > On 2014/02/11 17:11:12, Felix H. Dahlke wrote: > > On 2014/02/11 16:53:31, Thomas Greiner wrote: > > > On 2014/02/11 10:35:27, Felix H. Dahlke wrote: > > > > Didn't we discuss that questions should be shown in a notification box in > > > > Firefox? Note that the notification panel disappears automatically after > > some > > > > time, people can simply ignore it, which we don't want. > > > > > > Yes, but saying the extent of its constraints (see > > > https://developer.mozilla.org/en/docs/Web/API/notification) it seems like we > > > should stick with the existing panel in Firefox which allows us to add > > buttons. > > > > > > In ui.js I'm setting the "noautohide" attribute which makes it stay open > even > > > after it loses focus. > > > > I meant a notification box, not a desktop notification. These things: > > > https://developer.mozilla.org/en-US/docs/XUL/notificationbox?redirectlocale=e... > > I could live with doing this in a follow-up though. This is a big, important > change, and we need to have it in the dev builds for a while before it's > released. So feel free to leave things as they are for now and create a Trello > card for that. Ok, that's also fine with me. I wouldn't want to delay the initial release just because of that but I can work on it right after the release to push it soon after. http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:533: // Add "anti-adblock messages" notification On 2014/02/11 17:57:17, Felix H. Dahlke wrote: > I think all the anti adblock message specific code should go to its own function > (initAntiAdblockMessageBlocking?) - it's a lot of code. Done. http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:551: onApproved: function() On 2014/02/11 17:57:17, Felix H. Dahlke wrote: > I really don't think we should be able to put code directly into the > notification - it's a huge security issue. > > How about this: We put a map of notification listeners in notification.js, and > add addListener/removeListener functions, maybe like this: > > Notification.addListener("antiadblock", function(approved) > { > ... > }); You cannot include functions in JSON so they can only be added locally. But I prefer your approach so I changed it. http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:577: antiadblockNotification.urlFilters = getURLFiltersFromSubscription(value); On 2014/02/11 17:57:17, Felix H. Dahlke wrote: > This is repeated above. How about having a > createAntiAdblockBlockingNotification(subscription) function instead? Done. http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:586: var httpRequestObserver = { On 2014/02/11 17:57:17, Felix H. Dahlke wrote: > Shouldn't it be called documentCreatingObserver or something? This reacts to a > page being loaded effectively, not individual HTTP requests, doesn't it? Done. http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:847: if (Services.vc.compare(prevVersion, "2.5") < 0) On 2014/02/11 17:57:17, Felix H. Dahlke wrote: > I think we should do this for everyone, no matter what version. Done. http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:1913: while (!link || link === messageElement || link.localName !== "a") On 2014/02/11 17:57:17, Felix H. Dahlke wrote: > Shouldn't this be an "if"? > > Anyway, why aren't we bubbling up anymore? Done. The bubbling lead to the link opening no matter where you clicked on the panel. I fixed it now in a different way to allow bubbling again.
Starting to look good, some more comments. http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5733935958982656/lib/... lib/ui.js:551: onApproved: function() On 2014/02/12 11:58:49, Thomas Greiner wrote: > On 2014/02/11 17:57:17, Felix H. Dahlke wrote: > > I really don't think we should be able to put code directly into the > > notification - it's a huge security issue. > > > > How about this: We put a map of notification listeners in notification.js, and > > add addListener/removeListener functions, maybe like this: > > > > Notification.addListener("antiadblock", function(approved) > > { > > ... > > }); > > You cannot include functions in JSON so they can only be added locally. But I > prefer your approach so I changed it. Oh that's right of course, wouldn't work anyway. http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... File lib/notification.js (right): http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/notification.js:44: return (notification.severity in SEVERITY ? SEVERITY[notification.severity] : SEVERITY.information); I actually thought it's fine to call this TYPE, just that we should be downwards compatible. i.e.: let type = "type" in notification ? notification.type : notification.severity; return (type in TYPE ? TYPE[type] : TYPE.information); Not really important though, feel free to leave it as it is. Severity still makes some sort of sense here. http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/notification.js:154: * @param {String} url URL to match notifications to Would be nice to point out here that it's optional http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/notification.js:221: if (!notificationToShow || getNumericalSeverity(notification) > getNumericalSeverity(notificationToShow)) Can you restore the line break? To avoid unrelated changes. http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/notification.js:292: addListener: function(/**function(id, approved)*/ listener) I think it'd make more sense to register listeners per notification ID. Like this: addListener: function(/**string*/ id, /**function(approved)*/ listener) { if (!(id in listeners)) listeners[id] = []; if listeners[id].indexOf(listener) === -1) listeners[id].push(listener); } Also, I think we should call the whole thing "addQuestionListener" for clarity. http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/ui.js:401: initAntiAdblockNotification: function() Shouldn't this be "private"? http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/ui.js:403: let notification = { I think we should move this into addAntiAdblockNotification since it's only used there. Similarly, I think we should register the listener in addAntiAdblockNotification.
http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... File lib/notification.js (right): http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/notification.js:44: return (notification.severity in SEVERITY ? SEVERITY[notification.severity] : SEVERITY.information); On 2014/02/12 15:09:27, Felix H. Dahlke wrote: > I actually thought it's fine to call this TYPE, just that we should be downwards > compatible. i.e.: > > let type = "type" in notification ? notification.type : notification.severity; > return (type in TYPE ? TYPE[type] : TYPE.information); > > Not really important though, feel free to leave it as it is. Severity still > makes some sort of sense here. Done. http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/notification.js:154: * @param {String} url URL to match notifications to On 2014/02/12 15:09:27, Felix H. Dahlke wrote: > Would be nice to point out here that it's optional Done. http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/notification.js:221: if (!notificationToShow || getNumericalSeverity(notification) > getNumericalSeverity(notificationToShow)) On 2014/02/12 15:09:27, Felix H. Dahlke wrote: > Can you restore the line break? To avoid unrelated changes. Done. http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/notification.js:292: addListener: function(/**function(id, approved)*/ listener) On 2014/02/12 15:09:27, Felix H. Dahlke wrote: > I think it'd make more sense to register listeners per notification ID. Like > this: > > addListener: function(/**string*/ id, /**function(approved)*/ listener) > { > if (!(id in listeners)) > listeners[id] = []; > if listeners[id].indexOf(listener) === -1) > listeners[id].push(listener); > } > > Also, I think we should call the whole thing "addQuestionListener" for clarity. Done. Although from a consistency standpoint I'm more in favor of my approach (same as for FilterNotifier) but for now it's only used for this specific case so I can live with your suggestion. We can still change it whenever we need to in the future. http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/ui.js:401: initAntiAdblockNotification: function() On 2014/02/12 15:09:27, Felix H. Dahlke wrote: > Shouldn't this be "private"? Done. http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/ui.js:403: let notification = { On 2014/02/12 15:09:27, Felix H. Dahlke wrote: > I think we should move this into addAntiAdblockNotification since it's only used > there. Similarly, I think we should register the listener in > addAntiAdblockNotification. No, it's also used to remove the notification (see line 449). I moved the removal code into its own function now to make it clearer (and also added a call to Notification.removeQuestionListener in there).
Looking pretty good now, just one more thing really. http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... File lib/ui.js (right): http://codereview.adblockplus.org/5538776168267776/diff/6601759870943232/lib/... lib/ui.js:403: let notification = { On 2014/02/12 18:28:02, Thomas Greiner wrote: > On 2014/02/12 15:09:27, Felix H. Dahlke wrote: > > I think we should move this into addAntiAdblockNotification since it's only > used > > there. Similarly, I think we should register the listener in > > addAntiAdblockNotification. > > No, it's also used to remove the notification (see line 449). I moved the > removal code into its own function now to make it clearer (and also added a call > to Notification.removeQuestionListener in there). Oh, right. http://codereview.adblockplus.org/5538776168267776/diff/5203516087861248/lib/... File lib/notification.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5203516087861248/lib/... lib/notification.js:34: let TYPES = { Maybe we should call this TYPE_SEVERITY since it's effectively a type: severity mapping now? (also: singular) http://codereview.adblockplus.org/5538776168267776/diff/5203516087861248/lib/... lib/notification.js:187: if ((typeof notification.type === "undefined" || notification.type !== "critical") I'm sorry, my suggestion for being downwards compatible above wasn't complete, we need to do something here to :( How about this: When we download the notification, we do this: if ("severity" in notification) { if (!("type" in notification)) notification.type = notification.severity; delete notification.severity; } That way we could centralise the type/severity confusion.
http://codereview.adblockplus.org/5538776168267776/diff/5203516087861248/lib/... File lib/notification.js (right): http://codereview.adblockplus.org/5538776168267776/diff/5203516087861248/lib/... lib/notification.js:34: let TYPES = { On 2014/02/13 10:22:04, Felix H. Dahlke wrote: > Maybe we should call this TYPE_SEVERITY since it's effectively a type: severity > mapping now? (also: singular) I don't think we need to make it overly specific given that "information" and "critical" can also be considered notification types. http://codereview.adblockplus.org/5538776168267776/diff/5203516087861248/lib/... lib/notification.js:187: if ((typeof notification.type === "undefined" || notification.type !== "critical") On 2014/02/13 10:22:04, Felix H. Dahlke wrote: > I'm sorry, my suggestion for being downwards compatible above wasn't complete, > we need to do something here to :( How about this: > > When we download the notification, we do this: > > if ("severity" in notification) > { > if (!("type" in notification)) > notification.type = notification.severity; > delete notification.severity; > } > > That way we could centralise the type/severity confusion. Done.
Awesome, LGTM! 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
