Issue 1162 - Cache notification URL matcher

lib/notification.js

 /*
- * This file is part of Adblock Plus <http://adblockplus.org/>,
- * Copyright (C) 2006-2014 Eyeo GmbH
+ * This file is part of Adblock Plus <https://adblockplus.org/>,
+ * Copyright (C) 2006-present 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/>.
  */
 
+"use strict";
+
 /**
  * @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 = 12 * MILLIS_IN_MINUTE;
-let CHECK_INTERVAL = 1 * MILLIS_IN_HOUR;
-let EXPIRATION_INTERVAL = 1 * MILLIS_IN_DAY;
-let TYPE = {
+const {Prefs} = require("prefs");
+const {Downloader, Downloadable,
+       MILLIS_IN_MINUTE, MILLIS_IN_HOUR,
+       MILLIS_IN_DAY} = require("./downloader");
+const {Utils} = require("utils");
+const {Matcher, defaultMatcher} = require("./matcher");
+const {Filter, RegExpFilter, WhitelistFilter} = require("./filterClasses");
+
+const INITIAL_DELAY = 1 * MILLIS_IN_MINUTE;
+const CHECK_INTERVAL = 1 * MILLIS_IN_HOUR;
+const EXPIRATION_INTERVAL = 1 * MILLIS_IN_DAY;
+const TYPE = {
   information: 0,
   question: 1,
-  critical: 2
+  relentless: 2,
+  critical: 3
 };
-
-let listeners = {};
+const MATCHER = Symbol("Notification matcher");
+
+let showListeners = [];
+let questionListeners = {};
 
 function getNumericalSeverity(notification)
 {
-  return (notification.type in TYPE ? TYPE[notification.type] : TYPE.information);
+  if (notification.type in TYPE)
+    return TYPE[notification.type];
+  return 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)
 {
   if (locale in translations)
     return translations[locale];
 
   let languagePart = locale.substring(0, locale.indexOf("-"));
   if (languagePart && languagePart in translations)
     return translations[languagePart];
 
   let defaultLocale = "en-US";
   return translations[defaultLocale];
 }
 
+function parseVersionComponent(comp)
+{
+  if (comp == "*")
+    return Infinity;
+  return parseInt(comp, 10) || 0;
+}
+
+function compareVersion(v1, v2)
+{
+  let regexp = /^(.*?)([a-z].*)?$/i;
+  let [, head1, tail1] = regexp.exec(v1);
+  let [, head2, tail2] = regexp.exec(v2);
+  let components1 = head1.split(".");
+  let components2 = head2.split(".");
+
+  for (let i = 0; i < components1.length ||
+                  i < components2.length; i++)
+  {
+    let result = parseVersionComponent(components1[i]) -
+                 parseVersionComponent(components2[i]) || 0;
+
+    if (result != 0)
+      return result;
+  }
+
+  // Compare version suffix (e.g. 0.1alpha < 0.1b1 < 01.b2 < 0.1).
+  // However, note that this is a simple string comparision, meaning: b10 < b2
+  if (tail1 == tail2)
+    return 0;
+  if (!tail1 || tail2 && tail1 > tail2)
+    return 1;
+  return -1;
+}
+
+/**
+ * Initializes notification's matcher based on notification's URL filters
+ * @param {Object} notification
+ */
 function initNotificationMatcher(notification)
 {
-  if ("_matcher" in notification || !(notification.urlFilters instanceof Array))
+  if (MATCHER in notification || !(notification.urlFilters instanceof Array))
     return;
 
   let matcher = new Matcher();
   for (let urlFilter of notification.urlFilters)
+  {
     matcher.add(Filter.fromText(urlFilter));
-  matcher.toJSON = () => {};

[sergei, 2018/01/03 11:12:22]

This is not exactly how it's said in the issue "Notifications should remain
serializable (replace JSON.serialize() to drop all properties starting with "_"
if necessary)" but for me the current variant is fine.

[Thomas Greiner, 2018/01/11 18:18:19]

Fortunately, we can now use `Symbol` instead so that the matcher property won't
be in the enumeration when running `JSON.stringify()` so I'd change it to use
that instead so that we can avoid this workaround altogether.

[kzar, 2018/01/12 12:27:17]

Acknowledged.

-  notification._matcher = matcher;
+  }
+  notification[MATCHER] = matcher;
+}
+
+/**
+ * Matches URL against notification's URL filters
+ * @param {Object} notification
+ * @param {string} [url]
+ * @return {boolean} whether notification and URL match
+ */
+function matchesUrl(notification, url)
+{
+  // No matching necessary if there's nothing to match
+  if (typeof url !== "string" && !(MATCHER in notification))
+    return true;
+
+  // Notification shouldn't match if extension is disabled
+  if (!Prefs.enabled)
+    return false;
+
+  // Notification shouldn't match if matching cannot be done
+  if (typeof url !== "string" || !(MATCHER in notification))
+    return false;

[sergei, 2018/01/30 17:25:05]

It somehow conflicts with above
if (typeof url !== "string" && !(MATCHER in notification))
  return true;

Should we actually return `true` when the type of `url` parameter is actually
unexpected?

[Thomas Greiner, 2018/01/30 19:17:53]

The logic behind it is that if the notification doesn't target URLs then we
should just continue with checking whether it should be shown or not.
However, if one of them is missing it means that
- either the notification doesn't target URLs but we're only checking for
notifications which target URLs
- or that the notification targets URLs but that there's no URL to match against

In both situations there's a mismatch which means that the notification
shouldn't be shown.

---

If you think it'd be helpful, I can split this condition up into two separate
ones to add an individual comment to each of them.

Alternatively, I could introduce variables to clarify what each part stands for
and thereby also reduce duplication.

e.g.
let shouldCheck = (typeof url !== "string");
let targetsUrls = (MATCHER in notification);

if (!shouldCheck && !targetsUrls)
  return true;

if (!shouldCheck || !targetsUrls)
  return false;

[sergei, 2018/01/30 21:06:48]

Acknowledged.

+
+  let host;
+  try
+  {
+    host = new URL(url).hostname;
+  }
+  catch (e)
+  {
+    host = "";
+  }
+
+  // Notification shouldn't match if extension is disabled on provided domain
+  let exception = defaultMatcher.matchesAny(
+    url, RegExpFilter.typeMap.DOCUMENT, host, false, null
+  );
+  if (exception instanceof WhitelistFilter)
+    return false;
+
+  // Notification should match if one of its filters matches
+  let filter = notification[MATCHER].matchesAny(
+    url, RegExpFilter.typeMap.DOCUMENT, host, false, null
+  );
+  return !!filter;
 }
 
 /**
  * The object providing actual downloading functionality.
- * @type Downloader
+ * @type {Downloader}
  */
 let downloader = null;
