Issue 3532 - Generate animation images at runtime

lib/icon.js

Index: lib/icon.js
diff --git a/lib/icon.js b/lib/icon.js
index 2011d1e23701aada50df48f3f2722514d3b3b24e..1c79fa96e3556d4f2c1c37ebc65594cb09d9f4f2 100644
--- a/lib/icon.js
+++ b/lib/icon.js
@@ -17,73 +17,146 @@
 
 /** @module icon */
 
-const numberOfFrames = 10;
+"use strict";
 
+const frameOpacities =  [0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9,
+                         1, 1, 1, 1, 1,

[Sebastian Noack, 2016/01/23 20:38:30]

Since, we have 10 frames per second now, you also need to repeat 1 (full
opacity) twice as often to keep showing the icon for a full second.

Nit: If you use 0.0 (instead 0) and 1.0 instead (1) that array definition looks
a little nicer as the values align.

[kzar, 2016/01/23 21:16:56]

Done.

+                         0.9, 0.8, 0.7, 0.6, 0.5, 0.4, 0.3, 0.2, 0.1, 0];
+const numberOfFrames = frameOpacities.length;
+
+let stopAnimation = null;
+let animationPlaying = false;
 let whitelistedState = new ext.PageMap();
-let notificationType = null;
-let animationInterval = null;
-let animationStep = 0;
 
-function getIconPath(whitelisted)
+let loadImageAsCanvas =
+/**
+ * Loads an image and draws it onto a canvas.
+ *
+ * @param {string} path  Path of the image to load

[Sebastian Noack, 2016/01/23 20:38:29]

Nit: "url" would be more accurrate than "path" as it infect can be any URL not
only a relative path in the extension bundle.

[kzar, 2016/01/23 21:16:56]

Done.

+ * @return {Promise}
+ */
+exports.loadImageAsCanvas = function (path, size)

[Sebastian Noack, 2016/01/23 20:38:29]

Nit: No space before arguments list.

[Sebastian Noack, 2016/01/23 20:38:29]

The size argument is unused.

[kzar, 2016/01/23 21:16:55]

Done.

[kzar, 2016/01/23 21:16:56]

Done.

 {
-  let filename = "icons/abp-$size";
+  return new Promise((resolve, reject) => {
+    let image = new Image();
+    image.src = path;
+    image.addEventListener("load", () =>
+    {
+      let canvas = document.createElement("canvas");
+      let context = canvas.getContext("2d");
+      canvas.height = image.height;
+      canvas.width = image.width;
+      context.drawImage(image, 0, 0);
+      resolve(canvas);
+    });
+    image.addEventListener("error", () => {
+      reject("Failed to load image " + path);
+    });
+  });
+};
 
