Implemented anti-adblock message notification

lib/notification.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2013 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 13 matching lines @@
 let {TimeLine} = require("timeline");
 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 = {
+let TYPE = {
   information: 0,
   question: 1,
   critical: 2
 };
 
+let listeners = {};
+
 function getNumericalSeverity(notification)
 {
-  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.

+  return (notification.type in TYPE ? TYPE[notification.type] : TYPE.information);
 }
 
 function saveNotificationData()
 {
   // HACK: JSON values aren't saved unless they are assigned a different object.
   Prefs.notificationdata = JSON.parse(JSON.stringify(Prefs.notificationdata));
 }
 
 function localize(translations, locale)
 {
@@ skipping 65 matching lines @@
     Prefs.notificationdata.lastCheck = downloadable.lastCheck;
     Prefs.notificationdata.softExpiration = downloadable.softExpiration;
     Prefs.notificationdata.hardExpiration = downloadable.hardExpiration;
     saveNotificationData();
   },
 
   _onDownloadSuccess: function(downloadable, responseText, errorCallback, redirectCallback)
   {
     try
     {
-      Prefs.notificationdata.data = JSON.parse(responseText);
+      let data = JSON.parse(responseText);
+      for each (let notification in data.notifications)
+      {
+        if ("severity" in notification)
+        {
+          if (!("type" in notification))
+            notification.type = notification.severity;
+          delete notification.severity;
+        }
+      }
+      Prefs.notificationdata.data = data;
     }
     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
-   * @return {Array of Object} active notifications
-   */
-  _getActiveNotifications: function(url)
+   * @param {String} url URL to match notifications to (optional)
+   * @return {Object} notification to be shown, or null if there is none
+   */
+  getNextToShow: function(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;
 
     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 notifications = localData.concat(remoteData);
-    let activeNotifications = [];
+    let notificationToShow = null;
     for each (let notification in notifications)
     {
-      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.

+      if ((typeof notification.type === "undefined" || notification.type !== "critical")
           && 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.urlFilters instanceof Array)
       {
-        if (typeof url === "string" && notification.domains instanceof Array)
+        if (typeof url === "string" && notification.urlFilters 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));
+          for each (let urlFilter in notification.urlFilters)
+            matcher.add(Filter.fromText(urlFilter));
           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;
         for each (let target in notification.targets)
         {
           if (checkTarget(target, "extension", addonName, addonVersion) &&
               checkTarget(target, "application", application, applicationVersion) &&
               checkTarget(target, "platform", platform, platformVersion))
           {
             match = true;
             break;
           }
         }
         if (!match)
           continue;
       }
 
-      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
+          || getNumericalSeverity(notification) > getNumericalSeverity(notificationToShow))
+        notificationToShow = notification;
+    }
+
     if (notificationToShow && "id" in notificationToShow)
     {
       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);
@@ skipping 34 matching lines @@
     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);
+    let index = localData.indexOf(notification);
+    if (index > -1)
+      localData.splice(index, 1);
+  },
+
+  /**
+   * Adds a listener for question-type notifications
+   */
+  addQuestionListener: function(/**string*/ id, /**function(approved)*/ listener)
+  {
+    if (!(id in listeners))
+      listeners[id] = [];
+    if (listeners[id].indexOf(listener) === -1)
+      listeners[id].push(listener);
+  },
+
+  /**
+   * Removes a listener that was previously added via addQuestionListener
+   */
+  removeQuestionListener: function(/**string*/ id, /**function(approved)*/ listener)
+  {
+    if (!(id in listeners))
+      return;
+    let index = listeners[id].indexOf(listener);
+    if (index > -1)
+      listeners[id].splice(index, 1);
+    if (listeners[id].length === 0)
+      delete listeners[id];
+  },
+
+  /**
+   * Notifies listeners about interactions with a notification
+   * @param {String} id notification ID
+   * @param {Boolean} approved indicator whether notification has been approved or not
+   */
+  triggerQuestionListeners: function(id, approved)
+  {
+    if (!(id in listeners))
+      return;
+    for each (let listener in listeners[id])
+      listener(approved);
   }
 };
 Notification.init();