+
+/**
+ * List of notifications provided by the extension
+ * @type {Object[]}
+ */
 let localData = [];
-let remoteData = [];

[sergei, 2018/01/03 11:12:22]

Could you please add a comment that localData and remoteData are arrays of
notification objects and their role is to cache temporary and persistent
notifications respectively.

[Thomas Greiner, 2018/01/11 18:18:19]

Yep, will do.

 
 /**
  * Regularly fetches notifications and decides which to show.
  * @class
  */
 let Notification = exports.Notification =
 {
   /**
    * Called on module startup.
    */
-  init: function()
-  {
-    let notificationdata = Prefs.notificationdata.data;
-    if (notificationdata)
-    {
-      for (let notification of notificationdata.notifications)
+  init()
+  {
+    let {data} = Prefs.notificationdata;

[sergei, 2018/01/30 17:25:05]

Prefs' properties can be not ready yet, I would recommend to move it into a
function and check whether Prefs are already loaded then call it, otherwise add
the function as a listener for load event, Prefs in parent projects should be
adjusted too.

[sergei, 2018/01/30 17:28:24]

Perhaps alternatively we could do it in `matchesUrl`.

[Thomas Greiner, 2018/01/30 19:17:53]

In theory that's correct but in practice this is not an issue since the
preference is initialized as `{}` (at least in adblockpluschrome). Anyway, I do
agree that it wouldn't be a bad idea to catch it just in case, because it's only
an implicit dependency that we have here which makes it quite unreliable.

Besides that, the same issue also applies to
`Prefs.notifications_ignoredcategories` as well as pretty much every other use
of `Prefs.notificationdata` so we may need to rethink our approach on a more
fundamental level.
One option I could think of would be to introduce a lib/prefs.js module in
adblockpluscore so that it can define and initialize its own preferences.

Would you prefer if I add a workaround for this particular case or should I
create a ticket so that we can tackle the underlying issue separately?

[sergei, 2018/01/30 21:06:48]

It does happen on practice (at least I observed the same issue in
libadblockplus). Let's consider how it works:
there is some moral equivalent of "main.js" which just loads all our js files,
including notification.js and let's say prefs.js is loaded by it too (it's
equivalent when prefs.js is first time loaded by notification.js and
notification.js is loaded as a result of series of calls of `require` BTW), so

CURRENTLY RUNNING JS -- BEGIN --
require("...")
require("prefs")
  init settings to default values and
  load previously stored settings asynchronously
    at some point later
      schedule the call of "done" callback after finishing currently running JS
code
require("...")
require("notification")
  Notification.init()
    here we have only default values for Prefs
require("...")
CURRENTLY RUNNING JS  -- END --
....
CURRENTLY RUNNING JS -- BEGIN --
"done" callback
  set Prefs
now one may use Prefs.
CURRENTLY RUNNING JS  -- END --

BTW, in some test of libadblockplus there were a couple of such cases and for
the sake of simplicity and in order to not bother adblockpluscore we just called
"done" callback for preferences immediately (synchronously), so Prefs are ready
right after `require("prefs")` :)
You are right, currently no one observed the problem here because the
probability of the race condition with Prefs.notifications_ignoredcategories in
particular is too low because it accessed first time after a huge delay, but for
that new code, Prefs will not be populated by asynchronously loaded values yet.

In general of course it would be good to address it at another level, e.g.
either even load the rest of the modules or at least call their `init` functions
after Prefs are loaded. However, it can cause undesired delays, so I think such
complexity (fiddling with the "load" event) looks good for me. Actually, for the
sake of simplicity we can just add a method to Prefs which contains the logic
and either calls a callback immediately or adds it as a listener for some "load"
event.
Maybe it's a good idea to make the decision even about the approach to deal with
such case in an issue, feel free to file as many as you think there should be :)

[kzar, 2018/01/31 11:06:27]

How about using the Prefs.untilLoaded Promise?

[sergei, 2018/02/05 12:49:34]

It seems the best option.

+    if (data)
+    {
+      for (let notification of data.notifications)
+      {
         initNotificationMatcher(notification);
-      remoteData = notificationdata.notifications;
-    }
-
-    downloader = new Downloader(this._getDownloadables.bind(this), INITIAL_DELAY, CHECK_INTERVAL);
-    onShutdown.add(function()
-    {
-      downloader.cancel();
-    });
-
+      }
+    }
+
+    downloader = new Downloader(this._getDownloadables.bind(this),
+                                INITIAL_DELAY, CHECK_INTERVAL);
     downloader.onExpirationChange = this._onExpirationChange.bind(this);
     downloader.onDownloadSuccess = this._onDownloadSuccess.bind(this);
     downloader.onDownloadError = this._onDownloadError.bind(this);
