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, 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} url  URL string of the image to load
- * @return {Promise}
- */
-exports.loadImageAsCanvas = function(url)
+function loadImage(url)
 {
-  return new Promise((resolve, reject) => {
+  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);
+      resolve(image);
     });
-    image.addEventListener("error", () => {
+    image.addEventListener("error", () =>
+    {
       reject("Failed to load image " + url);
     });
   });
 };
 
-function setIcon(page, animationType, opacity, frames)
+function setIcon(page, notificationType, 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.

+  let whitelisted = !!whitelistedState.get(page) && !safariPlatform;
 
-  if (!animationType || !frames)
+  if (!notificationType || !frames)
   {
-    if (animationType && opacity > 0.5)
+    if (opacity > 0.5)
       page.browserAction.setIcon("icons/abp-$size-notification-"
-                                 + animationType + ".png");
+                                 + notificationType + ".png");
     else
       page.browserAction.setIcon("icons/abp-$size" +
-                                 (greyed ? "-whitelisted" : "") + ".png");
+                                 (whitelisted ? "-whitelisted" : "") + ".png");
   }
   else
   {
-    frames.then(frames =>
-    {
-      chrome.browserAction.setIcon({
-        tabId: page._id,
-        imageData: {19: frames["" + opacity + greyed + 19],
-                    38: frames["" + opacity + greyed + 38]}
-      });
+    chrome.browserAction.setIcon({
+      tabId: page._id,
+      imageData: frames["" + opacity + whitelisted]
     });
   }
 }
 
-function renderFrames(animationType)
+function renderFrames(notificationType)
 {
+  if (safariPlatform)
+    return Promise.resolve(null);
+
   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"),
+    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] },

[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])
+    for (let whitelisted of [false, true])
     {
-      canvas.width = size;
-      canvas.height = size;
-      for (let whitelisted of [false, true])
+      for (let i = 0, opacity = 0; i <= 10; opacity = ++i / 10)
       {
-        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?

+        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);
-          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);
+          imageData[size] = context.getImageData(0, 0, size, size);
         }
+        frames["" + opacity + whitelisted] = imageData;
       }
     }
 
     return frames;
   });
 }
 
-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.

+function runAnimation(notificationType)
 {
-  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()
+  function playAnimation(frames)
   {
-    animationPlaying = true;
     let animationStep = 0;
     ext.pages.query({active: true}, pages =>
     {
       function appendActivePage(page)
       {
         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.

 
       frameInterval = setInterval(() =>
       {
         let opacity = frameOpacities[animationStep++];
-        pages.forEach(page => {
+        for (let page of pages)
+        {
           if (whitelistedState.has(page))
-            setIcon(page, animationType, opacity, frames);
-        });
+            setIcon(page, notificationType, opacity, frames);
+        };
 
         if (animationStep > numberOfFrames)
         {
           animationStep = 0;
           clearInterval(frameInterval);
-          animationPlaying = false;
+          frameInterval = null;
           ext.pages.onActivated.removeListener(appendActivePage);
         }
       }, 100);
     });
   }
 
-  playAnimation();
-  let animationInterval = setInterval(playAnimation, 10000);
-  return () =>
+  renderFrames(notificationType).then(frames =>
   {
-    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;
-  };
+    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();
-};