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-19.png"),
-    loadImage("icons/abp-19-whitelisted.png"),
-    loadImage("icons/abp-20.png"),
-    loadImage("icons/abp-20-whitelisted.png"),
-    loadImage("icons/abp-32.png"),
-    loadImage("icons/abp-32-whitelisted.png"),
-    loadImage("icons/abp-38.png"),
-    loadImage("icons/abp-38-whitelisted.png"),
-    loadImage("icons/abp-40.png"),
-    loadImage("icons/abp-40-whitelisted.png"),

[Sebastian Noack, 2019/02/03 06:43:18]

The icons in the sizes 20 and 40 are only used on Microsoft Edge, hence we don't
need to pretender them if we only use this code path on Chromium.

The sizes 19 and 38 were used by older Chromium versions. We might no longer
need them anymore, but please double check.

[Manish Jethani, 2019/02/03 07:26:49]

Done.
I found the bug that led to support for arbitrary sizes:

https://bugs.chromium.org/p/chromium/issues/detail?id=564926

I think this probably made it into Chrome 49, but I'm not sure. I have removed
19x19 and 38x38 in this patch. QA will test on Chrome 49 anyway (I'll make a
note in the Trac ticket), and if it gives problems we can add those sizes back.

Done.

[Sebastian Noack, 2019/02/03 07:45:15]

I'm certain that it will work on Chrome 49. But Chrome changed the UI at some
point, rendering the browser action icons in 16px instead of 19px (respectively
32px and 38px on high-res displays). The worst that could happen (if some
supported versions are still rendering the browser action icon in 19px/38px) is
that the icon will look blurry/pixelated.

Also if these sizes are in fact no longer relevant we should remove them
completely (not only from this code path), as Firefox won't use them either.

[Manish Jethani, 2019/02/03 08:00:02]

Alright, let's test have QA test it out on Chrome 49. I'm personally OK with it
appearing blurred on really old versions of Chrome if it means better
performance on the newer versions.
Let's make this a separate issue? If QA finds that the icons looks OK on Chrome
49, we can make this change for the next release.

-  ])
-  .then(images =>
-  {
-    let imageMap = {
-      16: [images[0], images[1]],
-      19: [images[2], images[3]],
-      20: [images[4], images[5]],
-      32: [images[6], images[7]],
-      38: [images[8], images[9]],
-      40: [images[10], images[11]]
-    };
-
-    let canvas = document.createElement("canvas");
-    let context = canvas.getContext("2d");
-
-    for (let whitelisted of [false, true])
-    {
-      let imageData = {};
-      let sizes = [16, 19, 20, 32, 38, 40];
-
-      for (let size of sizes)
-      {
-        canvas.width = size;
-        canvas.height = size;
-        context.globalAlpha = 1;
-        context.drawImage(imageMap[size][whitelisted | 0], 0, 0);
-        imageData[size] = context.getImageData(0, 0, size, size);
-      }
-
-      icons[whitelisted | 0] = 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)
     setIcon(page);
 });
 
 function renderFrames(notificationType)
 {
   return Promise.all([
     loadImage("icons/abp-16.png"),
     loadImage("icons/abp-16-whitelisted.png"),
     loadImage("icons/abp-16-notification-" + notificationType + ".png"),
-    loadImage("icons/abp-19.png"),
-    loadImage("icons/abp-19-whitelisted.png"),
-    loadImage("icons/abp-19-notification-" + notificationType + ".png"),
     loadImage("icons/abp-20.png"),
     loadImage("icons/abp-20-whitelisted.png"),
     loadImage("icons/abp-20-notification-" + notificationType + ".png"),
     loadImage("icons/abp-32.png"),
     loadImage("icons/abp-32-whitelisted.png"),
     loadImage("icons/abp-32-notification-" + notificationType + ".png"),
-    loadImage("icons/abp-38.png"),
-    loadImage("icons/abp-38-whitelisted.png"),
-    loadImage("icons/abp-38-notification-" + notificationType + ".png"),
     loadImage("icons/abp-40.png"),
     loadImage("icons/abp-40-whitelisted.png"),
     loadImage("icons/abp-40-notification-" + notificationType + ".png")
   ]).then(images =>
   {
     let imageMap = {
       16: {base: [images[0], images[1]], overlay: images[2]},
-      19: {base: [images[3], images[4]], overlay: images[5]},
-      20: {base: [images[6], images[7]], overlay: images[8]},
-      32: {base: [images[9], images[10]], overlay: images[11]},
-      38: {base: [images[12], images[13]], overlay: images[14]},
-      40: {base: [images[15], images[16]], overlay: images[17]}
+      20: {base: [images[3], images[4]], overlay: images[5]},
+      32: {base: [images[6], images[7]], overlay: images[8]},
+      40: {base: [images[9], images[10]], overlay: images[11]}
     };
 
     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, 19, 20, 32, 38, 40];
+        let sizes = [16, 20, 32, 40];
         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();