+    onShutdown.add(() => downloader.cancel());
   },
 
   /**
    * Yields a Downloadable instances for the notifications download.
    */
-  _getDownloadables: function()

[kzar, 2018/01/04 17:04:09]

Why are we exporting functions like this and prefixing them with an underscore,
instead of simply not exporting them?

[Thomas Greiner, 2018/01/11 18:18:20]

I don't think it's meant to be exported because it's also not used anywhere. It
was probably just a way to group related functions. But since I haven't created
this I can't say for sure but would think that we could move some functions
which don't refer to `this`, like `_getDownloadables()`, outside of the
`Notification` prototype.

+  *_getDownloadables()
   {
     let downloadable = new Downloadable(Prefs.notificationurl);
     if (typeof Prefs.notificationdata.lastError === "number")
       downloadable.lastError = Prefs.notificationdata.lastError;
     if (typeof Prefs.notificationdata.lastCheck === "number")
       downloadable.lastCheck = Prefs.notificationdata.lastCheck;
-    if (typeof Prefs.notificationdata.data === "object" && "version" in Prefs.notificationdata.data)
+    if (typeof Prefs.notificationdata.data === "object" &&
+        "version" in Prefs.notificationdata.data)
+    {
       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;
+    if (typeof Prefs.notificationdata.downloadCount === "number")
+      downloadable.downloadCount = Prefs.notificationdata.downloadCount;
     yield downloadable;
   },
 
-  _onExpirationChange: function(downloadable)
+  _onExpirationChange(downloadable)
   {
     Prefs.notificationdata.lastCheck = downloadable.lastCheck;
     Prefs.notificationdata.softExpiration = downloadable.softExpiration;
     Prefs.notificationdata.hardExpiration = downloadable.hardExpiration;
     saveNotificationData();
   },
 
-  _onDownloadSuccess: function(downloadable, responseText, errorCallback, redirectCallback)
+  _onDownloadSuccess(downloadable, responseText, errorCallback,
+                     redirectCallback)
   {
     try
     {
       let data = JSON.parse(responseText);
       for (let notification of data.notifications)
       {
         if ("severity" in notification)
         {
           if (!("type" in notification))
             notification.type = notification.severity;
           delete notification.severity;
         }
         initNotificationMatcher(notification);
       }
-      remoteData = data.notifications;

[sergei, 2018/01/03 11:12:22]

It's a bit theoretical but IMO it can be a technical debt.
The difference from the previous approach is that earlier in getNextToShow we
read Prefs.notificationdata.data, which can be theoretically set by several
ways, but now in getNextToShow we are reading remoteData which is set only from
_onDownloadSuccess. IMO, although currently it does not change anything, we
should rather listen to the changes of Prefs and when
Prefs.notificationdata.data is changed remoteData should be updated.

[kzar, 2018/01/04 17:04:09]

Yea, I don't think these changes are necessary now with my other suggestions.

[Thomas Greiner, 2018/01/11 18:18:19]

Good point. I don't remember why I made this change back when I created this
review. It could've been just a leftover from development. Anyway, I agree that
this variable is redundant.

       Prefs.notificationdata.data = data;

[sergei, 2018/01/03 11:12:22]

Despite it's not directly a part of this issue, should new notifications
actually be added to already existing ones instead of overriding? If there are
no new notifications then it will remove all previously stored ones.

[kzar, 2018/01/04 17:04:09]

Hmm, I'm not too familiar with this code but I think you might be right.

[Thomas Greiner, 2018/01/11 18:18:19]

By overriding existing notifications we can control which notifications should
be active on which day. That's probably important for calculating conversion
rates. But again, I haven't implemented the initial logic so I can't say for
sure.

     }
     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);
+    [
+      Prefs.notificationdata.softExpiration,
+      Prefs.notificationdata.hardExpiration
+    ] = downloader.processExpirationInterval(EXPIRATION_INTERVAL);
+    Prefs.notificationdata.downloadCount = downloadable.downloadCount;
     saveNotificationData();
-  },
-
-  _onDownloadError: function(downloadable, downloadURL, error, channelStatus, responseStatus, redirectCallback)
+
+    Notification.showNext();
+  },
+
+  _onDownloadError(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(listener)
+  {
+    if (showListeners.indexOf(listener) == -1)
+      showListeners.push(listener);
+  },
+
+  /**
+   * Removes the supplied listener.
+   * @param {Function} listener Listener that was added via addShowListener()
+   */
+  removeShowListener(listener)
+  {
+    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)
+   * @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));
-    }
-
-    if (!(Prefs.notificationdata.shown instanceof Array))
-    {
-      Prefs.notificationdata.shown = [];
-      saveNotificationData();
-    }
-
+  _getNextToShow(url)
+  {
+    let remoteData = [];
+    if (typeof Prefs.notificationdata.data == "object" &&
+        Prefs.notificationdata.data.notifications instanceof Array)
+    {
+      remoteData = Prefs.notificationdata.data.notifications;
+    }
     let notifications = localData.concat(remoteData);
     if (notifications.length === 0)
       return null;
 
-    let {addonName, addonVersion, application, applicationVersion, platform, platformVersion} = require("info");
+    const {addonName, addonVersion, application,
+           applicationVersion, platform, platformVersion} = require("info");
+
+    let targetChecks = {
+      extension: v => v == addonName,
+      extensionMinVersion:
+        v => compareVersion(addonVersion, v) >= 0,
+      extensionMaxVersion:
+        v => compareVersion(addonVersion, v) <= 0,
+      application: v => v == application,
+      applicationMinVersion:
+        v => compareVersion(applicationVersion, v) >= 0,
+      applicationMaxVersion:
+        v => compareVersion(applicationVersion, v) <= 0,
+      platform: v => v == platform,
+      platformMinVersion:
+        v => compareVersion(platformVersion, v) >= 0,
+      platformMaxVersion:
+        v => compareVersion(platformVersion, v) <= 0,
+      blockedTotalMin: v => Prefs.show_statsinpopup &&
+        Prefs.blocked_total >= v,
+      blockedTotalMax: v => Prefs.show_statsinpopup &&
+        Prefs.blocked_total <= v,
+      locales: v => v.includes(Utils.appLocale)
+    };
+
     let notificationToShow = null;
     for (let notification of notifications)
     {
-      if ((typeof notification.type === "undefined" || notification.type !== "critical")
-          && Prefs.notificationdata.shown.indexOf(notification.id) !== -1)
-        continue;
-
-      if (typeof url === "string" || "_matcher" in notification)
+      if (typeof notification.type === "undefined" ||
+          notification.type !== "critical")
       {
-        if (typeof url === "string" && "_matcher" in notification)
+        let shown;
+        if (typeof Prefs.notificationdata.shown == "object")
+          shown = Prefs.notificationdata.shown[notification.id];
+
+        if (typeof shown != "undefined")
         {
-          if (!notification._matcher.matchesAny(url, "DOCUMENT", url))
+          if (typeof notification.interval == "number")
+          {
+            if (shown + notification.interval > Date.now())
+              continue;
+          }
+          else if (shown)
             continue;
         }
-        else
+
+        if (notification.type !== "relentless" &&
+            Prefs.notifications_ignoredcategories.indexOf("*") != -1)
+        {
           continue;
+        }
       }
+
+      if (!matchesUrl(notification, url))
+        continue;
 
       if (notification.targets instanceof Array)
       {
         let match = false;
+
         for (let target of notification.targets)
         {
-          if (checkTarget(target, "extension", addonName, addonVersion) &&
-              checkTarget(target, "application", application, applicationVersion) &&
-              checkTarget(target, "platform", platform, platformVersion))
+          if (Object.keys(target).every(key =>
+              targetChecks.hasOwnProperty(key) &&
+              targetChecks[key](target[key])))
           {
             match = true;
             break;
           }
         }
         if (!match)
+        {
           continue;
+        }
       }
 
-      if (!notificationToShow
-          || getNumericalSeverity(notification) > getNumericalSeverity(notificationToShow))
+      if (!notificationToShow ||
+          getNumericalSeverity(notification) >
+            getNumericalSeverity(notificationToShow))
         notificationToShow = notification;
     }
 
-    if (notificationToShow && "id" in notificationToShow)
-    {
-      if (notificationToShow.type !== "question")
-        this.markAsShown(notificationToShow.id);
-    }
-
     return notificationToShow;
   },
 
