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 195 matching lines @@
     {
       remoteData = Prefs.notificationdata.data.notifications;
     }
 
     let notifications = localData.concat(remoteData);
     if (notifications.length === 0)
       return null;
 
     const {addonName, addonVersion, application,
            applicationVersion, platform, platformVersion} = require("info");
+
+    let targetChecks = {
+      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,
+      blockedTotalMin: v => Prefs.show_statsinpopup &&
+        Prefs.blocked_total >= v,
+      blockedTotalMax: v => Prefs.show_statsinpopup &&
+        Prefs.blocked_total <= v,
+      locales: v => v.includes(Utils.appLocale)
+    };
+
     let notificationToShow = null;
     for (let notification of notifications)
     {
       if (typeof notification.type === "undefined" ||
           notification.type !== "critical")
       {
         let shown;
         if (typeof Prefs.notificationdata.shown == "object")
           shown = Prefs.notificationdata.shown[notification.id];
 
@@ skipping 44 matching lines @@
           {
             continue;
           }
         }
         else
           continue;
       }
 
       if (notification.targets instanceof Array)
       {
-        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.

+        let match = false;
+
         for (let target of notification.targets)
         {
-          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.

+          if (Object.keys(target).every(key =>
+              targetChecks.hasOwnProperty(key) &&
+              targetChecks[key](target[key])))
           {
-            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 ||
@@ skipping 158 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();