-  // 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 < numberOfFrames && require("info").platform != "safari")
-    filename += "-whitelisted";
+function getImageData(baseIcon, overlayIcon, opacity, animationStep, frameCache)
+{
+  let cacheIndex = baseIcon + overlayIcon + opacity;
 
-  // 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)
-  {
-    filename += "-notification-" + notificationType;
+  if (frameCache && frameCache[cacheIndex])
+    return frameCache[cacheIndex];
 
-    if (animationStep < numberOfFrames)
-      filename += "-" + animationStep;
-  }
+  let imageData = Promise.all(
+    [loadImageAsCanvas(baseIcon),
+     overlayIcon && loadImageAsCanvas(overlayIcon) || Promise.resolve(null)]
+  ).then(icons =>
+  {
+    let baseIconCanvas = icons[0];
+    let overlayIconCanvas = icons[1];
+    let context = baseIconCanvas.getContext("2d");
 
-  return filename + ".png";
+    if (overlayIconCanvas && opacity)
+    {
+      context.globalAlpha = opacity;
+      context.drawImage(overlayIconCanvas, 0, 0);
+    }
+
+    return context.getImageData(0, 0, baseIconCanvas.width,
+                                baseIconCanvas.height);
+  });
+  if (frameCache)

[Sebastian Noack, 2016/01/23 20:38:29]

Instead of caching we could probably simplify the logic by pre-rendering all
frames in advance. Another advantage of that approach would be that the first
time the icon is animated there is no overhead that might slow down the
animation on slower computers. And we wouldn't load the source images again for
each frame.

I experimented, with that approach and got as far as following (untested)
function. Feel free to use that code as-is or just for inspiration:

function renderAnimationFrames()
{
  return Promise.all([
    loadImageAsCanvas(getIconPath(19, "")),
    loadImageAsCanvas(getIconPath(19, "-whitelisted")),
    loadImageAsCanvas(getIconPath(19, "-notification-" + notificationType)),
    loadImageAsCanvas(getIconPath(38, "")),
    loadImageAsCanvas(getIconPath(38, "-whitelisted")),
    loadImageAsCanvas(getIconPath(38, "-notification-" + notificationType))
  ]).then(images => {
    return [false, true].map((whitelisted, i) =>
    {
      var frames = [];

      for (let opacity = 0; opacity += 0.1; opacity <= 1)
      {
        let imageData = {};

        [19, 38].forEach((size, j) =>
        {
          let baseImage    = images[j * 3 + i];
          let overlayImage = images[j * 3 + 2];

          let canvas = document.createElement("canvas");
          let context = canvas.getContext("2d");

          canvas.width = size;
          canvas.height = size;

          context.drawImage(baseImage, 0, 0);
          context.globalAlpha = opacity;
          context.drawImage(overlayImage, 0, 0);

          imageData[size] = context.getImageData(0, 0, size, size);
        });

        frames.push(imageData);
      }

      return frames;
    });
  });
}

[kzar, 2016/01/23 21:16:57]

I did consider pre-rendering them all, or caching everything but I was worried
that it might consume a bunch of memory. I decided that caching the current
animation only was a good compromise. That way when no animation is playing
we're not wasting any memory.

Having said that I'm flexible, I don't mind pre-rendering them all if you think
the memory tradeoff is worth it.

[Sebastian Noack, 2016/01/23 21:26:32]

The idea is NOT to prerender them on extension intilization. But prerender the
whole animation prior to performing it.

[kzar, 2016/01/24 15:43:00]

Ah I see, I think you're right. Done.

+    frameCache[cacheIndex] = imageData;
+  return imageData;
 }
 
-function setIcon(page)
+function setIcon(page, animationType, animationStep, frameCache)
 {
-  page.browserAction.setIcon(getIconPath(whitelistedState.get(page)));
+  let safari = require("info").platform == "safari";
+  let opacity = animationType ? frameOpacities[animationStep] : 0;
+  let greyed = whitelistedState.get(page) && !safari;
+  let blending = (animationType && opacity > 0 && opacity < 1);
+
+  let filename = "icons/abp-$size";
+  let baseIcon = filename + (greyed ? "-whitelisted" : "") + ".png";
+  let overlayIcon = animationType && (filename + "-notification-" +
+                                      animationType + ".png");
+
+
+  // If the icon doesn't need any modifications, or the platform doesn't support
+  // data URLs, we can just use the image's filename with the $size placeholder.
+  if (!blending || safari)
+    if (overlayIcon && opacity > 0.5)
+      return page.browserAction.setIcon(overlayIcon);
+    else
+      return page.browserAction.setIcon(baseIcon);
+
+  // Otherwise we must process the images using a canvas and return a data URL
+  // of the result for each size that's required. (19px and 38px are required by
+  // Chrome/Opera.)
+  let imageData = [19, 38].map(size =>
+  {
+    return getImageData(baseIcon.replace("$size", size.toString()),
+                        overlayIcon.replace("$size", size.toString()),
+                        opacity, animationStep, frameCache);
+  });
+  Promise.all(imageData).then(imageData =>
+  {
+    chrome.browserAction.setIcon({tabId: page._id,
+                                  imageData: {19: imageData[0],

[Sebastian Noack, 2016/01/23 20:38:29]

How about generating the object as it is used in the first place rather than
creating an array and turning it into an object here.

[kzar, 2016/01/23 21:16:55]

That would be nice but I'm not sure how to do it cleanly as I don't think
chrome.browserAction.setIcon can take promises for the image data. 

[Sebastian Noack, 2016/01/23 21:26:32]

See my other comment, and the code snippet I provided there.

+                                              38: imageData[1]}});
+  });
 }
 
