Implemented anti-adblock message notification

lib/notification.js

Index: lib/notification.js
===================================================================
--- a/lib/notification.js
+++ b/lib/notification.js
@@ -25,15 +25,21 @@
 let {Prefs} = require("prefs");
 let {Downloader, Downloadable, MILLIS_IN_MINUTE, MILLIS_IN_HOUR, MILLIS_IN_DAY} = require("downloader");
 let {Utils} = require("utils");
+let {Matcher} = require("matcher");
+let {Filter} = require("filterClasses");
 
 let INITIAL_DELAY = 12 * MILLIS_IN_MINUTE;
 let CHECK_INTERVAL = 1 * MILLIS_IN_HOUR;
 let EXPIRATION_INTERVAL = 1 * MILLIS_IN_DAY;
+let TYPES = {
+  information: 0,
+  question: 1,
+  critical: 2
+};
 
 function getNumericalSeverity(notification)
 {
-  let levels = {information: 0, critical: 1};
-  return (notification.severity in levels ? levels[notification.severity] : levels.information);
+  return (notification.type in TYPES ? TYPES[notification.type] : TYPES.information);

[Felix Dahlke, 2014/02/11 10:35:27]

I think we should be backwards compatible here, we won't change the name of this
field on the server right away, and if we do, we can't show notifications to all
users in the migration period.

[Thomas Greiner, 2014/02/11 16:53:31]

Done.

 }
 
 function saveNotificationData()
@@ -60,6 +66,7 @@
  * @type Downloader
  */
 let downloader = null;
+let localData = [];
 
 /**
  * Regularly fetches notifications and decides which to show.
@@ -142,10 +149,10 @@
 
   /**
    * Determines which notification is to be shown next.
-   * @param {Array of Object} notifications active notifications
-   * @return {Object} notification to be shown, or null if there is none
+   * @param {String} url URL to match notifications to
+   * @return {Array of Object} active notifications
    */
-  getNextToShow: function()
+  _getActiveNotifications: function(url)
   {
     function checkTarget(target, parameter, name, version)
     {
@@ -154,11 +161,11 @@
       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));
-
     }
 
-    if (typeof Prefs.notificationdata.data != "object" || !(Prefs.notificationdata.data.notifications instanceof Array))
-      return null;
+    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))
     {
@@ -167,14 +174,28 @@
     }
 
     let {addonName, addonVersion, application, applicationVersion, platform, platformVersion} = require("info");
-    let notifications = Prefs.notificationdata.data.notifications;
-    let notificationToShow = null;
+    let notifications = localData.concat(remoteData);
+    let activeNotifications = [];
     for each (let notification in notifications)
     {
-      if ((typeof notification.severity === "undefined" || notification.severity === "information")
+      if ((typeof notification.type === "undefined" || notification.type === "information" || notification.type === "question")

[Felix Dahlke, 2014/02/11 10:35:27]

I think this would make more sense now:

if ((typeof notification.type === "undefined" || notification.type !==
"warning")

[Thomas Greiner, 2014/02/11 16:53:31]

Done.

           && Prefs.notificationdata.shown.indexOf(notification.id) !== -1)
         continue;
 
+      if (typeof url === "string" || notification.domains instanceof Array)

[Felix Dahlke, 2014/02/11 10:35:27]

notification.domains are not necessarily domains if it's arbitrary filters, so I
think it needs another name. notification.urls? notification.urlFilters?

[Thomas Greiner, 2014/02/11 16:53:31]

Done.

+      {
+        if (typeof url === "string" && notification.domains instanceof Array)
+        {
+          let matcher = new Matcher();

[Thomas Greiner, 2014/02/07 17:29:17]

Let me know if you don't like this approach. However, if we want to create the
matcher beforehand we'd need to store it somewhere and I couldn't find a good
location for that:
- attaching it to the notification would be problematic due to it being stored
in serialized form)
- having the data separately means that we'd need to keep it synchronized with
the notification

[Felix Dahlke, 2014/02/11 10:35:27]

Pretty fine to me, wouldn't like to complicate this without any real performance
issues. And I don't expect any here, from what I see, there's not much overhead.

+          for each (let filter in notification.domains)
+            matcher.add(Filter.fromText(filter));
+          if (!matcher.matchesAny(url, "DOCUMENT", url))
+            continue;
+        }
+        else
+          continue;

[Felix Dahlke, 2014/02/11 10:35:27]

So if the url parameter is passed, we ignore all notifications that don't have
notification.domains? Seems wrong to me.

[Thomas Greiner, 2014/02/11 16:53:31]

This means that some notifications could be shown for any URL. Can you think of
a good use-case for that?

[Felix Dahlke, 2014/02/11 17:11:12]

Ah, I thought I'd change my mind on this after reviewing lib/ui.js. Indeed, this
function is called twice: Once with and once without URL. So yeah, fine.

+      }
+
       if (notification.targets instanceof Array)
       {
         let match = false;
@@ -192,20 +213,43 @@
           continue;
       }
 
-      if (!notificationToShow
-          || getNumericalSeverity(notification) > getNumericalSeverity(notificationToShow))
-        notificationToShow = notification;
+      activeNotifications.push(notification);

[Felix Dahlke, 2014/02/11 10:35:27]

Why did you change this logic? It looks to me like we're only ever interested in
the notification with the highest priority anyway.

[Thomas Greiner, 2014/02/11 16:53:31]

Done.

     }
 
+    return activeNotifications.sort(function(a, b)
+    {
+      return getNumericalSeverity(b) - getNumericalSeverity(a);
+    });
+  },
+
+  /**
+   * Determines which notification is to be shown next.
+   * @param {String} url URL to match notification to (optional)
+   * @return {Object} notification to be shown, or null if there is none
+   */
+  getNextToShow: function(url)
+  {
+    let [notificationToShow] = this._getActiveNotifications(url);
     if (notificationToShow && "id" in notificationToShow)
     {
-      Prefs.notificationdata.shown.push(notificationToShow.id);
-      saveNotificationData();
+      if (notificationToShow.type !== "question")
+        this.markAsShown(notificationToShow.id);
     }
+    else
+      notificationToShow = null;
 
     return notificationToShow;
   },
 
+  markAsShown: function(id)
+  {
+    if (Prefs.notificationdata.shown.indexOf(id) > -1)
+      return;
+
+    Prefs.notificationdata.shown.push(id);
+    saveNotificationData();
+  },
+
   /**
    * Localizes the texts of the supplied notification.
    * @param {Object} notification notification to translate
@@ -221,9 +265,34 @@
     for each (let key in textKeys)
     {
       if (key in notification)
-        localizedTexts[key] = localize(notification[key], locale);
+      {
+        if (typeof notification[key] == "string")
+          localizedTexts[key] = notification[key];
+        else
+          localizedTexts[key] = localize(notification[key], locale);
+      }
     }
     return localizedTexts;
+  },
+
+  /**
+   * Adds a local notification.
+   * @param {Object} notification notification to add
+   */
+  addNotification: function(notification)
+  {
+    if (localData.indexOf(notification) == -1)
+      localData.push(notification);
+  },
+
+  /**
+   * Removes an existing local notification.
+   * @param {Object} notification notification to remove
+   */
+  removeNotification: function(notification)
+  {
+    if (localData.indexOf(notification) > -1)

[Felix Dahlke, 2014/02/11 10:35:27]

Nit: Save the index to a temp instead of calling indexOf twice.

[Thomas Greiner, 2014/02/11 16:53:31]

Done.

+      localData.splice(localData.indexOf(notification), 1);
   }
 };
 Notification.init();