Show notifications on startup (Chrome)

background.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
@@ skipping 103 matching lines @@
 function refreshIconAndContextMenu(tab)
 {
   // The tab could have been closed by the time this function is called
   if(!tab)
     return;
 
   var excluded = isWhitelisted(tab.url);
   var iconFilename = excluded ? "icons/abp-19-whitelisted.png" : "icons/abp-19.png";
 
   if (activeNotification)
     startIconAnimation(tab, iconFilename);

[Wladimir Palant, 2013/07/23 11:53:17]

Don't we need to check whether this is actually the active tab? We do some
refreshes for background tabs when we stop animations.

[Felix Dahlke, 2013/07/23 12:52:02]

I don't think it's necessary, since we only refresh background tabs on startup
and right after activeAnimation is set to null. We could do it to be really
safe, but I think then we'd also have to ensure that the tab is in the currently
focused window, and we already have that logic below.

   else
     chrome.pageAction.setIcon({tabId: tab.id, path: iconFilename});
 
   // Only show icon for pages we can influence (http: and https:)
   if(/^https?:/.test(tab.url))
   {
     chrome.pageAction.setTitle({tabId: tab.id, title: "Adblock Plus"});
     if ("shouldShowIcon" in localStorage && localStorage["shouldShowIcon"] == "false")
       chrome.pageAction.hide(tab.id);
     else
@@ skipping 348 matching lines @@
 var iconAnimationTimer = null;
 var animatedIconTab = null;
 
 function stopIconAnimation()
 {
   if (!iconAnimationTimer)
     return;
 
   clearTimeout(iconAnimationTimer);
   iconAnimationTimer = null;
   animatedIconTab = null;

[Wladimir Palant, 2013/07/23 11:53:17]

refreshIconAndContextMenu(animatedIconTab)? Well, not really necessary I
guess...

[Felix Dahlke, 2013/07/23 12:52:02]

Not necessary, no. I used to do that here, but removed it now that
stopIconAnimation is called in startIconAnimation - doesn't really make much
sense to set the icon right before it's being animated IMO.

 }
 
 function loadImages(imageFiles, callback)
 {
   var images = {};
   var imagesLoaded = 0;
   imageFiles.forEach(function(imageFile)
   {
     var image = new Image();
     image.src = imageFile;
@@ skipping 54 matching lines @@
         interval = 10000;
       }
       iconAnimationTimer = setTimeout(animationStep, interval);
     }
     animationStep();
   });
 }
 
 function prepareNotificationIconAndPopup()
 {
-  function refreshAll(tabs)
-  {
-    tabs.forEach(refreshIconAndContextMenu);
-  }
-
   activeNotification.onClicked = function()
   {
+    var tab = animatedIconTab;
     stopIconAnimation();
     activeNotification = null;
-    chrome.tabs.query(null, refreshAll);

[Wladimir Palant, 2013/07/23 11:53:17]

Do we still need to refresh all icons here?

if (animatedIconTab)
{
  var tab = animatedIconTab;
  stopIconAnimation();
  refreshAll([tab]);
}

refreshTabs() might be a better name than refreshAll(), it shouldn't actually
apply to all tabs.

[Felix Dahlke, 2013/07/23 12:52:02]

Done. You're right, it's not necessary anymore now that we reset the icon for
tabs that become inactive.

+    refreshIconAndContextMenu(tab);
   };
 
   chrome.windows.getLastFocused({populate: true}, function(window)
   {
-    chrome.tabs.query({active: true, windowId: window.id}, refreshAll);
+    chrome.tabs.query({active: true, windowId: window.id}, function(tabs)
+    {
+      tabs.forEach(refreshIconAndContextMenu);
+    });
   });
 }
 
 function showNotification(notification)
 {
   activeNotification = notification;
 
   if (activeNotification.severity === "critical")
   {
     var notification = webkitNotifications.createHTMLNotification("notification.html");
@@ skipping 138 matching lines @@
 {
   chrome.tabs.sendRequest(tabId, {reqtype: "clickhide-deactivate"})
   if(changeInfo.status == "loading")
     refreshIconAndContextMenu(tab);
 });
 
 // Refresh icon when switching tabs or windows
 chrome.tabs.onActivated.addListener(function(activeInfo)
 {
   refreshIconAndContextMenu(animatedIconTab);
-  chrome.tabs.get(activeInfo.tabId, function(tab)
-  {
-    refreshIconAndContextMenu(tab);
-  });

[Wladimir Palant, 2013/07/23 11:53:17]

How about:

chrome.tabs.get(activeInfo.tabId, refreshIconAndContextMenu);

[Felix Dahlke, 2013/07/23 12:52:02]

Done.

+  chrome.tabs.get(activeInfo.tabId, refreshIconAndContextMenu);
 });
 chrome.windows.onFocusChanged.addListener(function(windowId)
 {
   refreshIconAndContextMenu(animatedIconTab);
   chrome.tabs.query({active: true, windowId: windowId}, function(tabs)
   {
-    refreshIconAndContextMenu(tabs[0]);

[Wladimir Palant, 2013/07/23 11:53:17]

How about dropping the assumption that there is only one tab:

tabs.forEach(refreshIconAndContextMenu);

[Felix Dahlke, 2013/07/23 12:52:02]

This should really only be one, but yeah, forEach is safer. I think the list can
actually be empty sometimes.

+    tabs.forEach(refreshIconAndContextMenu);
   });
 });
 
 setTimeout(function()
 {
   var notificationToShow = Notification.getNextToShow();
   if (notificationToShow)
     showNotification(notificationToShow);
 }, 3 * 60 * 1000);