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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /**
  * @fileOverview Handles notifications.
  */
 
 Cu.import("resource://gre/modules/Services.jsm");
 
 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 INITIAL_DELAY = 12 * 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 showListeners = [];
@@ skipping 35 matching lines @@
  * @class
  */
 let Notification = exports.Notification =
 {
   /**
    * 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);
+    onShutdown.add(() => downloader.cancel());
 
     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.

+    notificationTimer.initWithCallback(Notification.showNext.bind(this),
                                        STARTUP_SHOW_DELAY,
                                        Ci.nsITimer.TYPE_ONE_SHOT);
     onShutdown.add(() => notificationTimer.cancel());
   },
 
   /**
    * Yields a Downloadable instances for the notifications download.
    */
   _getDownloadables: function*()
   {
@@ skipping 68 matching lines @@
     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 = [];
+    let index = showListeners.indexOf(listener);
+    if (index != -1)
+      showListeners.splice(index, 1);
   },
 
   /**
    * 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)
   {
     function checkTarget(target, parameter, name, version)
@@ skipping 168 matching lines @@
 
   /**
    * 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 questionListeners))
       return;
-    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.

+    let listeners = questionListeners[id];
+    for (let listener of listeners)
       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();