Bug 1397196 - Fix pageAction icon loading when an extension has a cached browserAction theme-based icon. r=mixedpuppy
authorLuca Greco <lgreco@mozilla.com>
Wed, 06 Sep 2017 12:40:45 +0200
changeset 430215 39b0e0aead7b3c19c2492f6344045bf2343bf165
parent 430117 8b156b6abe6137e1d0e0c97f848a7b5d0058530f
child 430216 065ea3d56a5274db71a9c50f8876bc24f293e0f5
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmixedpuppy
bugs1397196
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1397196 - Fix pageAction icon loading when an extension has a cached browserAction theme-based icon. r=mixedpuppy 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;
     }