-  markAsShown: function(id)
-  {
-    if (Prefs.notificationdata.shown.indexOf(id) > -1)
-      return;
-
-    Prefs.notificationdata.shown.push(id);
+  /**
+   * Invokes the listeners added via addShowListener() with the next
+   * notification to be shown.
+   * @param {string} url URL to match notifications to (optional)
+   */
+  showNext(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(id)
+  {
+    let now = Date.now();
+    let data = Prefs.notificationdata;
+
+    if (data.shown instanceof Array)
+    {
+      let newShown = {};
+      for (let oldId of data.shown)
+        newShown[oldId] = now;
+      data.shown = newShown;
+    }
+
+    if (typeof data.shown != "object")
+      data.shown = {};
+
+    data.shown[id] = now;
+
     saveNotificationData();
   },
 
   /**
    * Localizes the texts of the supplied notification.
    * @param {Object} notification notification to translate
-   * @param {String} locale the target locale (optional, defaults to the
+   * @param {string} locale the target locale (optional, defaults to the
    *                        application locale)
    * @return {Object} the translated texts
    */
-  getLocalizedTexts: function(notification, locale)
+  getLocalizedTexts(notification, locale)
   {
     locale = locale || Utils.appLocale;
     let textKeys = ["title", "message"];
-    let localizedTexts = [];
+    let localizedTexts = {};
     for (let key of textKeys)
     {
       if (key in notification)
       {
         if (typeof notification[key] == "string")
           localizedTexts[key] = notification[key];
         else
           localizedTexts[key] = localize(notification[key], locale);
       }
     }
     return localizedTexts;
   },
 
   /**
    * Adds a local notification.
    * @param {Object} notification notification to add
    */
-  addNotification: function(notification)
+  addNotification(notification)
   {
     if (localData.indexOf(notification) == -1)
     {
       initNotificationMatcher(notification);
       localData.push(notification);
     }
   },
 
   /**
    * Removes an existing local notification.
    * @param {Object} notification notification to remove
    */
-  removeNotification: function(notification)
+  removeNotification(notification)
   {
     let index = localData.indexOf(notification);
     if (index > -1)
       localData.splice(index, 1);
   },
 
   /**
+   * A callback function which listens to see if notifications were approved.
+   *
+   * @callback QuestionListener
+   * @param {boolean} approved
+   */
+
+  /**
    * 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);
+   * @param {string} id
+   * @param {QuestionListener} listener
+   */
+  addQuestionListener(id, 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))
+   * @param {string} id
+   * @param {QuestionListener} listener
+   */
+  removeQuestionListener(id, listener)
+  {
+    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];
-  },
-
-  /**
-   * 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))
+      questionListeners[id].splice(index, 1);
+    if (questionListeners[id].length === 0)
+      delete questionListeners[id];
+  },
+
+  /**
+   * Notifies question listeners about interactions with a notification
+   * @param {string} id notification ID
+   * @param {boolean} approved indicator whether notification has been approved
+   */
+  triggerQuestionListeners(id, approved)
+  {
+    if (!(id in questionListeners))
       return;
-    let questionListeners = listeners[id];
-    for (let listener of questionListeners)
+    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(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();