Fixed miscellaneous issues with anti-adblock notification port

background.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 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 28 matching lines @@
 var Prefs = require("prefs").Prefs;
 var Synchronizer = require("synchronizer").Synchronizer;
 var Utils = require("utils").Utils;
 var Notification = require("notification").Notification;
 var initAntiAdblockNotification = require("antiadblockInit").initAntiAdblockNotification;
 
 // Some types cannot be distinguished
 RegExpFilter.typeMap.OBJECT_SUBREQUEST = RegExpFilter.typeMap.OBJECT;
 RegExpFilter.typeMap.MEDIA = RegExpFilter.typeMap.FONT = RegExpFilter.typeMap.OTHER;
 
+// Chrome on Linux does not fully support chrome.notifications until version 35
+// https://code.google.com/p/chromium/issues/detail?id=291485
+var canUseChromeNotifications = require("info").platform == "chromium"
+  && "notifications" in chrome
+  && (navigator.platform.indexOf("Linux") == -1 || parseInt(require("info").applicationVersion) > 34);
+
 var isFirstRun = false;
 var seenDataCorruption = false;
 require("filterNotifier").FilterNotifier.addListener(function(action)
 {
   if (action == "load")
   {
     var importingOldData = importOldData();
 
     var addonVersion = require("info").addonVersion;
     var prevVersion = ext.storage.currentVersion;
     if (prevVersion != addonVersion)
     {
       isFirstRun = !prevVersion;
       ext.storage.currentVersion = addonVersion;
       if (!importingOldData)
         addSubscription(prevVersion);
     }
 
+    if (canUseChromeNotifications)
+      initChromeNotifications();
     initAntiAdblockNotification();
   }
 
   // update browser actions when whitelisting might have changed,
   // due to loading filters or saving filter changes
   if (action == "load" || action == "save")
   {
     ext.windows.getAll(function(windows)
     {
       for (var i = 0; i < windows.length; i++)
@@ skipping 229 matching lines @@
   });
 }
 
 function prepareNotificationIconAndPopup()
 {
   var animateIcon = (activeNotification.type !== "question");
   activeNotification.onClicked = function()
   {
     if (animateIcon)
       iconAnimation.stop();
-    activeNotification = null;
+    notificationClosed();
   };
   if (animateIcon)
     iconAnimation.update(activeNotification.type);
 }
 
 function openNotificationLinks() 
 {
   if (activeNotification.links)
   {
     activeNotification.links.forEach(function(link)
     {
       ext.windows.getLastFocused(function(win)
       {
         win.openTab(Utils.getDocLink(link));
       });
     });
   }
 }
 
-function notificationButtonClick(id, index)
+function notificationButtonClick(buttonIndex)
 {
   if (activeNotification.type === "question")
   {
-    Notification.triggerQuestionListeners(activeNotification.id, index === 0);
+    Notification.triggerQuestionListeners(activeNotification.id, buttonIndex === 0);
     Notification.markAsShown(activeNotification.id);
     activeNotification.onClicked();
   }
-  else if (activeNotification.links && activeNotification.links[index])
+  else if (activeNotification.links && activeNotification.links[buttonIndex])
   {
     ext.windows.getLastFocused(function(win)
     {
-      win.openTab(Utils.getDocLink(activeNotification.links[index]));
+      win.openTab(Utils.getDocLink(activeNotification.links[buttonIndex]));
     });
   }
 }
 
 function notificationClosed()
 {
   activeNotification = null;
 }
 
-// Chrome on Linux does not fully support chrome.notifications until version 35

[Felix Dahlke, 2014/03/28 13:10:41]

Since this is executed immediately, I would have expected it at the bottom. How
about putting this in some "initChromeNotifications" function (or whatever) and
calling that at the bottom?

[Thomas Greiner, 2014/03/28 15:57:08]

Done.

-// https://code.google.com/p/chromium/issues/detail?id=291485
-var canUseChromeNotifications = (require("info").platform == "chromium" && "notifications" in chrome && (navigator.platform.indexOf("Linux") == -1 || parseInt(require("info").applicationVersion) > 34));

[Felix Dahlke, 2014/03/28 13:10:41]

The outer parentheses are redundant here. And I'd be in favor of wrapping this
at the &&s, super long line :D

[Thomas Greiner, 2014/03/28 15:57:08]

Done.

-if (canUseChromeNotifications)
-{
-  chrome.notifications.onButtonClicked.addListener(notificationButtonClick);
-  chrome.notifications.onClosed.addListener(notificationClosed);
+function initChromeNotifications()
+{
   // Chrome hides notifications in notification center when clicked so we need to close them
-  chrome.notifications.onClicked.addListener(function(id, callback)
-  {
-    chrome.notifications.clear(id, function(wasCleared)
+  function clearActiveNotification(notificationId)
+  {
+    if (activeNotification && activeNotification.type != "question")

[Felix Dahlke, 2014/03/28 16:16:39]

Why check for the notification type here? Seems to me this should be done for
all notifications that trigger a chrome.notification to be shown.

[Thomas Greiner, 2014/03/28 17:26:51]

What this function does is remove the notification from the notification center
- not close the notification if that's what you thought. It'd be nice to use the
notification center for non-interactive notifications but we can also go with
your suggestion for now.

Additional context:

While it's no problem that regular notifications are being kept in there, in
case users want to check for them, it's problematic for URL-specific
notifications because we can only show a notification again if it's not hidden
in the notification center.

In addition to that, we only save the last notification which means that
interacting with old notifications in the notificiation center wouldn't do
anything. So actually, we have three options:

1) Don't show any notifications in the notification center.

2) Change the check to the one below to only show notifications in the
notification center which do not offer any interaction.

if (activeNotification && (activeNotification.type == "question" || "links" in
activeNotification))

3) Keep all notifications in memory until they are properly closed to make use
of the notification center.

While I chose to go with (2) to at least keep non-interactive notifications in
the notification center, I could also live with (1). (3) would require more
effort to implement and is therefore not an option for now IMHO.

[Felix Dahlke, 2014/03/28 17:33:46]

Ah, no I get it. No it's fine let's go with 2 if you prefer that.

[Thomas Greiner, 2014/03/28 18:23:17]

For completeness' sake, what I meant for (2) was:

if (activeNotification && activeNotification.type != "question" && !("links" in
activeNotification))

+      return;
+    
+    chrome.notifications.clear(notificationId, function(wasCleared)
     {
       if (wasCleared)
         notificationClosed();
     });
+  }
+
+  chrome.notifications.onButtonClicked.addListener(function(notificationId, buttonIndex)
+  {
+    notificationButtonClick(buttonIndex);
+    clearActiveNotification(notificationId);

[Thomas Greiner, 2014/03/28 15:57:08]

I noticed that even notifications that were dismissed by clicking on one of the
buttons were only hidden in the notification center and not cleared so I added
this here.

   });
+  chrome.notifications.onClicked.addListener(clearActiveNotification);
+  chrome.notifications.onClosed.addListener(notificationClosed);
 }
 
 function showNotification(notification)
 {
   if (activeNotification && activeNotification.id === notification.id)
     return;
   
   activeNotification = notification;
   if (activeNotification.type === "critical" || activeNotification.type === "question")
   {
@@ skipping 164 matching lines @@
   tab.sendMessage({type: "clickhide-deactivate"});
   refreshIconAndContextMenu(tab);
 });
 
 setTimeout(function()
 {
   var notificationToShow = Notification.getNextToShow();
   if (notificationToShow)
     showNotification(notificationToShow);
 }, 3 * 60 * 1000);