Issue 29567749: Issue 5593 - Use messaging for the popup's notification code (Closed)
|
Created:
Oct. 6, 2017, 1:07 p.m. by kzar Modified:
Oct. 9, 2017, 4:49 p.m. CC:
Thomas Greiner Visibility:
Public. |
DescriptionIssue 5593 - Use messaging for the popup's notification code
Depends on these reviews:
- https://codereview.adblockplus.org/29567746/
- https://codereview.adblockplus.org/29567743/
Patch Set 1 #
Total comments: 16
Patch Set 2 : Use notifications.get message, addressed Manish's feedback #
Total comments: 3
Patch Set 3 : Replace !== with !=, === with == #
Total comments: 15
Patch Set 4 : Addressed Manish's feedback #
Total comments: 2
Patch Set 5 : Use chrome.tabs.create #Patch Set 6 : Update dependencies #Patch Set 7 : Rebased #
MessagesTotal messages: 18
Patch Set 1 Obviously the dependencies are placeholders until the related changes land.
Overall I wonder if some of the roundtrips to the background page could be reduced to just one called "notifications.getNotificationForDisplay" or something similar and have it return all the needed information at once. More comments inline. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:32: ext.backgroundPage.sendMessage({ We should use runtime.sendMessage here so we lose the dependence on ext.*, we're doing the same in popup.js https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:42: function getActiveDisplayedNotification() Nit: I feel like "displayed" implies it's already displayed, whereas we're about to display it here, so perhaps getActiveNotificationForDisplay. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:48: }, notification => I may be wrong about this, but "notification =>" here (the second argument) needs to start on the next line, at least according to ESLint's new indentation rules (which we have disabled for now). Same for "shouldDisplay =>" below. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:50: if (!notification) We would normally add braces here around the if block, if I'm not wrong. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:57: notificationType: notification.notificationType It should be notification.type, not notification.notificationType? At least that's the impression I got from the previous code. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:101: notification I'm not sure how message.locale gets in there. Is it added by the browser automatically? https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:112: messageElement.addEventListener("click", event => I feel like this should be added in the getDocLinks handler, after insertMessage. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:136: ext.backgroundPage.sendMessage({ You meant to use the togglePref function here, defined in popup.js?
Patch Set 2 : Use notifications.get message, addressed Manish's feedback https://codereview.adblockplus.org/29567749/diff/29567750/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:32: ext.backgroundPage.sendMessage({ On 2017/10/06 14:26:04, Manish Jethani wrote: > We should use runtime.sendMessage here so we lose the dependence on ext.*, we're > doing the same in popup.js Done. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:42: function getActiveDisplayedNotification() On 2017/10/06 14:26:04, Manish Jethani wrote: > Nit: I feel like "displayed" implies it's already displayed, whereas we're about > to display it here, so perhaps getActiveNotificationForDisplay. Acknowledged. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:48: }, notification => On 2017/10/06 14:26:05, Manish Jethani wrote: > I may be wrong about this, but "notification =>" here (the second argument) > needs to start on the next line, at least according to ESLint's new indentation > rules (which we have disabled for now). > > Same for "shouldDisplay =>" below. I think this looks OK like it is. If we decide to enforce that rule with ESLint in the future we can adjust it though. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:50: if (!notification) On 2017/10/06 14:26:05, Manish Jethani wrote: > We would normally add braces here around the if block, if I'm not wrong. IIRC we considered enforcing that braces for if... else... blocks should be consistent but decided against it. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:57: notificationType: notification.notificationType On 2017/10/06 14:26:05, Manish Jethani wrote: > It should be notification.type, not notification.notificationType? At least > that's the impression I got from the previous code. Acknowledged. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:101: notification On 2017/10/06 14:26:04, Manish Jethani wrote: > I'm not sure how message.locale gets in there. Is it added by the browser > automatically? If not specified we go with the default I think. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:112: messageElement.addEventListener("click", event => On 2017/10/06 14:26:04, Manish Jethani wrote: > I feel like this should be added in the getDocLinks handler, after > insertMessage. Done. https://codereview.adblockplus.org/29567749/diff/29567750/notification.js#new... notification.js:136: ext.backgroundPage.sendMessage({ On 2017/10/06 14:26:04, Manish Jethani wrote: > You meant to use the togglePref function here, defined in popup.js? Done.
https://codereview.adblockplus.org/29567749/diff/29567847/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29567847/notification.js#new... notification.js:91: while (link && link !== messageElement && link.localName !== "a") Nit: As per the Mozilla coding style guide, which is referenced in our coding style guide, we prefer == and != over of === and !==.
Patch Set 3 : Replace !== with !=, === with == https://codereview.adblockplus.org/29567749/diff/29567847/dependencies File dependencies (right): https://codereview.adblockplus.org/29567749/diff/29567847/dependencies#newcode5 dependencies:5: adblockplusui = adblockplusui git:5835-notification-message-handlers (Due to rebase, ignore.) https://codereview.adblockplus.org/29567749/diff/29567847/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29567847/notification.js#new... notification.js:91: while (link && link !== messageElement && link.localName !== "a") On 2017/10/08 01:19:21, Sebastian Noack wrote: > Nit: As per the Mozilla coding style guide, which is referenced in our coding > style guide, we prefer == and != over of === and !==. Done.
LGTM https://codereview.adblockplus.org/29567749/diff/29569558/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:18: /* global togglePref */ As I already told Manish on a different review, I'd like to see popup.js, notifications.js and stats.js being merged into one file, rather than having them share globals across files. Regardless, of the situation with the globals, I think it is is rather confusing to have the code for the popup spread out that much. But of course, this should be done with a seperate patch.
https://codereview.adblockplus.org/29567749/diff/29569558/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:24: let docLinks = []; The variable is unnecessary here. https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:32: return new Promise((resolve, reject) => If you prefer it, you could lose the braces and the return keyword and lose one level of indentation here. Also, reject is not being used so it could be skipped (then you could lose another level of indentation too). Just a suggestion, it looks fine as it is. https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:112: togglePref("notifications_ignoredcategories"); The "prefs.toggle" implementation in messageResponder.js does not pass true as the second argument here, which means it will actually get toggled: if (message.key == "notifications_ignoredcategories") return NotificationStorage.toggleIgnoreCategory("*"); Really we want to set the value to true, like so: if (message.key == "notifications_ignoredcategories") return NotificationStorage.toggleIgnoreCategory("*", true); But it's also called from elsewhere. I think we have to pass the value of message.forceValue here. Either that, or perhaps have a dedicated message for this, since we aren't really "toggling" here, we're just setting the value.
https://codereview.adblockplus.org/29567749/diff/29569558/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:112: togglePref("notifications_ignoredcategories"); On 2017/10/08 22:31:34, Manish Jethani wrote: > [...] > > Either that, or perhaps have a dedicated message for this, since we aren't > really "toggling" here, we're just setting the value. Since we're adding "notifications.get", we could add another one called "notifications.ignoreAllCategories" or "notifications.optOut"
https://codereview.adblockplus.org/29567749/diff/29569558/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:18: /* global togglePref */ On 2017/10/08 19:17:22, Sebastian Noack wrote: > As I already told Manish on a different review, I'd like to see popup.js, > notifications.js and stats.js being merged into one file, rather than having > them share globals across files. Regardless, of the situation with the globals, > I think it is is rather confusing to have the code for the popup spread out that > much. But of course, this should be done with a seperate patch. Acknowledged. https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:24: let docLinks = []; On 2017/10/08 22:31:34, Manish Jethani wrote: > The variable is unnecessary here. Sure, we could return an empty array if notification.links is falsey and then initialise the docLinks variable to an empty array otherwise. I preferred it this way since the same empty array is used, but whatever. If you insist I'll change it, I don't care too much either way. https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:32: return new Promise((resolve, reject) => On 2017/10/08 22:31:34, Manish Jethani wrote: > If you prefer it, you could lose the braces and the return keyword and lose one > level of indentation here. > > Also, reject is not being used so it could be skipped (then you could lose > another level of indentation too). > > Just a suggestion, it looks fine as it is. Yea I considered that, but I figured it looked nicer with the braces in this instance. I don't feel too strongly either way, I guess I'll leave it like it is. https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:112: togglePref("notifications_ignoredcategories"); On 2017/10/08 22:31:34, Manish Jethani wrote: > The "prefs.toggle" implementation in messageResponder.js does not pass true as > the second argument here, which means it will actually get toggled: Well my understanding was the option would be toggled, so if it's true it would be come false and vice versa. I just checked the implementation and it looks about right, so I'm not sure I agree that the value won't actually get toggled. Am I missing something? > Really we want to set the value to true, like so: > > Either that, or perhaps have a dedicated message for this, since we aren't > really "toggling" here, we're just setting the value. This is true, and kind of contradicts what you said above. Anyway since ignored notifications won't be displayed or therefore clicked, we can assume the notification _isn't_ ignored at this point right? Therefore we can assume that toggling it will set it to be ignored.
https://codereview.adblockplus.org/29567749/diff/29569558/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:24: let docLinks = []; On 2017/10/09 10:33:25, kzar wrote: > On 2017/10/08 22:31:34, Manish Jethani wrote: > > The variable is unnecessary here. > > Sure, we could return an empty array if notification.links is falsey and then > initialise the docLinks variable to an empty array otherwise. I preferred it > this way since the same empty array is used, but whatever. If you insist I'll > change it, I don't care too much either way. I must be looking at the wrong diff, because to me it looks like it's only being used in one place. https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:32: return new Promise((resolve, reject) => On 2017/10/09 10:33:25, kzar wrote: > On 2017/10/08 22:31:34, Manish Jethani wrote: > > If you prefer it, you could lose the braces and the return keyword and lose > one > > level of indentation here. > > > > Also, reject is not being used so it could be skipped (then you could lose > > another level of indentation too). > > > > Just a suggestion, it looks fine as it is. > > Yea I considered that, but I figured it looked nicer with the braces in this > instance. I don't feel too strongly either way, I guess I'll leave it like it > is. Acknowledged. https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:112: togglePref("notifications_ignoredcategories"); On 2017/10/09 10:33:25, kzar wrote: > On 2017/10/08 22:31:34, Manish Jethani wrote: > > The "prefs.toggle" implementation in messageResponder.js does not pass true as > > the second argument here, which means it will actually get toggled: > > Well my understanding was the option would be toggled, so if it's true it would > be come false and vice versa. I just checked the implementation and it looks > about right, so I'm not sure I agree that the value won't actually get toggled. > Am I missing something? The reason the original code was passing true as the second argument is to force the value to be true, regardless of the current value. I'm not sure if it matters though, as you said, but see below. > > Really we want to set the value to true, like so: > > > > Either that, or perhaps have a dedicated message for this, since we aren't > > really "toggling" here, we're just setting the value. > > This is true, and kind of contradicts what you said above. Anyway since ignored > notifications won't be displayed or therefore clicked, we can assume the > notification _isn't_ ignored at this point right? Therefore we can assume that > toggling it will set it to be ignored. "Relentless" notifications are always displayed, I think, regardless of the setting. Anyway I'm not sure that passing true or not makes a difference, maybe Thomas can throw some light on this. If you're sure though then it looks OK to me.
https://codereview.adblockplus.org/29567749/diff/29569558/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:112: togglePref("notifications_ignoredcategories"); On 2017/10/09 10:54:12, Manish Jethani wrote: > [...] > > "Relentless" notifications are always displayed, I think, regardless of the > setting. Anyway I'm not sure that passing true or not makes a difference, maybe > Thomas can throw some light on this. If you're sure though then it looks OK to > me. Looking at this code in adblockpluscore lib/notification.js: if (notification.type !== "relentless" && Prefs.notifications_ignoredcategories.indexOf("*") != -1) { continue; } So it seems even if the user has opted out of all notifications, the "relentless" type are still displayed, and in that case the user would have the option of clicking on the opt out button again (I'm not sure this is good UX but whatever). If they choose to "opt out" in this scenario, they would end up opting in instead. So I think we need to pass true there as the second argument.
Patch Set 4 : Addressed Manish's feedback https://codereview.adblockplus.org/29567749/diff/29569558/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:24: let docLinks = []; On 2017/10/09 10:54:12, Manish Jethani wrote: > On 2017/10/09 10:33:25, kzar wrote: > > On 2017/10/08 22:31:34, Manish Jethani wrote: > > > The variable is unnecessary here. > > > > Sure, we could return an empty array if notification.links is falsey and then > > initialise the docLinks variable to an empty array otherwise. I preferred it > > this way since the same empty array is used, but whatever. If you insist I'll > > change it, I don't care too much either way. > > I must be looking at the wrong diff, because to me it looks like it's only being > used in one place. Oh yea, you are right sorry. I switched to mapping over notification.links directly. Done. https://codereview.adblockplus.org/29567749/diff/29569558/notification.js#new... notification.js:112: togglePref("notifications_ignoredcategories"); On 2017/10/09 11:53:52, Manish Jethani wrote: > On 2017/10/09 10:54:12, Manish Jethani wrote: > > [...] > > > > "Relentless" notifications are always displayed, I think, regardless of the > > setting. Anyway I'm not sure that passing true or not makes a difference, > maybe > > Thomas can throw some light on this. If you're sure though then it looks OK to > > me. > > Looking at this code in adblockpluscore lib/notification.js: > > if (notification.type !== "relentless" && > Prefs.notifications_ignoredcategories.indexOf("*") != -1) > { > continue; > } > > So it seems even if the user has opted out of all notifications, the > "relentless" type are still displayed, and in that case the user would have the > option of clicking on the opt out button again (I'm not sure this is good UX but > whatever). If they choose to "opt out" in this scenario, they would end up > opting in instead. So I think we need to pass true there as the second argument. Done.
LGTM, modulo the dependency update.
https://codereview.adblockplus.org/29567749/diff/29570608/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29570608/notification.js#new... notification.js:95: ext.pages.open(link.href); ext.pages.open() doesn't exist anymore: https://hg.adblockplus.org/adblockpluschrome/rev/faf3d97ad473
Patch Set 5 : Use chrome.tabs.create (Untested so far.) https://codereview.adblockplus.org/29567749/diff/29570608/notification.js File notification.js (right): https://codereview.adblockplus.org/29567749/diff/29570608/notification.js#new... notification.js:95: ext.pages.open(link.href); On 2017/10/09 15:40:17, Sebastian Noack wrote: > ext.pages.open() doesn't exist anymore: > https://hg.adblockplus.org/adblockpluschrome/rev/faf3d97ad473 Done.
LGTM, once the dependency got updated.
Patch Set 6 : Update dependencies
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
