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
@@ skipping 13 matching lines @@
                          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 frameInterval = null;
 let animationInterval = null;
 let whitelistedState = new ext.PageMap();
 
 function loadImage(url)
 {
-  return new Promise((resolve, reject) => {
+  return new Promise((resolve, reject) =>
+  {
     let image = new Image();
     image.src = url;
     image.addEventListener("load", () =>

[Sebastian Noack, 2016/01/26 22:35:15]

Nit: The indentation of the block below is inconsistent with the arrow functions
in the surrounding code.

[kzar, 2016/01/26 22:59:31]

Done.

     {
       resolve(image);
     });
-    image.addEventListener("error", () => {
+    image.addEventListener("error", () =>
+    {
       reject("Failed to load image " + url);
     });
   });
 };
 
 function setIcon(page, notificationType, opacity, frames)
 {
   opacity = opacity || 0;
   let whitelisted = !!whitelistedState.get(page) && !safariPlatform;
 
   if (!notificationType || !frames)
   {
-    if (notificationType && opacity > 0.5)

[Sebastian Noack, 2016/01/26 22:35:17]

The check for notificationType seems to be redundant. If we are not in an icon
animation opacity should be 0 anyway.

[kzar, 2016/01/26 22:59:32]

Done.

+    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]
-      });

[Sebastian Noack, 2016/01/26 22:35:15]

Nit: You should close the braces on the same depth of indentation you opened it.

[kzar, 2016/01/26 22:59:32]

Done.

+    });
   }
 }
 
 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 = {

[Sebastian Noack, 2016/01/26 22:35:15]

Nit: You declare a variable that is already defined here.

[kzar, 2016/01/26 22:59:33]

I think it's pretty readable in this instance.

       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])
     {
@@ skipping 27 matching lines @@
     {
       function appendActivePage(page)
       {
         pages.push(page);
       }
       ext.pages.onActivated.addListener(appendActivePage);
 
       frameInterval = setInterval(() =>
       {
         let opacity = frameOpacities[animationStep++];
-        pages.forEach(page => {

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

Nit: Why not a for-of loop?

[kzar, 2016/01/26 22:59:33]

Done.

+        for (let page of pages)
+        {
           if (whitelistedState.has(page))
             setIcon(page, notificationType, opacity, frames);
-        });
+        };
 
         if (animationStep > numberOfFrames)
         {
           animationStep = 0;
           clearInterval(frameInterval);
           frameInterval = null;
           ext.pages.onActivated.removeListener(appendActivePage);
         }
       }, 100);
     });
   }
 
   renderFrames(notificationType).then(frames =>
   {
     playAnimation(frames);
-    animationInterval = setInterval(() => { playAnimation(frames); },
-                                    10000);

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

Nit: Wrapping the line here doesn't seem to be necessary to stay under 80 chars
per line, but certainly looks weird.

[kzar, 2016/01/26 22:59:31]

Done.

+    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
  */
@@ skipping 20 matching lines @@
 /**
  * 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)
 {
   stopIconAnimation();
   runAnimation(type);
 };