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..9a72c63df67e4098a4afa8879737833fea64a63c 100644
--- a/lib/icon.js
+++ b/lib/icon.js
@@ -17,73 +17,158 @@
 
 /** @module icon */
 
-const numberOfFrames = 10;
+"use strict";
 
+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 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} url  URL string of the image to load
+ * @return {Promise}
+ */
+exports.loadImageAsCanvas = function(url)
 {
-  let filename = "icons/abp-$size";
+  return new Promise((resolve, reject) => {
+    let image = new Image();
+    image.src = url;
+    image.addEventListener("load", () =>
+    {
+      let canvas = document.createElement("canvas");
+      let context = canvas.getContext("2d");
+      canvas.height = image.height;

[Sebastian Noack, 2016/01/25 15:44:45]

With the new approach, we don't necessarily need a canvas element anymore, but
just something that we be passed to drawImage(). So converting the image to a
canvas is redundant for the logic we have in this script.

As for notificationHelper.js, I wonder whether we actually need a data URL or
there or whether we could simply pass the URL of the icon instead? The
assumption there was that the icon would have to be a web accessible resource
otherwise. But I never doubled checked.

[kzar, 2016/01/26 15:01:42]

Good point about the canvas, Done. Not sure about just passing the URL, I've
asked Ross and others for an example notification, if I get one I'll try and
confirm.

+      canvas.width = image.width;
+      context.drawImage(image, 0, 0);
+      resolve(canvas);
+    });
+    image.addEventListener("error", () => {
+      reject("Failed to load image " + url);
+    });
+  });
+};
 
-  // 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 setIcon(page, animationType, opacity, frames)
+{
+  opacity = opacity || 0;
+  let greyed = whitelistedState.get(page) && !safariPlatform && true || false;

[Sebastian Noack, 2016/01/25 15:44:43]

Nit: Please don't add redundant boilerplate to cast values too bool. Not that it
would be any other type here anyway.

[kzar, 2016/01/26 15:01:41]

Actually the value here is either null or the whitelisting filter. Also that
does matter, we're converting the bool to a string as part of the frame key.

[Sebastian Noack, 2016/01/26 16:12:36]

I see. But how about:

  let whitelisted = !!whitelistedState.get(page) && !safariPlatform


Also note that I called the variable whitelisted for consistence with the
terminology everywhere else here.

[kzar, 2016/01/26 17:35:10]

Done.

 
-  // 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 (!animationType || !frames)
   {
-    filename += "-notification-" + notificationType;
-
-    if (animationStep < numberOfFrames)
-      filename += "-" + animationStep;
+    if (animationType && opacity > 0.5)
+      page.browserAction.setIcon("icons/abp-$size-notification-"
+                                 + animationType + ".png");
+    else
+      page.browserAction.setIcon("icons/abp-$size" +
+                                 (greyed ? "-whitelisted" : "") + ".png");
+  }
+  else
+  {
+    frames.then(frames =>
+    {
+      chrome.browserAction.setIcon({
+        tabId: page._id,
+        imageData: {19: frames["" + opacity + greyed + 19],
+                    38: frames["" + opacity + greyed + 38]}
+      });
+    });
   }
-
-  return filename + ".png";
 }
 
