Bug 1446250: Part 2 - Optimize/reduce calls into the Photon PageAction API. r?mixedpuppy draft
authorKris Maglione <maglione.k@gmail.com>
Thu, 15 Mar 2018 20:20:33 -0700
changeset 768852 1472efc96f42a89d0cea20e6b0a9dd00a8bfdb78
parent 768851 38731c468ecbbca45e5c78b2d4b2dc465cf90f0d
push id102991
push usermaglione.k@gmail.com
push dateFri, 16 Mar 2018 22:41:24 +0000
reviewersmixedpuppy
bugs1446250
milestone61.0a1
Bug 1446250: Part 2 - Optimize/reduce calls into the Photon PageAction API. r?mixedpuppy Calling into the Photon PageAction API to update a property is orders of magnitude more expensive than the simple DOM-based updates we used previously. To make matters worse, a lot of our caching was removed during the migration, and the Photon API introduces a lot of duplicated work when selecting icons. This patch caches the last known state for each property to avoid calling into the Photon APIs to update each property more than necessary, and removes the extraneous preferred icon size calculations that the Photon code already duplicates. MozReview-Commit-ID: LjPPxolmcd6
browser/components/extensions/ext-pageAction.js
toolkit/components/extensions/ExtensionParent.jsm
--- a/browser/components/extensions/ext-pageAction.js
+++ b/browser/components/extensions/ext-pageAction.js
@@ -80,24 +80,26 @@ this.pageAction = class extends Extensio
                                      extension);
 
     this.tabContext.on("location-change", this.handleLocationChange.bind(this)); // eslint-disable-line mozilla/balanced-listeners
 
     pageActionMap.set(extension, this);
 
     this.defaults.icon = await StartupCache.get(
       extension, ["pageAction", "default_icon"],
-      () => IconDetails.normalize({path: options.default_icon}, extension));
+      () => this.normalize({path: options.default_icon}));
+
+    this.lastValues = new DefaultWeakMap(() => ({}));
 
     if (!this.browserPageAction) {
       this.browserPageAction = PageActions.addAction(new PageActions.Action({
         id: widgetId,
         extensionID: extension.id,
         title: this.defaults.title,
-        iconURL: this.getIconData(this.defaults.icon),
+        iconURL: this.defaults.icon,
         pinnedToUrlbar: true,
         disabled: !this.defaults.show,
         onCommand: (event, buttonNode) => {
           this.handleClick(event.target.ownerGlobal);
         },
         onBeforePlacedInWindow: browserWindow => {
           if (this.extension.hasPermission("menus") ||
               this.extension.hasPermission("contextMenus")) {
@@ -155,40 +157,54 @@ this.pageAction = class extends Extensio
       delete this.tabContext.get(tab)[prop];
     }
 
     if (tab.selected) {
       this.updateButton(tab.ownerGlobal);
     }
   }
 
+  normalize(details, context = null) {
+    let icon = IconDetails.normalize(details, this.extension, context);
+    if (!Object.keys(icon).length) {
+      icon = null;
+    }
+    return icon;
+  }
+
   // Updates the page action button in the given window to reflect the
   // properties of the currently selected tab:
   //
   // Updates "tooltiptext" and "aria-label" to match "title" property.
   // Updates "image" to match the "icon" property.
   // Enables or disables the icon, based on the "show" and "patternMatching" properties.
   updateButton(window) {
     let tab = window.gBrowser.selectedTab;
     let tabData = this.tabContext.get(tab);
-    let title = tabData.title || this.extension.name;
-    this.browserPageAction.setTitle(title, window);
-    this.browserPageAction.setTooltip(title, window);
+    let last = this.lastValues.get(window);
 
-    // At least one of "show" or "patternMatching" must be defined here.
-    let {show = tabData.patternMatching} = tabData;
-    this.browserPageAction.setDisabled(!show, window);
+    window.requestAnimationFrame(() => {
+      let title = tabData.title || this.extension.name;
+      if (last.title !== title) {
+        this.browserPageAction.setTitle(title, window);
+        last.title = title;
+      }
 
-    let iconURL;
-    if (typeof(tabData.icon) == "string") {
-      iconURL = IconDetails.escapeUrl(tabData.icon);
-    } else {
-      iconURL = this.getIconData(tabData.icon);
-    }
-    this.browserPageAction.setIconURL(iconURL, window);
+      let show = tabData.show != null ? tabData.show : tabData.patternMatching;
+      if (last.show !== show) {
+        this.browserPageAction.setDisabled(!show, window);
+        last.show = show;
+      }
+
+      let icon = tabData.icon;
+      if (last.icon !== icon) {
+        this.browserPageAction.setIconURL(icon, window);
+        last.icon = icon;
+      }
+    });
   }
 
   // Checks whether the tab action is shown when the specified tab becomes active.
   // Does pattern matching if necessary, and caches the result as a tab-specific value.
   // @param {XULElement} tab
   //        The tab to be checked
   // @param [optional] {boolean} ignoreCache
   //        If true, forces pattern matching to be reevaluated, even if there is a cached value.
@@ -203,28 +219,16 @@ this.pageAction = class extends Extensio
     // Otherwise pattern matching must have been configured. Do it, caching the result.
     if (ignoreCache || tabData.patternMatching === undefined) {
       let uri = tab.linkedBrowser.currentURI;
       tabData.patternMatching = tabData.showMatches.matches(uri) && !tabData.hideMatches.matches(uri);
     }
     return tabData.patternMatching;
   }
 
-  getIconData(icons) {
-    let getIcon = size => {
-      let {icon} = IconDetails.getPreferredIcon(icons, this.extension, size);
-      // TODO: implement theme based icon for pageAction (Bug 1398156)
-      return IconDetails.escapeUrl(icon);
-    };
-    return {
-      "16": getIcon(16),
-      "32": getIcon(32),
-    };
-  }
-
   /**
    * Triggers this page action for the given window, with the same effects as
    * if it were clicked by a user.
    *
    * This has no effect if the page action is hidden for the selected tab.
    *
    * @param {Window} window
    */
@@ -344,20 +348,18 @@ this.pageAction = class extends Extensio
 
           let title = pageAction.getProperty(tab, "title");
           return Promise.resolve(title);
         },
 
         setIcon(details) {
           let tab = tabTracker.getTab(details.tabId);
 
-          let icon = IconDetails.normalize(details, extension, context);
-          if (!Object.keys(icon).length) {
-            icon = null;
-          }
+          let icon = pageAction.normalize(details, context);
+
           pageAction.setProperty(tab, "icon", icon);
         },
 
         setPopup(details) {
           let tab = tabTracker.getTab(details.tabId);
 
           // Note: Chrome resolves arguments to setIcon relative to the calling
           // context, but resolves arguments to setPopup relative to the extension
--- a/toolkit/components/extensions/ExtensionParent.jsm
+++ b/toolkit/components/extensions/ExtensionParent.jsm
@@ -1284,16 +1284,18 @@ function watchExtensionProxyContextLoad(
   return () => {
     extension.off("extension-proxy-context-load", listener);
   };
 }
 
 // Manages icon details for toolbar buttons in the |pageAction| and
 // |browserAction| APIs.
 let IconDetails = {
+  DEFAULT_ICON: "chrome://browser/content/extension.svg",
+
   // 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.
   //
@@ -1356,17 +1358,17 @@ let IconDetails = {
             url = baseURI.resolve(path[size]);
 
             // The Chrome documentation specifies these parameters as
             // relative paths. We currently accept absolute URLs as well,
             // which means we need to check that the extension is allowed
             // to load them. This will throw an error if it's not allowed.
             this._checkURL(url, extension);
           }
-          result[size] = url;
+          result[size] = url || this.DEFAULT_ICON;
         }
       }
 
       if (themeIcons) {
         themeIcons.forEach(({size, light, dark}) => {
           let lightURL = baseURI.resolve(light);
           let darkURL = baseURI.resolve(dark);