Add Notification module

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
  * 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 {TimeLine} = require("timeline");
 let {Prefs} = require("prefs");
+let {Downloader, Downloadable, MILLIS_IN_MINUTE, MILLIS_IN_HOUR, MILLIS_IN_DAY} = require("downloader");
 
-function compareSeverity(notification1, notification2)
+let INITIAL_DELAY = 12 * MILLIS_IN_MINUTE;

[Felix Dahlke, 2013/07/19 14:01:45]

How about using const for these?

[Wladimir Palant, 2013/07/19 14:55:11]

That will not have the results you expect. This code isn't running in a real
context, it's rather being loaded into an object by bootstrap.js. Using const
here will define a read-only property on that object - and an attempt to
overwrite it will fail silently rather than fail. So I decided to avoid const at
top level. This might work properly if we add "use strict" (which wasn't
available back when I implemented this module framework), we should check back
once we start using it.

+let CHECK_INTERVAL = 1 * MILLIS_IN_HOUR;
+let EXPIRATION_INTERVAL = 1 * MILLIS_IN_DAY;
+
+function getNumericalSeverity(notification)
 {
   let levels = {information: 0, critical: 1};
-  return levels[notification1.severity] - levels[notification2.severity];
+  return (notification.severity in levels ? levels[notification.severity] : levels.information);

[Felix Dahlke, 2013/07/19 14:01:45]

If we return -1 for unknown severities, those messages will never get precedence
over any other notifications. Maybe that's better?

[Wladimir Palant, 2013/07/19 14:55:11]

I would rather have "information" be the default, makes more sense IMHO,
particularly if we decide to add new severities in future that won't be
supported by old clients.

 }
+
+function saveNotificationData()
+{
+  // HACK: JSON values aren't saved unless they are assigned a different object.
+  Prefs.notificationdata = JSON.parse(JSON.stringify(Prefs.notificationdata));
+}
+
+/**
+ * The object providing actual downloading functionality.
+ * @type Downloader
+ */
+let downloader = null;
 
 /**
  * Regularly fetches notifications and decides which to show.
  * @class
  */
 let Notification = exports.Notification =
 {
   /**
+   * Called on module startup.
+   */
+  init: function()
+  {
+    TimeLine.enter("Entered Notification.init()");
+
+    downloader = new Downloader(this.getDownloadables.bind(this), INITIAL_DELAY, CHECK_INTERVAL);
+    onShutdown.add(function()
+    {
+      downloader.cancel();
+    });
+
+    downloader.onExpirationChange = this._onExpirationChange.bind(this);
+    downloader.onDownloadSuccess = this._onDownloadSuccess.bind(this);
+    downloader.onDownloadError = this._onDownloadError.bind(this);
+
+    TimeLine.leave("Notification.init() done");
+  },
+
+  /**
+   * Yields a Downloadable instances for the notifications download.
+   */
+  getDownloadables: function()

[Felix Dahlke, 2013/07/19 14:01:45]

Discussed a bit on IRC before, but it seems like getDownloadables is only used
internally. Will we use it externally later to react to new messages right away?

[Wladimir Palant, 2013/07/19 14:55:11]

Pretty unlikely, I guess that _getDownloadables would be better here.

+  {
+    let downloadable = new Downloadable(Prefs.notificationurl);
+    if (typeof Prefs.notificationdata.lastError == "number")

[Felix Dahlke, 2013/07/19 14:01:45]

I've noticed that Prefs.notificationdata is undefined in Chrome. I guess that's
a bug in the Prefs implementation we have in Chrome?

[Felix Dahlke, 2013/07/19 14:38:38]

Oh well, just remembered that the initial pref values need to be hard coded in
Chrome.

[Wladimir Palant, 2013/07/19 14:55:11]

Right, we need to add a default for it.

+      downloadable.lastError = Prefs.notificationdata.lastError;
+    if (typeof Prefs.notificationdata.lastCheck == "number")
+      downloadable.lastCheck = Prefs.notificationdata.lastCheck;
+    if (typeof Prefs.notificationdata.data == "object" && typeof Prefs.notificationdata.data.version == "number")
+      downloadable.lastVersion = Prefs.notificationdata.data.version;
+    if (typeof Prefs.notificationdata.softExpiration == "number")
+      downloadable.softExpiration = Prefs.notificationdata.softExpiration;
+    if (typeof Prefs.notificationdata.hardExpiration == "number")
+      downloadable.hardExpiration = Prefs.notificationdata.hardExpiration;
+    yield downloadable;
+  },
+
+  _onExpirationChange: function(downloadable)
+  {
+    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);
+    }
+    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 {Array of Object} notifications active notifications
    * @return {Object} notification to be shown, or null if there is none
    */
-  getNextToShow: function(notifications)
+  getNextToShow: function()
   {
-    if (!Prefs.shownNotifications)
-      Prefs.shownNotifications = [];
+    if (typeof Prefs.notificationdata.data != "object" || !(Prefs.notificationdata.data.notifications instanceof Array))
+      return null;
 
-    let notificationToShow;
+    if (!(Prefs.notificationdata.shown instanceof Array))
+    {
+      Prefs.notificationdata.shown = [];
+      saveNotificationData();
+    }
+
+    let {application, addonVersion} = require("info");
+    let notifications = Prefs.notificationdata.data.notifications;
+    let notificationToShow = null;
     for each (let notification in notifications)
     {
-      if (notification.severity === "information"
-          && Prefs.shownNotifications.indexOf(notification.timestamp) !== -1)
+      if ((typeof notification.severity == "undefined" || notification.severity === "information")

[Felix Dahlke, 2013/07/19 14:01:45]

I think using both == and === in the same line is a bit confusing, we should
settle on one. Don't really mind which one.

[Wladimir Palant, 2013/07/19 14:55:11]

True :)

+          && Prefs.notificationdata.shown.indexOf(notification.timestamp) !== -1)
         continue;
 
-      let info = require("info");
-      let platform = info.application;
-      let version = info.addonVersion;
-
-      if ("platforms" in notification
-          && notification.platforms.indexOf("chrome") === -1)
+      if (notification.platforms instanceof Array
+          && notification.platforms.indexOf(application) === -1)
         continue;
 
       if ("minVersion" in notification
-          && Services.vc.compare(version, notification.minVersion) < 0)
+          && Services.vc.compare(addonVersion, notification.minVersion) < 0)
         continue;
 
       if ("maxVersion" in notification
-          && Services.vc.compare(version, notification.maxVersion) > 0)
+          && Services.vc.compare(addonVersion, notification.maxVersion) > 0)
         continue;
 
       if (!notificationToShow
-          || compareSeverity(notification, notificationToShow) > 0)
+          || getNumericalSeverity(notification) > getNumericalSeverity(notificationToShow))
         notificationToShow = notification;
     }
 
     if (notificationToShow && "timestamp" in notificationToShow)
-      Prefs.shownNotifications.push(notificationToShow.timestamp);
+    {
+      Prefs.notificationdata.shown.push(notificationToShow.timestamp);

[Wladimir Palant, 2013/07/19 08:16:32]

It's probably better to call that field "id" rather than "timestamp" - it can
still be a timestamp then but it doesn't have to be.

[Felix Dahlke, 2013/07/19 14:01:45]

Yeah, fine by me. You already address this, so... :)

+      saveNotificationData();
+    }
 
     return notificationToShow;
   }
 };
+Notification.init();