Issue 1162 - Cache notification URL matcher

lib/notification.js

Index: lib/notification.js
===================================================================
--- a/lib/notification.js
+++ b/lib/notification.js
@@ -62,12 +62,25 @@
   return translations[defaultLocale];
 }
 
+function initNotificationMatcher(notification)
+{
+  if ("_matcher" in notification || !(notification.urlFilters instanceof Array))
+    return;
+
+  let matcher = new Matcher();
+  for (let urlFilter of notification.urlFilters)
+    matcher.add(Filter.fromText(urlFilter));
+  matcher.toJSON = () => {};

[sergei, 2018/01/03 11:12:22]

This is not exactly how it's said in the issue "Notifications should remain
serializable (replace JSON.serialize() to drop all properties starting with "_"
if necessary)" but for me the current variant is fine.

[Thomas Greiner, 2018/01/11 18:18:19]

Fortunately, we can now use `Symbol` instead so that the matcher property won't
be in the enumeration when running `JSON.stringify()` so I'd change it to use
that instead so that we can avoid this workaround altogether.

[kzar, 2018/01/12 12:27:17]

Acknowledged.

+  notification._matcher = matcher;
+}
+
 /**
  * The object providing actual downloading functionality.
  * @type Downloader
  */
 let downloader = null;
 let localData = [];
+let remoteData = [];

[sergei, 2018/01/03 11:12:22]

Could you please add a comment that localData and remoteData are arrays of
notification objects and their role is to cache temporary and persistent
notifications respectively.

[Thomas Greiner, 2018/01/11 18:18:19]

Yep, will do.

 
 /**
  * Regularly fetches notifications and decides which to show.
@@ -80,6 +93,14 @@
    */
   init: function()
   {
+    let notificationdata = Prefs.notificationdata.data;
+    if (notificationdata)
+    {
+      for (let notification of notificationdata.notifications)
+        initNotificationMatcher(notification);
+      remoteData = notificationdata.notifications;
+    }
+
     downloader = new Downloader(this._getDownloadables.bind(this), INITIAL_DELAY, CHECK_INTERVAL);
     onShutdown.add(function()
     {
@@ -131,7 +152,9 @@
             notification.type = notification.severity;
           delete notification.severity;
         }
+        initNotificationMatcher(notification);
       }
+      remoteData = data.notifications;

[sergei, 2018/01/03 11:12:22]

It's a bit theoretical but IMO it can be a technical debt.
The difference from the previous approach is that earlier in getNextToShow we
read Prefs.notificationdata.data, which can be theoretically set by several
ways, but now in getNextToShow we are reading remoteData which is set only from
_onDownloadSuccess. IMO, although currently it does not change anything, we
should rather listen to the changes of Prefs and when
Prefs.notificationdata.data is changed remoteData should be updated.

[kzar, 2018/01/04 17:04:09]

Yea, I don't think these changes are necessary now with my other suggestions.

[Thomas Greiner, 2018/01/11 18:18:19]

Good point. I don't remember why I made this change back when I created this
review. It could've been just a leftover from development. Anyway, I agree that
this variable is redundant.

       Prefs.notificationdata.data = data;

[sergei, 2018/01/03 11:12:22]

Despite it's not directly a part of this issue, should new notifications
actually be added to already existing ones instead of overriding? If there are
no new notifications then it will remove all previously stored ones.

[kzar, 2018/01/04 17:04:09]

Hmm, I'm not too familiar with this code but I think you might be right.

[Thomas Greiner, 2018/01/11 18:18:19]

By overriding existing notifications we can control which notifications should
be active on which day. That's probably important for calculating conversion
rates. But again, I haven't implemented the initial logic so I can't say for
sure.

     }
     catch (e)
@@ -170,10 +193,6 @@
                (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;
-
     if (!(Prefs.notificationdata.shown instanceof Array))
     {
       Prefs.notificationdata.shown = [];
@@ -192,14 +211,11 @@
           && Prefs.notificationdata.shown.indexOf(notification.id) !== -1)

[kzar, 2018/01/04 17:04:09]

We can use .includes(notification.id) these days (same elsewhere).

         continue;
 
-      if (typeof url === "string" || notification.urlFilters instanceof Array)

[kzar, 2018/01/04 17:04:09]

IMO this code should instead be replaced with a call to a function. Something
like urlFiltersMatchUrl(url, urlFilters). That function should worry about
caching the matcher and it can do so using a WeakMap instead of mutating the
notification itself. The function should return true if the url matches, false
if it doesn't and undefined if there's no URL / urlFilters to match.

[Thomas Greiner, 2018/01/11 18:18:19]

I agree that splitting that off into its own function is a good idea to make it
more readable and easier to maintain.

However, using `WeakMap` seems pretty equal to using `Symbol` (see
https://codereview.adblockplus.org/5330039625220096/diff/5629499534213120/lib...).
Therefore I'd be fine with either so just wanted to point that out.

[kzar, 2018/01/12 12:27:17]

Acknowledged.

+      if (typeof url === "string" || "_matcher" in notification)
       {
-        if (typeof url === "string" && notification.urlFilters instanceof Array)
+        if (typeof url === "string" && "_matcher" in notification)
         {
-          let matcher = new Matcher();
-          for (let urlFilter of notification.urlFilters)
-            matcher.add(Filter.fromText(urlFilter));
-          if (!matcher.matchesAny(url, "DOCUMENT", url))
+          if (!notification._matcher.matchesAny(url, "DOCUMENT", url))
             continue;
         }
         else
@@ -278,7 +294,10 @@
   addNotification: function(notification)
   {
     if (localData.indexOf(notification) == -1)
+    {
+      initNotificationMatcher(notification);
       localData.push(notification);
+    }
   },
 
   /**