Issue 1162 - Cache notification URL matcher

lib/notification.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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 44 matching lines @@
     return translations[locale];
 
   let languagePart = locale.substring(0, locale.indexOf("-"));
   if (languagePart && languagePart in translations)
     return translations[languagePart];
 
   let defaultLocale = "en-US";
   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.
  * @class
  */
 let Notification = exports.Notification =
 {
   /**
    * Called on module startup.
    */
   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()
     {
       downloader.cancel();
     });
 
     downloader.onExpirationChange = this._onExpirationChange.bind(this);
     downloader.onDownloadSuccess = this._onDownloadSuccess.bind(this);
     downloader.onDownloadError = this._onDownloadError.bind(this);
   },
 
   /**
    * Yields a Downloadable instances for the notifications download.
    */
   _getDownloadables: function()

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

Why are we exporting functions like this and prefixing them with an underscore,
instead of simply not exporting them?

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

I don't think it's meant to be exported because it's also not used anywhere. It
was probably just a way to group related functions. But since I haven't created
this I can't say for sure but would think that we could move some functions
which don't refer to `this`, like `_getDownloadables()`, outside of the
`Notification` prototype.

   {
     let downloadable = new Downloadable(Prefs.notificationurl);
     if (typeof Prefs.notificationdata.lastError === "number")
       downloadable.lastError = Prefs.notificationdata.lastError;
     if (typeof Prefs.notificationdata.lastCheck === "number")
       downloadable.lastCheck = Prefs.notificationdata.lastCheck;
     if (typeof Prefs.notificationdata.data === "object" && "version" in Prefs.notificationdata.data)
       downloadable.lastVersion = Prefs.notificationdata.data.version;
     if (typeof Prefs.notificationdata.softExpiration === "number")
       downloadable.softExpiration = Prefs.notificationdata.softExpiration;
@@ skipping 16 matching lines @@
     {
       let data = JSON.parse(responseText);
       for (let notification of data.notifications)
       {
         if ("severity" in notification)
         {
           if (!("type" in notification))
             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)
     {
       Cu.reportError(e);
       errorCallback("synchronize_invalid_data");
       return;
     }
 
     Prefs.notificationdata.lastError = 0;
     Prefs.notificationdata.downloadStatus = "synchronize_ok";
     [Prefs.notificationdata.softExpiration, Prefs.notificationdata.hardExpiration] = downloader.processExpirationInterval(EXPIRATION_INTERVAL);
     saveNotificationData();
   },
 
   _onDownloadError: function(downloadable, downloadURL, error, channelStatus, responseStatus, redirectCallback)
   {
     Prefs.notificationdata.lastError = Date.now();
     Prefs.notificationdata.downloadStatus = error;
     saveNotificationData();
   },
 
   /**
    * 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: function(url)

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

This function is confusing, each section needs a comment explaining the logic.
E.g. "If the notification is restricted to certain URLs we must check that the
current URL (if any) matches."

I also think it's too long and could do with being split apart.

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

Agreed but I'd suggest not touching any code that's unrelated to this issue and
instead tackling it separately. The reason for it being that we may break some
stuff which would then require backing out the entire change.

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

IMO I think we should tidy this code up while addressing the issue, but sure
split the changes up into separate commits (and codereviews).

   {
     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;
-
     if (!(Prefs.notificationdata.shown instanceof Array))
     {
       Prefs.notificationdata.shown = [];
       saveNotificationData();
     }
 
     let notifications = localData.concat(remoteData);
     if (notifications.length === 0)
       return null;
 
     let {addonName, addonVersion, application, applicationVersion, platform, platformVersion} = require("info");
     let notificationToShow = null;
     for (let notification of notifications)
     {
       if ((typeof notification.type === "undefined" || notification.type !== "critical")
           && 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
           continue;
       }
 
       if (notification.targets instanceof Array)
       {
         let match = false;
         for (let target of notification.targets)
@@ skipping 58 matching lines @@
     return localizedTexts;
   },
 
   /**
    * Adds a local notification.
    * @param {Object} notification notification to add
    */
   addNotification: function(notification)
   {
     if (localData.indexOf(notification) == -1)
+    {
+      initNotificationMatcher(notification);
       localData.push(notification);
+    }
   },
 
   /**
    * Removes an existing local notification.
    * @param {Object} notification notification to remove
    */
   removeNotification: function(notification)
   {
     let index = localData.indexOf(notification);
     if (index > -1)
@@ skipping 33 matching lines @@
   triggerQuestionListeners: function(id, approved)
   {
     if (!(id in listeners))
       return;
     let questionListeners = listeners[id];
     for (let listener of questionListeners)
       listener(approved);
   }
 };
 Notification.init();