Issue 7253 - Pre-render icons for badge on Chromium

lib/icon.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 35 matching lines @@
     });
     image.addEventListener("error", () =>
     {
       reject("Failed to load image " + url);
     });
   });
 }
 
 function renderIcons()
 {
-  Promise.all([
-    loadImage("icons/abp-16.png"),
-    loadImage("icons/abp-16-whitelisted.png"),
-    loadImage("icons/abp-32.png"),
-    loadImage("icons/abp-32-whitelisted.png")
-  ])
-  .then(images =>
-  {
-    let imageMap = {
-      16: [images[0], images[1]],
-      32: [images[2], images[3]]
-    };
-
-    let canvas = document.createElement("canvas");
-    let context = canvas.getContext("2d");
-
-    for (let whitelisted = 0; whitelisted <= 1; whitelisted++)
-    {
-      let imageData = {};
-      let sizes = [16, 32];

[Sebastian Noack, 2019/02/03 08:47:16]

Not: This can be inlined below.

[Manish Jethani, 2019/02/03 09:09:35]

Do you mean unroll the loop?

[Manish Jethani, 2019/02/03 09:50:12]

Never mind, I have rewritten the function.

[Sebastian Noack, 2019/02/03 10:07:53]

Not unrolling it but avoiding the temporary variable writing "for (let size of
[16, 32])" instead. FWIW, the reason the original implementation didn't do that
was how jsHydra (a fairly limited transpiler we used at the time) converted
for..of loops. BTW, I meant "Not", not "Not".

[Manish Jethani, 2019/02/03 10:37:55]

I see, I remember JSHydra.
I see you meant "Not" :D

-
-      for (let size of sizes)
-      {
-        canvas.width = size;
-        canvas.height = size;
-        context.globalAlpha = 1;
-        context.drawImage(imageMap[size][whitelisted], 0, 0);
-        imageData[size] = context.getImageData(0, 0, size, size);
-      }
-
-      icons[whitelisted] = imageData;
-    }
-  });
+  let paths = [
+    "icons/abp-16.png", "icons/abp-16-whitelisted.png",
+    "icons/abp-32.png", "icons/abp-32-whitelisted.png"
+  ];
+
+  for (let path of paths)
+  {
+    loadImage(path).then(image =>
+    {
+      let [, size, whitelisted] = /\/abp-(16|32)(-whitelisted)?\./.exec(path);

[Manish Jethani, 2019/02/03 09:47:55]

Note: size is a string, not a number, but that's fine because JS APIs handle it
well.

+
+      let canvas = document.createElement("canvas");
+      let context = canvas.getContext("2d");
+      let imageData = icons[!!whitelisted | 0] || {};

[Sebastian Noack, 2019/02/03 10:07:54]

Nit: Isn't coercing to boolean (through !!) redundant as you already coerce the
value to a number (with |0)?

[Manish Jethani, 2019/02/03 10:37:55]

`"-whitelisted" | 0` is 0. Because "-whitelisted" as a number is NaN, and `NaN |
0` is 0.

+
+      canvas.width = size;
+      canvas.height = size;
+      context.globalAlpha = 1;
+      context.drawImage(image, 0, 0);
+      imageData[size] = context.getImageData(0, 0, size, size);
+
+      icons[!!whitelisted | 0] = imageData;
+    });
+  }
 }
 
 function setIcon(page, notificationType, opacity, frames)
 {
   opacity = opacity || 0;
   let whitelisted = !!whitelistedState.get(page);
 
   if (!notificationType || !frames)
   {
     if (opacity > 0.5)
     {
-      page.browserAction.setIcon("/icons/abp-$size-notification-" +
-                                 notificationType + ".png");
+      page.browserAction.setIconPath("/icons/abp-$size-notification-" +
+                                     notificationType + ".png");
+    }
+    else if (icons[whitelisted | 0])
+    {
+      page.browserAction.setIconImageData(icons[whitelisted | 0]);
     }
     else
     {
-      page.browserAction.setIcon("/icons/abp-$size" +
-                                 (whitelisted ? "-whitelisted" : "") + ".png",
-                                 icons[whitelisted | 0]);
+      page.browserAction.setIconPath("/icons/abp-$size" +
+                                     (whitelisted ? "-whitelisted" : "") + ".png");
     }
   }
   else
   {
-    browser.browserAction.setIcon({
+    browser.browserAction.setIconPath({
       tabId: page.id,
       imageData: frames["" + opacity + whitelisted]
     });
   }
 }
 
 filterNotifier.on("page.WhitelistingStateRevalidate", (page, filter) =>
 {
   whitelistedState.set(page, !!filter);
   if (canUpdateIcon)
@@ skipping 26 matching lines @@
 
     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 imageData = {};
         let sizes = [16, 20, 32, 40];

[Sebastian Noack, 2019/02/03 08:47:16]

Not: While at it, same here.

         for (let size of sizes)
         {
           canvas.width = size;
           canvas.height = size;
           context.globalAlpha = 1;
           context.drawImage(imageMap[size]["base"][whitelisted | 0], 0, 0);
           context.globalAlpha = opacity;
           context.drawImage(imageMap[size]["overlay"], 0, 0);
           imageData[size] = context.getImageData(0, 0, size, size);
         }
@@ skipping 97 matching lines @@
 
         animateIcon(type, frames);
       }, 10000);
     });
   });
 };
 
 // Pre-render icons on Chromium (#7253).
 if (info.platform == "chromium")
   renderIcons();