Issue 2420 - Move notification show logic to core

lib/notification.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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 12 matching lines @@
 
 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 = 1 * MILLIS_IN_MINUTE;
 let CHECK_INTERVAL = 1 * MILLIS_IN_HOUR;
 let EXPIRATION_INTERVAL = 1 * MILLIS_IN_DAY;
+let STARTUP_SHOW_DELAY = 3 * MILLIS_IN_MINUTE;
 let TYPE = {
   information: 0,
   question: 1,
   critical: 2
 };
 
-let listeners = {};
+let showListeners = [];
+let questionListeners = {};
 
 function getNumericalSeverity(notification)
 {
   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));
@@ skipping 27 matching lines @@
 {
   /**
    * Called on module startup.
    */
   init: function()
   {
     downloader = new Downloader(this._getDownloadables.bind(this), INITIAL_DELAY, CHECK_INTERVAL);
     onShutdown.add(function()
     {
       downloader.cancel();
     });

[Wladimir Palant, 2015/06/08 11:33:15]

Mind changing this into arrow function as well for consistency?

[Felix Dahlke, 2015/06/08 19:34:34]

Done.

 
     downloader.onExpirationChange = this._onExpirationChange.bind(this);
     downloader.onDownloadSuccess = this._onDownloadSuccess.bind(this);
     downloader.onDownloadError = this._onDownloadError.bind(this);
+
+    notificationTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+    notificationTimer.initWithCallback(Notification.showNext,

[Wladimir Palant, 2015/06/08 11:33:15]

Notification.showNext.bind(this)? The next person changing something here might
spend lots of time wondering why their code doesn't work.

[Felix Dahlke, 2015/06/08 19:34:34]

Done.

+                                       STARTUP_SHOW_DELAY,
+                                       Ci.nsITimer.TYPE_ONE_SHOT);
+    onShutdown.add(() => notificationTimer.cancel());
   },
 
   /**
    * Yields a Downloadable instances for the notifications download.
    */
   _getDownloadables: function*()
   {
     let downloadable = new Downloadable(Prefs.notificationurl);
     if (typeof Prefs.notificationdata.lastError === "number")
       downloadable.lastError = Prefs.notificationdata.lastError;
@@ skipping 49 matching lines @@
   },
 
   _onDownloadError: function(downloadable, downloadURL, error, channelStatus, responseStatus, redirectCallback)
   {
     Prefs.notificationdata.lastError = Date.now();
     Prefs.notificationdata.downloadStatus = error;
     saveNotificationData();
   },
 
   /**
+   * Adds a listener for notifications to be shown.
+   * @param {Function} listener Listener to be invoked when a notification is
+   *                   to be shown
+   */
+  addShowListener: function(listener)
+  {
+    if (showListeners.indexOf(listener) == -1)
+      showListeners.push(listener);
+  },
+
+  /**
+   * Removes the supplied listener.
+   * @param {Function} listener Listener that was added via addShowListener()
+   */
+  removeShowListener: function(listener)
+  {
+    showListeners.splice(showListeners.indexOf(listener));

[Wladimir Palant, 2015/06/08 11:33:15]

For reference:

[1,2,3].splice(1) => [1]
[1,2,3].splice(-1) => [1,2]

[Felix Dahlke, 2015/06/08 19:34:34]

Done.

+  },
+
+  /**
+   * Removes all listeners added via addShowListener().
+   */
+  removeAllShowListeners: function()

[Wladimir Palant, 2015/06/08 11:33:15]

Why do we need that function? Seems to be a footgun. Normally, code should only
remove listeners it added itself.

[Felix Dahlke, 2015/06/08 19:34:34]

Done.

[Wladimir Palant, 2015/06/08 20:45:15]

Why am I still seeing it? :)

[Felix Dahlke, 2015/06/08 22:01:53]

Good question, done for real now.

+  {
+    showListeners = [];
+  },
+
+  /**
    * 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)
+  _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));
     }
 
@@ skipping 51 matching lines @@
         }
         if (!match)
           continue;
       }
 
       if (!notificationToShow
           || getNumericalSeverity(notification) > getNumericalSeverity(notificationToShow))
         notificationToShow = notification;
     }
 
-    if (notificationToShow && "id" in notificationToShow)
-    {
-      if (notificationToShow.type !== "question")
-        this.markAsShown(notificationToShow.id);
-    }
-
     return notificationToShow;
   },
 
+  /**
+   * Invokes the listeners added via addShowListener() with the next
+   * notification to be shown.
+   * @param {String} url URL to match notifications to (optional)
+   */
+  showNext: function(url)
+  {
+    let notification = Notification._getNextToShow(url);
+    if (notification)
+      for (let showListener of showListeners)
+        showListener(notification);
+  },
+
+  /**
+   * Marks a notification as shown.
+   * @param {String} id ID of the notification to be marked as shown
+   */
   markAsShown: function(id)
   {
     if (Prefs.notificationdata.shown.indexOf(id) > -1)
       return;
 
     Prefs.notificationdata.shown.push(id);
     saveNotificationData();
   },
 
   /**
@@ skipping 40 matching lines @@
     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);
+    if (!(id in questionListeners))
+      questionListeners[id] = [];
+    if (questionListeners[id].indexOf(listener) === -1)
+      questionListeners[id].push(listener);
   },
 
   /**
    * Removes a listener that was previously added via addQuestionListener
    */
   removeQuestionListener: function(/**string*/ id, /**function(approved)*/ listener)
   {
-    if (!(id in listeners))
+    if (!(id in questionListeners))
       return;
-    let index = listeners[id].indexOf(listener);
+    let index = questionListeners[id].indexOf(listener);
     if (index > -1)
-      listeners[id].splice(index, 1);
-    if (listeners[id].length === 0)
-      delete listeners[id];
+      questionListeners[id].splice(index, 1);
+    if (questionListeners[id].length === 0)
+      delete questionListeners[id];
   },
 
   /**
-   * Notifies listeners about interactions with a notification
+   * Notifies question 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))
+    if (!(id in questionListeners))
       return;
-    let questionListeners = listeners[id];
-    for (let listener of questionListeners)
+    for (let listener of questionListeners[id])

[Wladimir Palant, 2015/06/08 11:33:15]

Please keep using a temporary variable here - otherwise questionListeners[id]
will be evaluated for each loop iteration in all browsers but Firefox.

[Felix Dahlke, 2015/06/08 19:34:34]

Done.

[Wladimir Palant, 2015/06/08 20:45:15]

I would have called that variable "listeners" or something equally short.
Doesn't matter much however...

[Felix Dahlke, 2015/06/08 22:01:53]

True, wasn't a brilliant name. Done, since I was uploading another patch set
anyway.

       listener(approved);
   },
   
   /**
    * Toggles whether notifications of a specific category should be ignored
    * @param {String} category notification category identifier
    * @param {Boolean} [forceValue] force specified value
    */
   toggleIgnoreCategory: function(category, forceValue)
   {
     let categories = Prefs.notifications_ignoredcategories;
     let index = categories.indexOf(category);
     if (index == -1 && forceValue !== false)
     {
       categories.push(category);
       Prefs.notifications_showui = true;
     }
     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();