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,
-                         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 loadImage =
-/**
- * Loads an image and draws it onto a canvas

[Sebastian Noack, 2016/01/23 14:04:09]

Nit: Missing full stop?

[kzar, 2016/01/23 14:43:14]

Done.

- *
- * @param {string} path  Path of the image to load
- * @param {number} [int]  Size of image to load (replaces "$size" in path)
- * @return {promise}

[Sebastian Noack, 2016/01/23 14:04:09]

Nit: It's "Promise" (capitalized) since it's a class.

[kzar, 2016/01/23 14:43:14]

Done.

- */
-exports.loadImage = function loadImage(path, size)

[Sebastian Noack, 2016/01/23 14:04:08]

Perhaps we should call the function loadCanvas or loadImageAsCanvas?

[Sebastian Noack, 2016/01/23 14:04:09]

Nit: Named function assigned to a variable?

[kzar, 2016/01/23 14:43:13]

Done.

[kzar, 2016/01/23 14:43:13]

Done.

+function loadImage(url)
+{
+  return new Promise((resolve, reject) =>
   {

[Sebastian Noack, 2016/01/23 14:04:08]

Nit: It seems that the indentation is off here.

[kzar, 2016/01/23 14:43:13]

Done.

-    if (!path)

[Sebastian Noack, 2016/01/23 14:04:09]

Ah, now I see why you check for the path being empty. If for whatever reason the
path should be empty neither onload nor onerror would fire and the promise will
never be resolved. However, I'd rather throw an exception in that case.

[kzar, 2016/01/23 14:43:14]

Done.

-      return Promise.resolve(null);
-    if (size)

[Sebastian Noack, 2016/01/23 14:04:09]

I'm not sure whether this logic should be handled here or rather in the calling
code, in particular as it seems to be specific to the code below.

[kzar, 2016/01/23 14:43:13]

Done.

-      path = path.replace("$size", size.toString());
+    let image = new Image();
+    image.src = url;
+    image.addEventListener("load", () =>
+    {
+      resolve(image);
+    });
+    image.addEventListener("error", () =>
+    {
+      reject("Failed to load image " + url);
+    });
+  });
+};
 
-    return new Promise(function(resolve, reject) {
-      let image = new Image();
-      image.src = path;
-      image.addEventListener("load", function()

[Sebastian Noack, 2016/01/23 14:04:09]

Nit: How about using arrow functions?

[kzar, 2016/01/23 14:43:14]

Done.

+function setIcon(page, notificationType, opacity, frames)
+{
+  opacity = opacity || 0;
+  let whitelisted = !!whitelistedState.get(page) && !safariPlatform;
+
+  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]
+    });
+  }
+}
+
+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 images = {
+      19: { base: [images[0], images[1]], overlay: images[2] },
+      38: { base: [images[3], images[4]], overlay: images[5] }
+    };
+
+    let frames = {};
+    let canvas = document.createElement("canvas");
+    let context = canvas.getContext("2d");
+
+    for (let whitelisted of [false, true])
+    {
+      for (let i = 0, opacity = 0; i <= 10; opacity = ++i / 10)
       {
-        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", function(error) { reject(error); });

[Sebastian Noack, 2016/01/23 14:17:41]

Note that the value passed to the listener here isn't an Error but an Event. So
we should create our own Error to reject the promise with.

[kzar, 2016/01/23 14:43:14]

Done.

-    });
-  };
-
-function getImageData(size, baseIcon, overlayIcon, opacity,
-                      animationStep, frameCache)
-{
-  let cacheIndex = baseIcon + size + opacity;
-
-  if (frameCache && frameCache[cacheIndex])
-    return frameCache[cacheIndex];
-
-  let imageData = Promise.all(
-    [loadImage(baseIcon, size), loadImage(overlayIcon, size)]
-  ).then(function(icons)
-  {
-    let baseIconCanvas = icons[0];
-    let overlayIconCanvas = icons[1];
-    let context = baseIconCanvas.getContext("2d");
-
-    if (overlayIconCanvas && opacity)
-    {
-      context.globalAlpha = opacity;
-      context.drawImage(overlayIconCanvas, 0, 0);
+        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)
-    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(function(size)
-  {
-    return getImageData(size, baseIcon, overlayIcon, opacity,
-                        animationStep, frameCache);
-  });
-  Promise.all(imageData).then(function(imageData)
-  {
-    chrome.browserAction.setIcon({tabId: page._id,
-                                  imageData: {19: imageData[0],
-                                              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(function()
-    {
-      ext.pages.query({active: true}, function(pages)
+      frameInterval = setInterval(() =>
       {
-        pages.forEach(function (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 function()
+  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();
-};