Issue 1965 - Simplified and fixed missing image for icon animation

background.js

Index: background.js
===================================================================
--- a/background.js
+++ b/background.js
@@ -100,25 +100,59 @@
 
 var activeNotification = null;
 
-// Adds or removes browser action icon according to options.
-function refreshIconAndContextMenu(tab)
+function getTabStatus(tab, animation)
 {
   var whitelisted = isWhitelisted(tab.url);
 
-  var iconFilename;
+  var icon;
   if (require("info").platform == "safari")
-    // There is no grayscale version of the icon for whitelisted tabs
-    // when using Safari, because icons are grayscale already and icons
-    // aren't per tab in Safari.
-    iconFilename = "icons/abp-16.png"
+    // pick an icon with approtiate size for Safari. However since
+    // toolbar item icons can't use different colors, we don't have
+    // different versions indicating whether the page is whitelisted
+    icon = "icons/abp-16";
   else
-    iconFilename = whitelisted ? "icons/abp-19-whitelisted.png" : "icons/abp-19.png";
+  {
+    // pick an icon with the approtiate size for Chrome/Opera
+    // that indicates whether the page is whitelisted
+    if (whitelisted && (!animation || animation.step < 10))

[Felix Dahlke, 2014/02/27 22:02:05]

So we will use the red ABP icon if the animation step is >= 10? Why not the grey
one?

[Sebastian Noack, 2014/02/28 08:35:16]

We are neither going to use the red default nor the gray whitelisted icon, but
the notification icon at full opacity, which is the same for both states, and is
called "abp-[16|19]-notification-[information|critical].png".

+      icon = "icons/abp-19-whitelisted";
+    else
+      icon = "icons/abp-19";
+  }
 
-  tab.browserAction.setIcon(iconFilename);
-  iconAnimation.registerTab(tab, iconFilename);
+  // pick the current frame if the icon is currently animating
+  // in order to indicate a pending notification
+  if (animation && animation.step > 0)
+  {
+    icon += "-notification-";
+    icon += animation.severity;

[Wladimir Palant, 2014/03/06 14:31:35]

Nit: why split this to two statements? This seems more intuitive:

  icon += "-notification-" + animation.severity;

Same below with animation.step.

[Sebastian Noack, 2014/03/06 20:41:13]

I found it slightly more readable to have one operation per line here.
Done.

 
-  // Set context menu status according to whether current tab has whitelisted domain
-  if (whitelisted || !/^https?:/.test(tab.url))
+    if (animation.step < 10)
+    {
+      icon += "-";
+      icon += animation.step;
+    }
+  }
+
+  icon += ".png";
+
+  return {icon: icon, whitelisted: whitelisted};
+}
+
+function refreshIconAndContextMenu(tab)
+{
+  var status = getTabStatus(tab);
+
+  // update icon indicating whether the page is whitelisted.
+  // But don't interfere the icon animation if active.

[Felix Dahlke, 2014/02/27 22:02:05]

s/interfere/interrupt/?

[Sebastian Noack, 2014/02/28 08:35:16]

Don't know. It wouldn't just pause the animation, but inject a wrong frame
(showing the default icon) into the animation. Do you call that interrupting or
interfering?

[Wladimir Palant, 2014/03/06 14:31:35]

If we now have a generic function to calculate the icon, why add this special
case in the first place?

  var animation = iconAnimation.tabs.get(tab) ? iconAnimation : null;

This can be added to getTabStatus() instead of the animation parameter. And then
you will always set the correct icon, animation or not.

[Wladimir Palant, 2014/03/06 14:34:33]

In fact, this isn't even necessary. If iconAnimation.stop() resets
iconAnimation.step to zero then getTabStatus() can just always work with
iconAnimation.step. Not sure whether iconAnimation.tabs is even required then.

[Sebastian Noack, 2014/03/06 20:41:13]

Done.

On 2014/03/06 14:34:33, Wladimir Palant wrote:
The reason this is necessary is that otherwise when you open a new tab while the
animation is running, the icon for the new tab will be a still of the current
step of the animation.

[Wladimir Palant, 2014/03/06 20:49:44]

Isn't that exactly the issue that registerTab() was addressing? The animation
should be running for new tabs as well. You seem to have overlooked that
comment.

[Sebastian Noack, 2014/03/07 17:26:11]

Now it is addressed within getTabStatus() as you suggested.

[Wladimir Palant, 2014/03/07 22:13:29]

Still missing something that would start the animation for new tabs (like the
registerTab() function you removed).

[Sebastian Noack, 2014/03/08 11:36:39]

Considering that the animation is interrupted anyway when you switch to an other
open tab, I don't know it is worth to  keep the animation running while opening
a new tab. If we don't want to interrupt the animation when switching tabs, we
would have to update the icon always for all tabs. What do you think?

[Wladimir Palant, 2014/03/21 14:10:11]

Why interrupted? The animation step doesn't change when you open a new tab. So
the icon we show for this tab should be correct - if we consider the animation
there at all.

[Sebastian Noack, 2014/03/31 13:20:32]

Since the icon is set per tab, when the active tab changes, the icon changes as
well.

Of course we could update the icon every time a new tab becomes active. But
beside that case, onActivated wouldn't be needed anymore and therefore is going
to be removed with http://codereview.adblockplus.org/5464830253203456/.

Alternatively we could update the icon for every tab, regardless whether it is
visible or not, while animating.

Or we could limit the animation to the tabs that were visible when the animation
started. This is how it is currently implemented.

+  // In that case the icon animation takes care to update
+  // the icon to its new status with its next frame.
+  if (!iconAnimation.tabs.has(tab))
+    tab.browserAction.setIcon(status.icon);
+
+  // show or hide the context menu entry dependent on whether
+  // adblocking is active in the tab
+  if (status.whitelisted || !/^https?:/.test(tab.url))
     ext.contextMenus.hideMenuItems();
   else
     ext.contextMenus.showMenuItems();