Issue 5459 - Add support to show a notification based on the number of ads blocked

Issue 5459 - Add support to show a notification based on the number of ads blocked

[wspee, 2017-07-31 15:16:22]

https://codereview.adblockplus.org/29501607/diff/29501608/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29501607/diff/29501608/lib/notification.js...
lib/notification.js:308: (typeof Prefs.blocked_total != 'number' ||
NOTE: Prefs.blocked_total only exists on webext based extensions but
blocked_total_min/max is supposed to limit the amount of users that receive the
extension so it wouldn't make sense to show the notification is blocked_total
doesn't exists.

You could argue that this is platform specific but it seems to me that Prefs is
intended be platform independent and also firefox-xul will be deprecated
soon-ish so I opted for the simpler implementation.

Let me know what you think?

[Wladimir Palant, 2017-08-15 10:31:21]

https://codereview.adblockplus.org/29501607/diff/29501613/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29501607/diff/29501613/lib/notification.js...
lib/notification.js:310: continue
Nit: please run ESLint. There should be a semicolon at the end of this
statement. Also, we usually require brackets around the body even if it is the
condition that is spanning multiple lines - ESLint cannot flag that. Yes, I am
aware that this rule already isn't being followed for the block above already.

https://codereview.adblockplus.org/29501607/diff/29501613/lib/notification.js...
lib/notification.js:315: continue
We should use camelCase for consistency. Also, we should expect structured data,
something like:

  blockedTotal: {min: 0, max: 4000}

I just discussed this with Felix, and the conclusion is that this condition
belongs into the targets array, it shouldn't be there on its own.

[wspee, 2017-08-21 15:18:22]

https://codereview.adblockplus.org/29501607/diff/29501613/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29501607/diff/29501613/lib/notification.js...
lib/notification.js:310: continue
On 2017/08/15 10:31:21, Wladimir Palant wrote:
> Nit: please run ESLint. There should be a semicolon at the end of this
> statement. Also, we usually require brackets around the body even if it is the
> condition that is spanning multiple lines - ESLint cannot flag that. Yes, I am
> aware that this rule already isn't being followed for the block above already.

Of course, sorry forgot to use ESLint.

https://codereview.adblockplus.org/29501607/diff/29501613/lib/notification.js...
lib/notification.js:315: continue
On 2017/08/15 10:31:21, Wladimir Palant wrote:
> We should use camelCase for consistency. Also, we should expect structured
data,
> something like:
> 
>   blockedTotal: {min: 0, max: 4000}
> 
> I just discussed this with Felix, and the conclusion is that this condition
> belongs into the targets array, it shouldn't be there on its own.

I implemented the syntax from the issue (#5457), then it's also consistent with
extensionVersion, etc..

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js...
lib/notification.js:310: if (!check || !check(value))
This implementation no longer ignores unknown targets. Notifications with
unknown targets will not be shown. I believe this makes sense because it makes
adding targets saver. You don't have to remember specifying the version the
target was added in addition.

Let me know if you agree.

[Wladimir Palant, 2017-08-22 07:54:59]

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js...
lib/notification.js:280: let checks = new Map([
This shouldn't be set inside the loop, we'll get new closures for each loop
iteration unnecessarily. Declare this variable right after addonVersion and
others are declared?

Nit: The map syntax isn't very readable. Given that we aren't storing arbitrary
data here, a regular object literal should do as well?

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js...
lib/notification.js:297: ["blockedTotalMin", v => Prefs.blocked_total >=
parseInt(v, 10)],
Why parseInt() here? I'd say that the backend is supposed to give us numbers,
not strings. Note that strings will be converted to numbers implicitly anyway.

Nit²: You always handle Min before Max, why not here?

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js...
lib/notification.js:301: let match = true;
I'd say that this should be initialized with false still, initially there is no
match.

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js...
lib/notification.js:305: for (let key of Object.keys(target))
How about making this more compact and closer to the original logic:

  if (Object.keys(target).every(key => checks.hasOwnProperty(key) &&
                                       checks[key](target[key]))
  {
    match = true;
    break;
  }

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js...
lib/notification.js:310: if (!check || !check(value))
On 2017/08/21 15:18:21, wspee wrote:
> This implementation no longer ignores unknown targets. Notifications with
> unknown targets will not be shown. I believe this makes sense because it makes
> adding targets saver. You don't have to remember specifying the version the
> target was added in addition.

Agreed.

https://codereview.adblockplus.org/29501607/diff/29522625/test/notification.js
File test/notification.js (right):

https://codereview.adblockplus.org/29501607/diff/29522625/test/notification.j...
test/notification.js:40: sandboxedRequire("./stub-modules/utils");
This change seems unnecessary?

[wspee, 2017-08-23 10:12:20]

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js...
lib/notification.js:280: let checks = new Map([
On 2017/08/22 07:54:58, Wladimir Palant wrote:
> This shouldn't be set inside the loop, we'll get new closures for each loop
> iteration unnecessarily. Declare this variable right after addonVersion and
> others are declared?
> 
> Nit: The map syntax isn't very readable. Given that we aren't storing
arbitrary
> data here, a regular object literal should do as well?

Acknowledged.

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js...
lib/notification.js:297: ["blockedTotalMin", v => Prefs.blocked_total >=
parseInt(v, 10)],
On 2017/08/22 07:54:58, Wladimir Palant wrote:
> Why parseInt() here? I'd say that the backend is supposed to give us numbers,
> not strings. Note that strings will be converted to numbers implicitly anyway.

Acknowledged.

Thanks for pointing that out, the backend implementation was supplying strings
so far, not a good idea, changed.


> Nit²: You always handle Min before Max, why not here?

Acknowledged.

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js...
lib/notification.js:301: let match = true;
On 2017/08/22 07:54:58, Wladimir Palant wrote:
> I'd say that this should be initialized with false still, initially there is
no
> match.

Acknowledged.

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js...
lib/notification.js:305: for (let key of Object.keys(target))
On 2017/08/22 07:54:58, Wladimir Palant wrote:
> How about making this more compact and closer to the original logic:
> 
>   if (Object.keys(target).every(key => checks.hasOwnProperty(key) &&
>                                        checks[key](target[key]))
>   {
>     match = true;
>     break;
>   }

Acknowledged.

https://codereview.adblockplus.org/29501607/diff/29522625/lib/notification.js...
lib/notification.js:310: if (!check || !check(value))
On 2017/08/22 07:54:58, Wladimir Palant wrote:
> On 2017/08/21 15:18:21, wspee wrote:
> > This implementation no longer ignores unknown targets. Notifications with
> > unknown targets will not be shown. I believe this makes sense because it
makes
> > adding targets saver. You don't have to remember specifying the version the
> > target was added in addition.
> 
> Agreed.

See https://issues.adblockplus.org/ticket/5558#ticket.

https://codereview.adblockplus.org/29501607/diff/29522625/test/notification.js
File test/notification.js (right):

https://codereview.adblockplus.org/29501607/diff/29522625/test/notification.j...
test/notification.js:40: sandboxedRequire("./stub-modules/utils");
On 2017/08/22 07:54:58, Wladimir Palant wrote:
> This change seems unnecessary?

Acknowledged.

[Wladimir Palant, 2017-08-23 11:00:11]

https://codereview.adblockplus.org/29501607/diff/29524557/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29501607/diff/29524557/lib/notification.js...
lib/notification.js:220: v => Services.vc.compare(addonVersion, v) >= 0,
Here and below: we shouldn't have one-space indentations, these aren't obvious
enough. It should always be at least two spaces.

https://codereview.adblockplus.org/29501607/diff/29524557/lib/notification.js...
lib/notification.js:235: locales: v => v.indexOf(Utils.appLocale) != -1
Sorry, overlooked a detail in the previous review. This should be
v.includes(Utils.appLocale)

https://codereview.adblockplus.org/29501607/diff/29524557/lib/notification.js...
lib/notification.js:306: match = false;
This assignment is no longer necessary.

[wspee, 2017-08-24 16:39:23]

Also updated patch to no longer show notifications for blockedTotal* if
Prefs.show_statsinpopup is false, see:
https://issues.adblockplus.org/ticket/5459#comment:10

https://codereview.adblockplus.org/29501607/diff/29524557/lib/notification.js
File lib/notification.js (right):

https://codereview.adblockplus.org/29501607/diff/29524557/lib/notification.js...
lib/notification.js:220: v => Services.vc.compare(addonVersion, v) >= 0,
On 2017/08/23 11:00:10, Wladimir Palant wrote:
> Here and below: we shouldn't have one-space indentations, these aren't obvious
> enough. It should always be at least two spaces.

Acknowledged.

https://codereview.adblockplus.org/29501607/diff/29524557/lib/notification.js...
lib/notification.js:235: locales: v => v.indexOf(Utils.appLocale) != -1
On 2017/08/23 11:00:10, Wladimir Palant wrote:
> Sorry, overlooked a detail in the previous review. This should be
> v.includes(Utils.appLocale)

Acknowledged.

https://codereview.adblockplus.org/29501607/diff/29524557/lib/notification.js...
lib/notification.js:306: match = false;
On 2017/08/23 11:00:11, Wladimir Palant wrote:
> This assignment is no longer necessary.

Acknowledged.

[Wladimir Palant, 2017-08-25 08:21:30]

LGTM