Issue 2420 - Move notification show logic to core

lib/notification.js

Index: lib/notification.js
===================================================================
--- a/lib/notification.js
+++ b/lib/notification.js
@@ -30,13 +30,15 @@
 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)
 {
@@ -89,6 +91,12 @@
     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());
   },
 
   /**
@@ -158,11 +166,39 @@
   },
 
   /**
+   * 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)
     {
@@ -234,15 +270,26 @@
         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)
@@ -303,10 +350,10 @@
    */
   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);
   },
 
   /**
@@ -314,26 +361,25 @@
    */
   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);
   },