-function setIcon(page)
+function renderFrames(animationType)
 {
-  page.browserAction.setIcon(getIconPath(whitelistedState.get(page)));
+  return Promise.all([
+    loadImageAsCanvas("icons/abp-19.png"),
+    loadImageAsCanvas("icons/abp-19-whitelisted.png"),
+    loadImageAsCanvas("icons/abp-19-notification-" + animationType + ".png"),
+    loadImageAsCanvas("icons/abp-38.png"),
+    loadImageAsCanvas("icons/abp-38-whitelisted.png"),
+    loadImageAsCanvas("icons/abp-38-notification-" + animationType + ".png"),
+  ]).then(images =>
+  {
+    let images = {
+      19: { base: [images[0], images[1]], overlay: images[2] },

[Sebastian Noack, 2016/01/25 15:44:44]

While using a structured object here, how about using a separate name for the
whitelisted image, rather than nesting an array? That way all values are
documented, and you keep the data structures flat/simple.

  { default: images[0], whitelisted: images[1], overlay: images[2] }

[kzar, 2016/01/26 15:01:40]

Could do but then the logic later wouldn't be as easy I suppose.
`images[size]["base"][whitelisted | 0]` would instead need a conditional. I
don't feel too strongly either way, up to you.

[Sebastian Noack, 2016/01/26 16:12:35]

Fair enough.

+      38: { base: [images[3], images[4]], overlay: images[5] }
+    };
+
+    let frames = {};
+    let canvas = document.createElement("canvas");
+    let context = canvas.getContext("2d");
+
+    for (let size of [19, 38])
+    {
+      canvas.width = size;
+      canvas.height = size;
+      for (let whitelisted of [false, true])
+      {
+        for (let opacity of [0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9, 1])

[Sebastian Noack, 2016/01/25 15:44:44]

I'd rather generate the opacity values rather than hard-coding them:

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

[kzar, 2016/01/26 15:01:40]

As would I but unfortunately that doesn't work. (Try 0.2 + 0.1 in the console!)

[Sebastian Noack, 2016/01/26 16:12:36]

Alright, damn floating point precision. But you could generate numbers form 0 to
10 and, then divide them by 10. Alternatively, you could write the loop like
that:

  for (let opacity = 0; opacity <= 1; opacity = (opacity * 10 + 1) / 10)

[kzar, 2016/01/26 17:35:09]

Yea, I thought about those options but in the end figured that just hard coding
the values was easier.

The ideal solution IMHO would be something like `for (let opacity of
uniques(frameOpacities)) {` but the creating the uniques function would then be
more complexity...

[Sebastian Noack, 2016/01/26 18:10:34]

Well, instead a custom unique() function, we should rather use a Set. But we
would need a polyfill for Chrome <39 and Safari <7.1. With our current
requirements a polyfill could be as simple as:

function Set(iterable)
{
  this._map = Object.create(null);
  if (iterable)
    for (let value of iterable)
      this.add(value);
}
Set.prototype = {
  add: function(value)
  {
    this._map[JSON.stringify(value)] = null;
  },
  values: function()
  {
    return Object.getOwnPropertyNames(this._map).map(JSON.parse);
  }
}

But I'm still undecided whether this would be reasonable as long as this is the
only use case.

However, I'm opposed to hard-coding an array of 11 values in a for-of loop. Also
keep in mind what piece of crap jsHydra will generate there.

[kzar, 2016/01/26 18:46:34]

Fair enough, how about this?

+        {
+          context.globalAlpha = 1;
+          context.drawImage(images[size]["base"][whitelisted | 0], 0, 0);
+          context.globalAlpha = opacity;
+          context.drawImage(images[size]["overlay"], 0, 0);
+          frames["" + opacity +

[Sebastian Noack, 2016/01/25 15:44:44]

Following data structure would be preferable:

  frames[keyBasedOnOpacityAndWhitelisted] = {19: [..], 38: [..]}

That way we have objects that can be passed as-is to
chrome.browserAction.setIcon(), keeping the code performed during the animation
to a minimum.

[kzar, 2016/01/26 15:01:41]

Good point, Done.

+                 whitelisted + size] = context.getImageData(0, 0, size, size);
+        }
+      }
+    }
+
+    return frames;
+  });
 }
 
-function runAnimation()
+function runAnimation(animationType)

[Sebastian Noack, 2016/01/25 15:44:45]

Any reason you changed the semantic from notificationType to animationType? The
icon indicates whether there is an important or normal notification, not whether
the animation is important or not.

[kzar, 2016/01/26 15:01:41]

