Issue 1965 - Simplified and fixed missing image for icon animation

lib/icon.js

Index: lib/icon.js
===================================================================
rename from iconAnimation.js
rename to lib/icon.js
--- a/iconAnimation.js
+++ b/lib/icon.js
@@ -15,93 +15,112 @@
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-iconAnimation = {
-  _icons: new ext.PageMap(),
-  _animatedPages: new ext.PageMap(),
-  _step: 0,
+let whitelistedState = new ext.PageMap();
+let notificationType = null;
+let animationInterval = null;
+let animationStep = 0;
 
-  update: function(type)
+function getIconPath(whitelisted)
+{
+  let filename = "icons/abp-$size";
+
+  // If the current page is whitelisted, pick an icon that indicates that
+  // Adblock Plus is disabled, however not when the notification icon has
+  // full opacity, or on Safari where icons are genrally grayscale-only.
+  if (whitelisted && animationStep < 10 && require("info").platform != "safari")
+    filename += "-whitelisted";
+
+  // If the icon is currently animating to indicate a pending notification,
+  // pick the icon for the corresponing notification type and animation frame.
+  if (notificationType && animationStep > 0)
   {
-    if (type == this._type)
-       return;
+    filename += "-notification-" + notificationType;
 
-    if (!this._type)
-      this._start();
+    if (animationStep < 10)
+      filename += "-" + animationStep;
+  }
 
-    this._type = type;
-  },
-  stop: function()
+  return filename + ".png";
+}
+
+function setIcon(page)
+{
+  page.browserAction.setIcon(getIconPath(whitelistedState.get(page)));
+}
+
+function runAnimation()
+{
+  return setInterval(function()
   {
-    clearInterval(this._interval);
+    ext.pages.query({active: true}, function(pages)
+    {
+      let fadeInInterval = setInterval(function()
+      {
+        animationStep++;
+        pages.forEach(setIcon);
 
-    delete this._interval;
-    delete this._type;
-
-    this._animatedPages.clear();
-  },
-  registerPage: function(page, icon)
-  {
-    this._icons.set(page, icon);
-
-    if (this._animatedPages.has(page))
-      this._updateIcon(page);
-  },
-  _start: function()
-  {
-    this._interval = setInterval(function()
-    {
-      ext.pages.query({active: true}, function(pages)
-      {
-        if (pages.length == 0)
+        if (animationStep < 10)

[Felix Dahlke, 2015/02/12 04:52:56]

We're checking for `animationStep < 10` three times in this file - this calls
for a constant.

[Sebastian Noack, 2015/02/12 11:15:29]

Done.

           return;
 
-        for (var i = 0; i < pages.length; i++)
-          this._animatedPages.set(pages[i], null);
+        setTimeout(function()
+        {
+          let fadeOutInterval = setInterval(function()
+          {
+            animationStep--;
+            pages.forEach(setIcon);
 
-        var interval = setInterval(function()
-        {
-          this._step++;
-          pages.forEach(this._updateIcon.bind(this));
+            if (animationStep > 0)
+              return;
 
-          if (this._step < 10)
-            return;
+            clearInterval(fadeOutInterval);
+          }, 100);
+        },1000);
 
-          clearInterval(interval);
-          setTimeout(function()
-          {
-            interval = setInterval(function()
-            {
-              this._step--;
-              pages.forEach(this._updateIcon.bind(this));
+        clearInterval(fadeInInterval);
+      }, 100);
+    });
+  }, 15000);
+}
 
-              if (this._step > 0)
-                return;
+/**
+ * Set the browser action icon for the given page, indicating whether
+ * adblocking is active there, and considering the icon animation.
+ *
+ * @param {Page}    [page]        The page to set the browser action icon for
+ * @param {Boolean} [whitelisted] Whether the page has been whitelisted
+ */
+function updateIcon(page, whitelisted)
+{
+  page.browserAction.setIcon(getIconPath(whitelisted));
+  whitelistedState.set(page, whitelisted);
+}
+exports.updateIcon = updateIcon;
 
-              clearInterval(interval);
-              this._animatedPages.clear();
-            }.bind(this), 100);
-          }.bind(this), 1000);
-        }.bind(this), 100);
-      }.bind(this));
-    }.bind(this), 15000);
-  },
-  _updateIcon: function(page)
+/**
+ * Starts to animate the browser action icon to indicate a pending notifcation.
+ *
+ * @param {string} [type] The notifaction type (i.e: "information" or "critical")
+ */
+function startIconAnimation(type)
+{
+  notificationType = type;
+
+  if (animationInterval == null)
+    animationInterval = runAnimation();
+}
+exports.startIconAnimation = startIconAnimation;
+
+/**
+ * Stops to animate the browser action icon.
+ */
+function stopIconAnimation()

[Felix Dahlke, 2015/02/12 04:52:56]

Now that I'm giving this code a good read - don't we have to reset the icon for
all tabs (active or inactive) when we call this?

How I see it: If a tab becomes inactive during the animation, it's going to get
stuck with some intermediate frame. background.js will only update the icon when
a new URL is loaded, not when the tab is made active again.

[Sebastian Noack, 2015/02/12 11:15:29]

To keep things simple we let the current transition finish, when the icon
animation is stopped. Otherwise we would need to consider all internal timeouts
and intervals used by the animation, clearing them. Moreover, it looks more
smooth when the current transition finishes instead resetting the icon
immediately.
We check for active tabs everytime we start to transition the icon to the
notification icon and back. That means if a tab becomes inactive while the
transition runs, we just keep running the transition for it. So it doesn't get
stuck.

[Felix Dahlke, 2015/02/12 14:14:30]

You're right, I somehow thought we'd abort the transition. The behaviour does
make most sense the way it's implemented.

+{
+  if (animationInterval != null)
   {
-    var path = this._icons.get(page);
+    clearInterval(animationInterval);
+    animationInterval = null;
+  }
 
-    if (!path)
-      return;
-
-    if (this._step > 0)
-    {
-      var suffix = "-notification-" + this._type;
-
-      if (this._step < 10)
-        suffix += "-" + this._step;
-
-      path = path.replace(/(?=\..+$)/, suffix);
-    }
-
-    page.browserAction.setIcon(path);
-  }
-};
+  notificationType  = null;
+}
+exports.stopIconAnimation = stopIconAnimation;