Issue 3532 - Generate animation images at runtime

lib/icon.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /** @module icon */
 
 "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 frameOpacities =  [0.0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9,
+                         1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0,
+                         0.9, 0.8, 0.7, 0.6, 0.5, 0.4, 0.3, 0.2, 0.1, 0.0];
 const numberOfFrames = frameOpacities.length;
+const safariPlatform = require("info").platform == "safari";
 
-let stopAnimation = null;
-let animationPlaying = false;
+let frameInterval = null;
+let animationInterval = null;
 let whitelistedState = new ext.PageMap();
 
-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.

+function loadImage(url)
 {
-  return new Promise((resolve, reject) => {
+  return new Promise((resolve, reject) =>
+  {
     let image = new Image();
-    image.src = path;
+    image.src = url;
     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);
+      resolve(image);
     });
-    image.addEventListener("error", () => {
-      reject("Failed to load image " + path);
+    image.addEventListener("error", () =>
+    {
+      reject("Failed to load image " + url);
     });
   });
 };
 
-function getImageData(baseIcon, overlayIcon, opacity, animationStep, frameCache)
+function setIcon(page, notificationType, opacity, frames)
 {
-  let cacheIndex = baseIcon + overlayIcon + opacity;
+  opacity = opacity || 0;
+  let whitelisted = !!whitelistedState.get(page) && !safariPlatform;
 
-  if (frameCache && frameCache[cacheIndex])
-    return frameCache[cacheIndex];
+  if (!notificationType || !frames)
+  {
+    if (opacity > 0.5)
+      page.browserAction.setIcon("icons/abp-$size-notification-"
+                                 + notificationType + ".png");
+    else
+      page.browserAction.setIcon("icons/abp-$size" +
+                                 (whitelisted ? "-whitelisted" : "") + ".png");
+  }
+  else
+  {
+    chrome.browserAction.setIcon({
+      tabId: page._id,
+      imageData: frames["" + opacity + whitelisted]
+    });
+  }
+}
 
-  let imageData = Promise.all(
-    [loadImageAsCanvas(baseIcon),
-     overlayIcon && loadImageAsCanvas(overlayIcon) || Promise.resolve(null)]
-  ).then(icons =>
+function renderFrames(notificationType)
+{
+  if (safariPlatform)
+    return Promise.resolve(null);
+
+  return Promise.all([
+    loadImage("icons/abp-19.png"),
+    loadImage("icons/abp-19-whitelisted.png"),
+    loadImage("icons/abp-19-notification-" + notificationType + ".png"),
+    loadImage("icons/abp-38.png"),
+    loadImage("icons/abp-38-whitelisted.png"),
+    loadImage("icons/abp-38-notification-" + notificationType + ".png"),
+  ]).then(images =>
   {
-    let baseIconCanvas = icons[0];
-    let overlayIconCanvas = icons[1];
-    let context = baseIconCanvas.getContext("2d");
+    let images = {
+      19: { base: [images[0], images[1]], overlay: images[2] },
+      38: { base: [images[3], images[4]], overlay: images[5] }
+    };
 
-    if (overlayIconCanvas && opacity)
+    let frames = {};
+    let canvas = document.createElement("canvas");
+    let context = canvas.getContext("2d");
+
+    for (let whitelisted of [false, true])
     {
-      context.globalAlpha = opacity;
-      context.drawImage(overlayIconCanvas, 0, 0);
+      for (let i = 0, opacity = 0; i <= 10; opacity = ++i / 10)
+      {
+        let imageData = {};
+        for (let size of [19, 38])
+        {
+          canvas.width = size;
+          canvas.height = size;
+          context.globalAlpha = 1;
+          context.drawImage(images[size]["base"][whitelisted | 0], 0, 0);
+          context.globalAlpha = opacity;
+          context.drawImage(images[size]["overlay"], 0, 0);
+          imageData[size] = context.getImageData(0, 0, size, size);
+        }
+        frames["" + opacity + whitelisted] = imageData;
+      }
     }
 
-    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, animationType, animationStep, frameCache)
-{
-  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]}});
+    return frames;
   });
 }
 
-function runAnimation(animationType)
+function runAnimation(notificationType)
 {
-  let frameCache = {};
-  let frameInterval;
+  function playAnimation(frames)
+  {
+    let animationStep = 0;
+    ext.pages.query({active: true}, pages =>
+    {
+      function appendActivePage(page)
+      {
+        pages.push(page);
+      }
+      ext.pages.onActivated.addListener(appendActivePage);
 
-  function playAnimation()
-  {
-    animationPlaying = true;
-    let animationStep = 0;
-    frameInterval = setInterval(() =>
-    {
-      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.)

+      frameInterval = setInterval(() =>
       {
-        pages.forEach(page => {
-          setIcon(page, animationType, animationStep++, frameCache);
-        });
-        if (animationStep >= numberOfFrames)
+        let opacity = frameOpacities[animationStep++];
+        for (let page of pages)
+        {
+          if (whitelistedState.has(page))
+            setIcon(page, notificationType, opacity, frames);
+        };
+
+        if (animationStep > numberOfFrames)
         {
           animationStep = 0;
           clearInterval(frameInterval);
-          animationPlaying = false;
+          frameInterval = null;
+          ext.pages.onActivated.removeListener(appendActivePage);
         }
-      });
-    }, 100);
+      }, 100);
+    });
   }
 
-  playAnimation();
-  let animationInterval = setInterval(playAnimation, 10000);
-  return () =>
+  renderFrames(notificationType).then(frames =>
   {
-    clearInterval(frameInterval);
-    clearInterval(animationInterval);
-    animationPlaying = false;
-  };
+    playAnimation(frames);
+    animationInterval = setInterval(() => { playAnimation(frames); }, 10000);
+  });
 }
 
 /**
  * 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
  */
 exports.updateIcon = function(page, whitelisted)
 {
   whitelistedState.set(page, whitelisted);
-  if (!animationPlaying)
+  if (frameInterval == null)
     setIcon(page);
+};
+
+let stopIconAnimation =
+/**
+ * Stops to animate the browser action icon.
+ */
+exports.stopIconAnimation = function()
+{
+  if (frameInterval != null)
+    clearInterval(frameInterval);
+  if (animationInterval != null)
+    clearInterval(animationInterval);
+  frameInterval = animationInterval = null;
 };
 
 /**
  * Starts to animate the browser action icon to indicate a pending notifcation.
  *
  * @param {string} type  The notification type (i.e: "information" or "critical")
  */
 exports.startIconAnimation = function(type)
 {
-  stopAnimation && stopAnimation();
-  stopAnimation = runAnimation(type);
+  stopIconAnimation();
+  runAnimation(type);
 };
-
-/**
- * Stops to animate the browser action icon.
- */
-exports.stopIconAnimation = function()
-{
-  stopAnimation && stopAnimation();
-};