Done.

 {
-  return setInterval(function()
+  let frames = !safariPlatform && renderFrames(animationType);

[Sebastian Noack, 2016/01/25 15:44:44]

I'd rather wait for the promise to be fulfilled before starting the animation,
instead adding the overhead of adding a callback on every frame.

[kzar, 2016/01/26 15:01:41]

Good point, Done.

+  let frameInterval;
+
+  function playAnimation()
   {
-    ext.pages.query({active: true}, function(pages)
+    animationPlaying = true;
+    let animationStep = 0;
+    ext.pages.query({active: true}, pages =>
     {
-      let fadeInInterval = setInterval(function()
+      function appendActivePage(page)
       {
-        animationStep++;
-        pages.forEach(setIcon);
+        pages.push(page);
+      }
+      ext.pages.onActivated.addListener(appendActivePage);

[Sebastian Noack, 2016/01/25 15:44:43]

It seems that you only implemented ext.pages.onActivated for Chrome. Any chance
this code i reached on Safari? And if not, why not simply calling
chrome.tabs.onActivated directly?

Also what happened with the case when a tab is removed while the animation runs,
causing errors to be logged?

[kzar, 2016/01/26 15:01:42]

I have implemented it for Safari too, check safari/ext/background.js . (Please
could you to check the Safari implementation as I'm not sure I've got it right.)

As for the logged errors when a tab is removed, I fixed that in Patch Set 11.

 
-        if (animationStep < numberOfFrames)
-          return;
+      frameInterval = setInterval(() =>
+      {
+        let opacity = frameOpacities[animationStep++];
+        pages.forEach(page => {
+          if (whitelistedState.has(page))
+            setIcon(page, animationType, opacity, frames);
+        });
 
-        setTimeout(function()
+        if (animationStep > numberOfFrames)
         {
-          let fadeOutInterval = setInterval(function()
-          {
-            animationStep--;
-            pages.forEach(setIcon);
-
-            if (animationStep > 0)
-              return;
-
-            clearInterval(fadeOutInterval);
-          }, 100);
-        },1000);
-
-        clearInterval(fadeInInterval);
+          animationStep = 0;
+          clearInterval(frameInterval);
+          animationPlaying = false;
+          ext.pages.onActivated.removeListener(appendActivePage);
+        }
       }, 100);
     });
-  }, 10000);
+  }
+
+  playAnimation();
+  let animationInterval = setInterval(playAnimation, 10000);
+  return () =>
+  {
+    clearInterval(frameInterval);

[Sebastian Noack, 2016/01/25 15:44:43]

I think the previous logic, using global variables rather than passing around
functions was either to understand.

[kzar, 2016/01/26 15:01:42]

I disagree there.

[Sebastian Noack, 2016/01/26 16:12:35]

It's not that we return a function with different logic deepening on some state,
in which case functional programming might make sense. The calling code isn't
even completely agnostic of the state as it still has to check whether the
stopAnimation is set, which is a global variable as well. And since we are in a
module here it doesn't matter whether we have 1 or n global variables.

[kzar, 2016/01/26 17:35:09]

I just think that having as much of the animation state encapsulated as possible
is good. I find it easier to reason about code that uses less global state. Also
during working on this code I sometimes found edge cases where two animations
would run at once and the shared global state got in a mess, resulting in
flickering and other glitchy behaviour.

[Sebastian Noack, 2016/01/26 18:10:34]

You don't have any less global state. But the global variables are now just
attached to the closure of this function rather than the closure of the module.
You still have only one global variable, all state depends on, so if two
animations would run at the same time you would still end up in a mess. Even
worse, the function top stop the first animation will be gone and never be
called.

[kzar, 2016/01/26 18:46:34]

Fair enough, Done.

+    clearInterval(animationInterval);
+    animationPlaying = false;
+  };
 }
 
 /**
@@ -95,8 +180,9 @@ function runAnimation()
  */
 exports.updateIcon = function(page, whitelisted)
 {
-  page.browserAction.setIcon(getIconPath(whitelisted));
   whitelistedState.set(page, whitelisted);
+  if (!animationPlaying)
+    setIcon(page);
 };
 
 /**
@@ -106,10 +192,8 @@ exports.updateIcon = function(page, whitelisted)
  */
 exports.startIconAnimation = function(type)
 {
-  notificationType = type;
-
-  if (animationInterval == null)
-    animationInterval = runAnimation();
+  stopAnimation && stopAnimation();
+  stopAnimation = runAnimation(type);
 };
 
 /**
@@ -117,11 +201,5 @@ exports.startIconAnimation = function(type)
  */
 exports.stopIconAnimation = function()
 {
-  if (animationInterval != null)
-  {
-    clearInterval(animationInterval);
-    animationInterval = null;
-  }
-
-  notificationType  = null;
+  stopAnimation && stopAnimation();
 };