[adblockpluschrome] Issue 4764 - Added "relentless" notification that shows up in intervals

[adblockpluschrome] Issue 4764 - Added "relentless" notification that shows up in intervals

[Sebastian Noack, 2017-01-05 12:46:12]

As mentioned on the issue, I wonder how those notifications should be triggered.
By simply adding another notification type as you did, this type will merely
become available for notifications fetched from our servers. However, it seems
that we want to show this particular notification type for specific websites the
user visits, which would require more logic, if we really want to go there.

https://codereview.adblockplus.org/29370565/diff/29370566/lib/notificationHel...
File lib/notificationHelper.js (right):

https://codereview.adblockplus.org/29370565/diff/29370566/lib/notificationHel...
lib/notificationHelper.js:102: if (["critical",
"relentless"].indexOf(notificationType) == -1)
Using indexOf() for membership checks seems unnecessary here. The following
seems much easier to read to me (and is also faster if it would matter):

 if (notificationType != "critical" && notificationType != "relentless")
   ...

Also note that arrays also have an includes() method which would still be better
than using indexOf(). Or you could use a Set and it's has() method, which would
be most appropriate if you have a container that you only use for membership
checks. Though for simplicity, I'd prefer to use simple comparision logic here,
as indicated above.

https://codereview.adblockplus.org/29370565/diff/29370566/lib/notificationHel...
lib/notificationHelper.js:243: if (notification.type !== "question")
As per the Mozilla coding style guide
(https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...)
which is referenced in our coding style guide
(https://adblockplus.org/coding-style#javascript), we prefer != over !==.

[wspee, 2017-01-05 15:19:27]

The notification will be triggered via notification.json and use urlFilters to
only show up on certain websites (see my email also)

https://codereview.adblockplus.org/29370565/diff/29370566/lib/notificationHel...
File lib/notificationHelper.js (right):

https://codereview.adblockplus.org/29370565/diff/29370566/lib/notificationHel...
lib/notificationHelper.js:102: if (["critical",
"relentless"].indexOf(notificationType) == -1)
On 2017/01/05 12:46:11, Sebastian Noack wrote:
> Using indexOf() for membership checks seems unnecessary here. The following
> seems much easier to read to me (and is also faster if it would matter):
> 
>  if (notificationType != "critical" && notificationType != "relentless")
>    ...
> 
> Also note that arrays also have an includes() method which would still be
better
> than using indexOf(). Or you could use a Set and it's has() method, which
would
> be most appropriate if you have a container that you only use for membership
> checks. Though for simplicity, I'd prefer to use simple comparision logic
here,
> as indicated above.

Done.

https://codereview.adblockplus.org/29370565/diff/29370566/lib/notificationHel...
lib/notificationHelper.js:243: if (notification.type !== "question")
On 2017/01/05 12:46:11, Sebastian Noack wrote:
> As per the Mozilla coding style guide
>
(https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style...)
> which is referenced in our coding style guide
> (https://adblockplus.org/coding-style#javascript), we prefer != over !==.

Done.

[Sebastian Noack, 2017-01-11 18:19:29]

LGTM. But I guess, lets wait until the related core change landed, and then
update the dependency in the same go.

[wspee, 2017-01-20 10:52:14]

On 2017/01/11 18:19:29, Sebastian Noack wrote:
> LGTM. But I guess, lets wait until the related core change landed, and then
> update the dependency in the same go.

Changes have landed, I have updated the dependency.

[Sebastian Noack, 2017-02-21 09:21:26]

Still LGTM