Bug 1446250: Part 1 - Optimize Photon PageAction update performance. r=Gijs

The amount of computational complexity and garbage array/string/object
generation for each update to a pageAction property went up astronomically
with the migration of WebExtension page actions to the Photon API. This
resulted in non-trivial talos regression when Screenshots attempted to switch
back to the built-in pageAction API.

These changes fix most of the garbage generation, and reduce a lot of the
duplicated work for each update.

MozReview-Commit-ID: 4uPLnAesdU2

--HG--
extra : rebase_source : 3f723f3f35abf032cf12e02ce38552e21ea4827f

browser/base/content/browser-pageActions.js

@@ -462,28 +462,36 @@ var BrowserPageActions = {
    * @param  propertyName (string, optional)
    *         The name of the property to update.  If not given, then DOM nodes
    *         will be updated to reflect the current values of all properties.
+   * @param  value (optional)
+   *         If a property name is passed, this argument may contain its
+   *         current value, in order to prevent a further look-up.
    */
-  updateAction(action, propertyName = null) {
+  updateAction(action, propertyName = null, value) {
-    let propertyNames = propertyName ? [propertyName] : [
+    if (propertyName) {
-      "iconURL",
+      this[this._updateMethods[propertyName]](action, value);
-      "title",
+    } else {
-      "tooltip",
+      for (let name of ["iconURL", "title", "tooltip"]) {
-    ];
+        this[this._updateMethods[name]](action, value);
-    for (let name of propertyNames) {
+      }
-      let upper = name[0].toUpperCase() + name.substr(1);
-      this[`_updateAction${upper}`](action);
     }
   },
 
-  _updateActionDisabled(action) {
+  _updateMethods: {
-    this._updateActionDisabledInPanel(action);
+    disabled: "_updateActionDisabled",
+    iconURL: "_updateActionIconURL",
+    title: "_updateActionTitle",
+    tooltip: "_updateActionTooltip",
+  },
+
+  _updateActionDisabled(action, disabled) {
+    this._updateActionDisabledInPanel(action, disabled);
     this.placeActionInUrlbar(action);
   },
 
-  _updateActionDisabledInPanel(action) {
+  _updateActionDisabledInPanel(action, disabled = action.getDisabled(window)) {
     let panelButton = this.panelButtonNodeForActionID(action.id);
     if (panelButton) {
-      if (action.getDisabled(window)) {
+      if (disabled) {
         panelButton.setAttribute("disabled", "true");
       } else {
         panelButton.removeAttribute("disabled");
@@ -491,26 +499,21 @@ var BrowserPageActions = {
     }
   },
 
-  _updateActionIconURL(action) {
+  _updateActionIconURL(action, properties = action.getIconProperties(window)) {
-    let nodes = [
+    let panelButton = this.panelButtonNodeForActionID(action.id);
-      this.panelButtonNodeForActionID(action.id),
+    let urlbarButton = this.urlbarButtonNodeForActionID(action.id);
-      this.urlbarButtonNodeForActionID(action.id),
+
-    ].filter(n => !!n);
+    for (let [prop, value] of Object.entries(properties)) {
-    for (let node of nodes) {
+      if (panelButton) {
-      for (let size of [16, 32]) {
+        panelButton.style.setProperty(prop, value);
-        let url = action.iconURLForSize(size, window);
+      }
-        let prop = `--pageAction-image-${size}px`;
+      if (urlbarButton) {
-        if (url) {
+        urlbarButton.style.setProperty(prop, value);
-          node.style.setProperty(prop, `url("${url}")`);
-        } else {
-          node.style.removeProperty(prop);
-        }
       }
     }
   },
 
-  _updateActionTitle(action) {
+  _updateActionTitle(action, title = action.getTitle(window)) {
-    let title = action.getTitle(window);
     if (!title) {
       // `title` is a required action property, but the bookmark action's is an
       // empty string since its actual title is set via
@@ -518,25 +521,28 @@ var BrowserPageActions = {
       // return is to ignore that empty title.
       return;
     }
-    let attrNamesByNodeFnName = {
+    let panelButton = this.panelButtonNodeForActionID(action.id);
-      panelButtonNodeForActionID: "label",
+    if (panelButton) {
-      urlbarButtonNodeForActionID: "aria-label",
+      panelButton.setAttribute("label", title);
-    };
+    }
-    for (let [fnName, attrName] of Object.entries(attrNamesByNodeFnName)) {
+
-      let node = this[fnName](action.id);
+    let urlbarButton = this.urlbarButtonNodeForActionID(action.id);
-      if (node) {
+    if (urlbarButton) {
-        node.setAttribute(attrName, title);
+      urlbarButton.setAttribute("aria-label", title);
-      }
+
+      // tooltiptext falls back to the title, so update it, too.
+      this._updateActionTooltip(action, undefined, title, urlbarButton);
     }
-    // tooltiptext falls back to the title, so update it, too.
-    this._updateActionTooltip(action);
   },
 
-  _updateActionTooltip(action) {
+  _updateActionTooltip(action, tooltip = action.getTooltip(window),
-    let node = this.urlbarButtonNodeForActionID(action.id);
+                       title,
+                       node = this.urlbarButtonNodeForActionID(action.id)) {
     if (node) {
-      let tooltip = action.getTooltip(window) || action.getTitle(window);
+      if (!tooltip && title === undefined) {
-      node.setAttribute("tooltiptext", tooltip);
+        title = action.getTitle(window);
+      }
+      node.setAttribute("tooltiptext", tooltip || title);
     }
   },
 

browser/modules/PageActions.jsm

@@ -32,6 +32,11 @@ const ACTION_ID_BUILT_IN_SEPARATOR = "builtInSeparator";
 const PREF_PERSISTED_ACTIONS = "browser.pageActions.persistedActions";
 const PERSISTED_ACTIONS_CURRENT_VERSION = 1;
 
+// Escapes the given raw URL string, and returns an equivalent CSS url()
+// value for it.
+function escapeCSSURL(url) {
+  return `url("${url.replace(/[\\\s"]/g, encodeURIComponent)}")`;
+}
 
 var PageActions = {
   /**
@@ -617,6 +622,29 @@ function Action(options) {
   if (this._subview) {
     this._subview = new Subview(options.subview);
   }
+
+  /**
+   * A cache of the pre-computed CSS variable values for a given icon
+   * URLs object, as passed to _createIconProperties.
+   */
+  this._iconProperties = new WeakMap();
+
+  /**
+   * The global values for the action properties.
+   */
+  this._globalProps = {
+    disabled: this._disabled,
+    iconURL: this._iconURL,
+    iconProps: this._createIconProperties(this._iconURL),
+    title: this._title,
+    tooltip: this._tooltip,
+  };
+
+  /**
+   * A mapping of window-specific action property objects, each of which
+   * derives from the _globalProps object.
+   */
+  this._windowProps = new WeakMap();
 }
 
 Action.prototype = {
@@ -661,7 +689,7 @@ Action.prototype = {
    * The action's disabled state (bool, nonnull)
    */
   getDisabled(browserWindow = null) {
-    return !!this._getProperty("disabled", browserWindow);
+    return !!this._getProperties(browserWindow).disabled;
   },
   setDisabled(value, browserWindow = null) {
     return this._setProperty("disabled", !!value, browserWindow);
@@ -672,17 +700,49 @@ Action.prototype = {
    * (string or object, nullable)
    */
   getIconURL(browserWindow = null) {
-    return this._getProperty("iconURL", browserWindow);
+    return this._getProperties(browserWindow).iconURL;
   },
   setIconURL(value, browserWindow = null) {
-    return this._setProperty("iconURL", value, browserWindow);
+    let props = this._getProperties(browserWindow, !!browserWindow);
+    props.iconURL = value;
+    props.iconProps = this._createIconProperties(value);
+
+    this._updateProperty("iconURL", props.iconProps, browserWindow);
+    return value;
+  },
+
+  /**
+   * The set of CSS variables which define the action's icons in various
+   * sizes. This is generated automatically from the iconURL property.
+   */
+  getIconProperties(browserWindow = null) {
+    return this._getProperties(browserWindow).iconProps;
+  },
+
+  _createIconProperties(urls) {
+    if (urls && typeof urls == "object") {
+      let props = this._iconProperties.get(urls);
+      if (!props) {
+        props = Object.freeze({
+          "--pageAction-image-16px": escapeCSSURL(this._iconURLForSize(urls, 16)),
+          "--pageAction-image-32px": escapeCSSURL(this._iconURLForSize(urls, 32)),
+        });
+        this._iconProperties.set(urls, props);
+      }
+      return props;
+    }
+
+    return Object.freeze({
+      "--pageAction-image-16px": null,
+      "--pageAction-image-32px": urls ? escapeCSSURL(urls) : null,
+    });
   },
 
   /**
    * The action's title (string, nonnull)
    */
   getTitle(browserWindow = null) {
-    return this._getProperty("title", browserWindow);
+    return this._getProperties(browserWindow).title;
   },
   setTitle(value, browserWindow = null) {
     return this._setProperty("title", value, browserWindow);
@@ -692,7 +752,7 @@ Action.prototype = {
    * The action's tooltip (string, nullable)
    */
   getTooltip(browserWindow = null) {
-    return this._getProperty("tooltip", browserWindow);
+    return this._getProperties(browserWindow).tooltip;
   },
   setTooltip(value, browserWindow = null) {
     return this._setProperty("tooltip", value, browserWindow);
@@ -710,56 +770,45 @@ Action.prototype = {
    *         globally.
    */
   _setProperty(name, value, browserWindow) {
-    if (!browserWindow) {
+    let props = this._getProperties(browserWindow, !!browserWindow);
-      // Set the global state.
+    props[name] = value;
-      this[`_${name}`] = value;
+
-    } else {
+    this._updateProperty(name, value, browserWindow);
-      // Set the per-window state.
-      let props = this._propertiesByBrowserWindow.get(browserWindow);
-      if (!props) {
-        props = {};
-        this._propertiesByBrowserWindow.set(browserWindow, props);
-      }
-      props[name] = value;
-    }
-    // This may be called before the action has been added.
-    if (PageActions.actionForID(this.id)) {
-      for (let bpa of allBrowserPageActions(browserWindow)) {
-        bpa.updateAction(this, name);
-      }
-    }
     return value;
   },
 
-  /**
+  _updateProperty(name, value, browserWindow) {
-   * Gets a property, optionally for a particular browser window.
+    // This may be called before the action has been added.
-   *
+    if (PageActions.actionForID(this.id)) {
-   * @param  name (string, required)
+      for (let bpa of allBrowserPageActions(browserWindow)) {
-   *         The (non-underscored) name of the property.
+        bpa.updateAction(this, name, value);
-   * @param  browserWindow (DOM window, optional)
-   *         If given, then the property will be fetched from this window's
-   *         state.  If the property does not exist in the window's state, or if
-   *         no window is given, then the global value is returned.
-   * @return The property value.
-   */
-  _getProperty(name, browserWindow) {
-    if (browserWindow) {
-      // Try the per-window state.
-      let props = this._propertiesByBrowserWindow.get(browserWindow);
-      if (props && name in props) {
-        return props[name];
       }
     }
-    // Fall back to the global state.
-    return this[`_${name}`];
   },
 
-  // maps browser windows => object with properties for that window
+  /**
-  get _propertiesByBrowserWindow() {
+   * Returns the properties object for the given window, if it exists,
-    if (!this.__propertiesByBrowserWindow) {
+   * or the global properties object if no window-specific properties
-      this.__propertiesByBrowserWindow = new WeakMap();
+   * exist.
+   *
+   * @param {Window?} window
+   *        The window for which to return the properties object, or
+   *        null to return the global properties object.
+   * @param {bool} [forceWindowSpecific = false]
+   *        If true, always returns a window-specific properties object.
+   *        If a properties object does not exist for the given window,
+   *        one is created and cached.
+   * @returns {object}
+   */
+  _getProperties(window, forceWindowSpecific = false) {
+    let props = window && this._windowProps.get(window);
+
+    if (!props && forceWindowSpecific) {
+      props = Object.create(this._globalProps);
+      this._windowProps.set(window, props);
     }
-    return this.__propertiesByBrowserWindow;
+
+    return props || this._globalProps;
   },
 
   /**
@@ -813,25 +862,33 @@ Action.prototype = {
       return iconURL;
     }
     if (typeof(iconURL) == "object") {
-      // This case is copied from ExtensionParent.jsm so that our image logic is
+      return this._iconURLForSize(iconURL, preferredSize);
-      // the same, so that WebExtensions page action tests that deal with icons
-      // pass.
-      let bestSize = null;
-      if (iconURL[preferredSize]) {
-        bestSize = preferredSize;
-      } else if (iconURL[2 * preferredSize]) {
-        bestSize = 2 * preferredSize;
-      } else {
-        let sizes = Object.keys(iconURL)
-                          .map(key => parseInt(key, 10))
-                          .sort((a, b) => a - b);
-        bestSize = sizes.find(candidate => candidate > preferredSize) || sizes.pop();
-      }
-      return iconURL[bestSize];
     }
     return null;
   },
 
+  /**
+   * Selects the best matching icon from the given URLs object for the
+   * given preferred size, as described in {@see iconURLForSize}.
+   */
+  _iconURLForSize(urls, preferredSize) {
+    // This case is copied from ExtensionParent.jsm so that our image logic is
+    // the same, so that WebExtensions page action tests that deal with icons
+    // pass.
+    let bestSize = null;
+    if (urls[preferredSize]) {
+      bestSize = preferredSize;
+    } else if (urls[2 * preferredSize]) {
+      bestSize = 2 * preferredSize;
+    } else {
+      let sizes = Object.keys(urls)
+                        .map(key => parseInt(key, 10))
+                        .sort((a, b) => a - b);
+      bestSize = sizes.find(candidate => candidate > preferredSize) || sizes.pop();
+    }
+    return urls[bestSize];
+  },
+
   /**
    * Performs the command for an action.  If the action has an onCommand
    * handler, then it's called.  If the action has a subview or iframe, then a