Bug 1397196 - Fix pageAction icon loading when an extension has a cached browserAction theme-based icon. draft
authorLuca Greco <lgreco@mozilla.com>
Wed, 06 Sep 2017 12:40:45 +0200
changeset 663895 a78e6ecdd3c2a7c1abe507255845a0cc90ee6311
parent 663831 1888ec2f277f6bb26271b8808e08914a21db9efe
child 731312 27dda0375fe3f2cfce25eac55ea6746f14a6cb75
push id79555
push userluca.greco@alcacoop.it
push dateWed, 13 Sep 2017 13:38:02 +0000
bugs1397196
milestone57.0a1
Bug 1397196 - Fix pageAction icon loading when an extension has a cached browserAction theme-based icon. MozReview-Commit-ID: Lmi5pLerzul
browser/components/extensions/ext-browserAction.js
browser/components/extensions/ext-pageAction.js
browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js
toolkit/components/extensions/ExtensionParent.jsm
--- a/browser/components/extensions/ext-browserAction.js
+++ b/browser/components/extensions/ext-browserAction.js
@@ -100,16 +100,17 @@ this.browserAction = class extends Exten
     }
 
     browserActionMap.set(extension, this);
 
     this.defaults.icon = await StartupCache.get(
       extension, ["browserAction", "default_icon"],
       () => IconDetails.normalize({
         path: options.default_icon,
+        iconType: "browserAction",
         themeIcons: options.theme_icons,
       }, extension));
 
     this.iconData.set(
       this.defaults.icon,
       await StartupCache.get(
         extension, ["browserAction", "default_icon_data"],
         () => this.getIconData(this.defaults.icon)));
@@ -617,16 +618,18 @@ this.browserAction = class extends Exten
 
           let title = browserAction.getProperty(tab, "title");
           return Promise.resolve(title);
         },
 
         setIcon: function(details) {
           let tab = getTab(details.tabId);
 
+          details.iconType = "browserAction";
+
           let icon = IconDetails.normalize(details, extension, context);
           browserAction.setProperty(tab, "icon", icon);
         },
 
         setBadgeText: function(details) {
           let tab = getTab(details.tabId);
 
           browserAction.setProperty(tab, "badgeText", details.text);
--- a/browser/components/extensions/ext-pageAction.js
+++ b/browser/components/extensions/ext-pageAction.js
@@ -143,22 +143,21 @@ this.pageAction = class extends Extensio
         button.setAttribute("style", style);
       }
 
       button.hidden = !tabData.show;
     });
   }
 
   getIconData(icons) {
-    // These URLs should already be properly escaped, but make doubly sure CSS
-    // string escape characters are escaped here, since they could lead to a
-    // sandbox break.
-    let escape = str => str.replace(/[\\\s"]/g, encodeURIComponent);
-
-    let getIcon = size => escape(IconDetails.getPreferredIcon(icons, this.extension, size).icon);
+    let getIcon = size => {
+      let {icon} = IconDetails.getPreferredIcon(icons, this.extension, size);
+      // TODO: implement theme based icon for pageAction (Bug 1398156)
+      return IconDetails.escapeUrl(icon);
+    };
 
     let style = `
       --webextension-urlbar-image: url("${getIcon(16)}");
       --webextension-urlbar-image-2x: url("${getIcon(32)}");
     `;
 
     return {style};
   }
--- a/browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js
+++ b/browser/components/extensions/test/browser/browser_ext_browserAction_pageAction_icon.js
@@ -292,8 +292,80 @@ add_task(async function testDetailsObjec
 
     if (!test.menuResolutions) {
       continue;
     }
   }
 
   await extension.unload();
 });
+
+// NOTE: The current goal of this test is ensuring that Bug 1397196 has been fixed,
+// and so this test extension manifest has a browser action which specify
+// a theme based icon and a pageAction, both the pageAction and the browserAction
+// have a common default_icon.
+//
+// Once Bug 1398156 will be fixed, this test should be converted into testing that
+// the browserAction and pageAction themed icons (as well as any other cached icon,
+// e.g. the sidebar and devtools panel icons) can be specified in the same extension
+// and do not conflict with each other.
+//
+// This test currently fails without the related fix, but only if the browserAction
+// API has been already loaded before the pageAction, otherwise the icons will be cached
+// in the opposite order and the test is not able to reproduce the issue anymore.
+add_task(async function testPageActionIconLoadingOnBrowserActionThemedIcon() {
+  async function background() {
+    const tabs = await browser.tabs.query({active: true});
+    await browser.pageAction.show(tabs[0].id);
+
+    browser.test.sendMessage("ready");
+  }
+
+  const extension = ExtensionTestUtils.loadExtension({
+    background,
+    manifest: {
+      "name": "Foo Extension",
+
+      "browser_action": {
+        "default_icon": "common_cached_icon.png",
+        "default_popup": "default_popup.html",
+        "default_title": "BrowserAction title",
+        "theme_icons": [
+          {
+            "dark": "1.png",
+            "light": "2.png",
+            "size": 16
+          }
+        ],
+      },
+
+      "page_action": {
+        "default_icon": "common_cached_icon.png",
+        "default_popup": "default_popup.html",
+        "default_title": "PageAction title",
+      },
+
+      "permissions": ["tabs"],
+    },
+
+    "files": {
+      "common_cached_icon.png": imageBuffer,
+      "1.png": imageBuffer,
+      "2.png": imageBuffer,
+      "default_popup.html": "<!DOCTYPE html><html><body>popup</body></html>",
+    },
+  });
+
+  await extension.startup();
+
+  await extension.awaitMessage("ready");
+
+  await promiseAnimationFrame();
+
+  let pageActionId = `${makeWidgetId(extension.id)}-page-action`;
+  let pageActionImage = document.getElementById(pageActionId);
+
+  const iconURL = new URL(getListStyleImage(pageActionImage));
+
+  is(iconURL.pathname, "/common_cached_icon.png", "Got the expected pageAction icon url");
+
+  await extension.unload();
+});
--- a/toolkit/components/extensions/ExtensionParent.jsm
+++ b/toolkit/components/extensions/ExtensionParent.jsm
@@ -1219,18 +1219,20 @@ function extensionNameFromURI(uri) {
     }
   }
   return GlobalManager.getExtension(id).name;
 }
 
 // Manages icon details for toolbar buttons in the |pageAction| and
 // |browserAction| APIs.
 let IconDetails = {
-  // WeakMap<Extension -> Map<url-string -> object>>
-  iconCache: new DefaultWeakMap(() => new DefaultMap(() => new Map())),
+  // WeakMap<Extension -> Map<url-string -> Map<iconType-string -> object>>>
+  iconCache: new DefaultWeakMap(() => {
+    return new DefaultMap(() => new DefaultMap(() => new Map()));
+  }),
 
   // Normalizes the various acceptable input formats into an object
   // with icon size as key and icon URL as value.
   //
   // If a context is specified (function is called from an extension):
   // Throws an error if an invalid icon size was provided or the
   // extension is not allowed to load the specified resources.
   //
@@ -1241,17 +1243,18 @@ let IconDetails = {
       // Pick a cache key for the icon paths. If the path is a string,
       // use it directly. Otherwise, stringify the path object.
       let key = details.path;
       if (typeof key !== "string") {
         key = uneval(key);
       }
 
       let icons = this.iconCache.get(extension)
-                      .get(context && context.uri.spec);
+                      .get(context && context.uri.spec)
+                      .get(details.iconType);
 
       let icon = icons.get(key);
       if (!icon) {
         icon = this._normalize(details, extension, context);
         icons.set(key, icon);
       }
       return icon;
     }