Issue 29326181: Issue 3022 - Implemented new notification type for normal messages (Closed)
|
Created:
Sept. 9, 2015, 1:12 p.m. by Thomas Greiner Modified:
Sept. 11, 2015, 1:15 p.m. Reviewers:
Sebastian Noack CC:
Felix Dahlke Visibility:
Public. |
DescriptionIn addition to what's specified in the ticket description, this review also contains the following fixes:
1. Remove dead "question"-type notification code from icon popup because the `#notification-question` no longer exists anyway.
2. If a notification specifies an unknown type, don't try to animate the icon because this currently results in errors when trying to fetch the icon images because they don't exist.
Patch Set 1 #
Total comments: 2
Patch Set 2 : Centralized mapping information for types #
Total comments: 3
Patch Set 3 : #
Total comments: 1
MessagesTotal messages: 9
https://codereview.adblockplus.org/29326181/diff/29326182/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29326181/diff/29326182/lib/notificationHel... lib/notificationHelper.js:45: let animateIcon = (activeNotification.type == "critical" I wonder whether we should use global objects used as set, to configure the notification types to be animated, and types to trigger a desktop notification below. Then you'd have less complexity here, and can simply use the in-operator.
Just a sidenote: I think that, in the long term, it may make sense to decouple the display methods from notification types so that we can specify them directly on the server side. For now, centralizing that information should make it at least more maintainable, I agree. https://codereview.adblockplus.org/29326181/diff/29326182/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29326181/diff/29326182/lib/notificationHel... lib/notificationHelper.js:45: let animateIcon = (activeNotification.type == "critical" On 2015/09/09 13:35:36, Sebastian Noack wrote: > I wonder whether we should use global objects used as set, to configure the > notification types to be animated, and types to trigger a desktop notification > below. Then you'd have less complexity here, and can simply use the in-operator. Done. However, I decided to implement a function to avoid duplication of fallback-to-default logic and to avoid having to share the entire structure with notification.js.
https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... lib/notificationHelper.js:27: let displayMethods = { Please use Object.create(null) for objects used as map (or set). https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... lib/notificationHelper.js:28: _default: ["popup"], There is no reason why this is a prefixed property rather than stored in a separate variable. https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... lib/notificationHelper.js:261: return methods.indexOf(method) > -1; If you use objects for these "lists", you can use the in-operator here.
On 2015/09/09 17:38:58, Sebastian Noack wrote: > https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... > File lib/notificationHelper.js (right): > > https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... > lib/notificationHelper.js:27: let displayMethods = { > Please use Object.create(null) for objects used as map (or set). > > https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... > lib/notificationHelper.js:28: _default: ["popup"], > There is no reason why this is a prefixed property rather than stored in a > separate variable. > > https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... > lib/notificationHelper.js:261: return methods.indexOf(method) > -1; > If you use objects for these "lists", you can use the in-operator here. It appears that all these comments are personal preferences. Therefore I'm wondering which ones are optional or required for this review to finish.
On 2015/09/10 11:11:39, Thomas Greiner wrote: > It appears that all these comments are personal preferences. Therefore I'm > wondering which ones are optional or required for this review to finish. I don't think, they are personal preference. > https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... > > lib/notificationHelper.js:27: let displayMethods = { > > Please use Object.create(null) for objects used as map (or set). If you map keys to values, or check whether an item is in a set of items, the appropriate data structure is a hashtable. It's more efficient, and avoids collisions with built-in properties like "toString". In ES6 there are Map and Set objects for this very purpose. However, since we cannot use Map and Set for backwards compatibility, Object.create(null) it is. > https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... > > lib/notificationHelper.js:28: _default: ["popup"], > > There is no reason why this is a prefixed property rather than stored in a > > separate variable. Since this is CommonJS module, there is no reason to avoid additional global variables. And having one special key that is threaded differently, and even looked up separately, is inconsistent and makes the code unnecessarily hard to understand. > https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... > > lib/notificationHelper.js:261: return methods.indexOf(method) > -1; > > If you use objects for these "lists", you can use the in-operator here. As before, the appropriate data structure to check whether an item is in a set of items, would be a set or if not available an object. |x in obj| is more efficient and makes the code more readable than |arr.indexOf(x) != -1|. However, I see that this will introduce some boilerplate when defining the data structure. So feel free to ignore that one.
On 2015/09/10 11:36:02, Sebastian Noack wrote: > On 2015/09/10 11:11:39, Thomas Greiner wrote: > > It appears that all these comments are personal preferences. Therefore I'm > > wondering which ones are optional or required for this review to finish. > > I don't think, they are personal preference. > > > > https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... > > > lib/notificationHelper.js:27: let displayMethods = { > > > Please use Object.create(null) for objects used as map (or set). > > If you map keys to values, or check whether an item is in a set of items, the > appropriate data structure is a hashtable. It's more efficient, and avoids > collisions with built-in properties like "toString". In ES6 there are Map and > Set objects for this very purpose. However, since we cannot use Map and Set for > backwards compatibility, Object.create(null) it is. > > > > https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... > > > lib/notificationHelper.js:28: _default: ["popup"], > > > There is no reason why this is a prefixed property rather than stored in a > > > separate variable. > > Since this is CommonJS module, there is no reason to avoid additional global > variables. And having one special key that is threaded differently, and even > looked up separately, is inconsistent and makes the code unnecessarily hard to > understand. > > > > https://codereview.adblockplus.org/29326181/diff/29326185/lib/notificationHel... > > > lib/notificationHelper.js:261: return methods.indexOf(method) > -1; > > > If you use objects for these "lists", you can use the in-operator here. > > As before, the appropriate data structure to check whether an item is in a set > of items, would be a set or if not available an object. |x in obj| is more > efficient and makes the code more readable than |arr.indexOf(x) != -1|. However, > I see that this will introduce some boilerplate when defining the data > structure. So feel free to ignore that one. Thanks, I really appreciate the feedback. I'm usually opting for legibility over performance unless the code is within a performance-critical area so I was wondering whether that's what you were criticizing. Anyway, applied the changes now.
On 2015/09/10 13:56:06, Thomas Greiner wrote: > Thanks, I really appreciate the feedback. I'm usually opting for legibility over > performance unless the code is within a performance-critical area so I was > wondering whether that's what you were criticizing. Absolutely, performance was just a side effect. Readability and consistency was the main relational. LGTM
Message was sent while issue was closed.
https://codereview.adblockplus.org/29326181/diff/29327139/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29326181/diff/29327139/lib/notificationHel... lib/notificationHelper.js:256: exports.shouldDisplay = shouldDisplay; I just realized that this way shouldDisplay is documented as static member rather than a method of the module: https://adblockplus.org/jsdoc/adblockpluschrome/module-notificationHelper.html Also see: http://usejsdoc.org/howto-commonjs-modules.html Mind writing a follow-up patch to fix that? I'd go for: let shouldDisplay = /** * ... */ exports.shouldDisplay = function() { ... }; We already do it that way in other modules. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
