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

lib/notification.js

Index: lib/notification.js
diff --git a/lib/notification.js b/lib/notification.js
index 311e4e881389424155f1a61a3c351a3f69cfaf88..055c8e9187646fb651a94da43103d8a2c87da382 100644
--- a/lib/notification.js
+++ b/lib/notification.js
@@ -200,17 +200,6 @@ let Notification = exports.Notification =
    */
   _getNextToShow(url)
   {
-    function checkTarget(target, parameter, name, version)
-    {
-      let minVersionKey = parameter + "MinVersion";
-      let maxVersionKey = parameter + "MaxVersion";
-      return !((parameter in target && target[parameter] != name) ||
-               (minVersionKey in target &&
-                Services.vc.compare(version, target[minVersionKey]) < 0) ||
-               (maxVersionKey in target &&
-                Services.vc.compare(version, target[maxVersionKey]) > 0));
-    }
-
     let remoteData = [];
     if (typeof Prefs.notificationdata.data == "object" &&
         Prefs.notificationdata.data.notifications instanceof Array)
@@ -288,20 +277,50 @@ let Notification = exports.Notification =
 
       if (notification.targets instanceof Array)
       {
-        let match = false;
+        let checks = new Map([

[Wladimir Palant, 2017/08/22 07:54:58]

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?

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

Acknowledged.

+          ["extension", v => v == addonName],
+          ["extensionMinVersion",
+           v => Services.vc.compare(addonVersion, v) >= 0],
+          ["extensionMaxVersion",
+           v => Services.vc.compare(addonVersion, v) <= 0],
+          ["application", v => v == application],
+          ["applicationMinVersion",
+           v => Services.vc.compare(applicationVersion, v) >= 0],
+          ["applicationMaxVersion",
+           v => Services.vc.compare(applicationVersion, v) <= 0],
+          ["platform", v => v == platform],
+          ["platformMinVersion",
+           v => Services.vc.compare(platformVersion, v) >= 0],
+          ["platformMaxVersion",
+           v => Services.vc.compare(platformVersion, v) <= 0],
+          ["blockedTotalMax", v => Prefs.blocked_total <= parseInt(v, 10)],
+          ["blockedTotalMin", v => Prefs.blocked_total >= parseInt(v, 10)],

[Wladimir Palant, 2017/08/22 07:54:58]

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?

[wspee, 2017/08/23 10:12:19]

Acknowledged.

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

+          ["locales", v => v.indexOf(Utils.appLocale) != -1]
+        ]);
+
+        let match = true;

[Wladimir Palant, 2017/08/22 07:54:58]

I'd say that this should be initialized with false still, initially there is no
match.

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

Acknowledged.

         for (let target of notification.targets)
         {
-          if (checkTarget(target, "extension", addonName, addonVersion) &&
-              checkTarget(target, "application", application,
-                          applicationVersion) &&
-              checkTarget(target, "platform", platform, platformVersion))
+          match = true;
+          for (let key of Object.keys(target))

[Wladimir Palant, 2017/08/22 07:54:58]

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;
  }

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

Acknowledged.

+          {
+            let value = target[key];
+            let check = checks.get(key);
+
+            if (!check || !check(value))

[wspee, 2017/08/21 15:18:21]

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:58]

Agreed.

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

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

+            {
+              match = false;
+            }
+          }
+          if (match)
           {
-            match = true;
             break;
           }
         }
         if (!match)
+        {
           continue;
+        }
       }
 
       if (!notificationToShow ||