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

lib/notification.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 182 matching lines @@
       showListeners.splice(index, 1);
   },
 
   /**
    * Determines which notification is to be shown next.
    * @param {string} url URL to match notifications to (optional)
    * @return {Object} notification to be shown, or null if there is none
    */
   _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)
     {
       remoteData = Prefs.notificationdata.data.notifications;
     }
 
     let notifications = localData.concat(remoteData);
     if (notifications.length === 0)
       return null;
@@ skipping 57 matching lines @@
           {
             continue;
           }
         }
         else
           continue;
       }
 
       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.

           {
-            match = true;
+            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)
+          {
             break;
           }
         }
         if (!match)
+        {
           continue;
+        }
       }
 
       if (!notificationToShow ||
           getNumericalSeverity(notification) >
             getNumericalSeverity(notificationToShow))
         notificationToShow = notification;
     }
 
     return notificationToShow;
   },
@@ skipping 151 matching lines @@
     else if (index != -1 && forceValue !== true)
       categories.splice(index, 1);
 
     // HACK: JSON values aren't saved unless they are assigned a
     // different object.
     Prefs.notifications_ignoredcategories =
       JSON.parse(JSON.stringify(categories));
   }
 };
 Notification.init();