-function runAnimation()
+function runAnimation(animationType)
 {
-  return setInterval(function()
+  let frameCache = {};
+  let frameInterval;
+
+  function playAnimation()
   {
-    ext.pages.query({active: true}, function(pages)
+    animationPlaying = true;
+    let animationStep = 0;
+    frameInterval = setInterval(() =>
     {
-      let fadeInInterval = setInterval(function()
+      ext.pages.query({active: true}, pages =>

[Sebastian Noack, 2016/01/23 20:38:29]

Querying every 100ms for active tabs will impair the performance. We have a
hotspot here. Note that the old logic merely checked every 10 seconds for active
tabs running the animation on them. Though this caused the icon animation to be
interrupted when tabs switch. If you don't want to live with that limitation you
should listen on the onActivated event.

[kzar, 2016/01/23 21:16:56]

Ah, whoops. I changed this around because I noticed that when you closed a tab
during an animation a lot of exceptions would be thrown.

I've changed it back, I'm not sure that avoiding those exceptions is worth
setting up a `chrome.tabs.onRemoved` listener, what do you think?

[Sebastian Noack, 2016/01/23 21:26:33]

Well, exceptions shouldn't occur. The easiest way, that would probably work, to
avoid them, would be not calling chrome.browserAction.setIcon if there is no
entry in the PageMap we use to determine whether the icon is in
whitelisted/disabled state.

[kzar, 2016/01/23 22:01:06]

On second thoughts I think it is worth adding the event listeners. Otherwise
when you switch tab mid animation the animation continues to play for the wrong
tab. (FWIW I tried your idea of checking `whitelistedState.has(page)` and it
worked to avoid the exceptions.)

       {
-        animationStep++;
-        pages.forEach(setIcon);
-
-        if (animationStep < numberOfFrames)
-          return;
-
-        setTimeout(function()
+        pages.forEach(page => {
+          setIcon(page, animationType, animationStep++, frameCache);
+        });
+        if (animationStep >= numberOfFrames)
         {
-          let fadeOutInterval = setInterval(function()
-          {
-            animationStep--;
-            pages.forEach(setIcon);
-
-            if (animationStep > 0)
-              return;
-
-            clearInterval(fadeOutInterval);
-          }, 100);
-        },1000);
+          animationStep = 0;
+          clearInterval(frameInterval);
+          animationPlaying = false;
+        }
+      });
+    }, 100);
+  }
 
-        clearInterval(fadeInInterval);
-      }, 100);
-    });
-  }, 10000);
+  playAnimation();
+  let animationInterval = setInterval(playAnimation, 10000);
+  return () =>
+  {
+    clearInterval(frameInterval);
+    clearInterval(animationInterval);
+    animationPlaying = false;
+  };
 }
 
 /**
@@ -95,8 +168,9 @@ function runAnimation()
  */
 exports.updateIcon = function(page, whitelisted)
 {
-  page.browserAction.setIcon(getIconPath(whitelisted));
   whitelistedState.set(page, whitelisted);
+  if (!animationPlaying)
+    setIcon(page);
 };
 
 /**
@@ -106,10 +180,8 @@ exports.updateIcon = function(page, whitelisted)
  */
 exports.startIconAnimation = function(type)
 {
-  notificationType = type;
-
-  if (animationInterval == null)
-    animationInterval = runAnimation();
+  stopAnimation && stopAnimation();
+  stopAnimation = runAnimation(type);
 };
 
 /**
@@ -117,11 +189,5 @@ exports.startIconAnimation = function(type)
  */
 exports.stopIconAnimation = function()
 {
-  if (animationInterval != null)
-  {
-    clearInterval(animationInterval);
-    animationInterval = null;
-  }
-
-  notificationType  = null;
+  stopAnimation && stopAnimation();
 };