Bug 1446250: Part 1 - Optimize Photon PageAction update performance. r?Gijs draft
authorKris Maglione <maglione.k@gmail.com>
Thu, 15 Mar 2018 21:34:01 -0700
changeset 768851 38731c468ecbbca45e5c78b2d4b2dc465cf90f0d
parent 768345 f1625f41dda7a51114cc0c51dc6b468897a01622
child 768852 1472efc96f42a89d0cea20e6b0a9dd00a8bfdb78
push id102991
push usermaglione.k@gmail.com
push dateFri, 16 Mar 2018 22:41:24 +0000
reviewersGijs
bugs1446250
milestone61.0a1
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
browser/base/content/browser-pageActions.js
browser/modules/PageActions.jsm
--- a/browser/base/content/browser-pageActions.js
+++ b/browser/base/content/browser-pageActions.js
@@ -457,91 +457,97 @@ var BrowserPageActions = {
    * Updates the DOM nodes of an action to reflect either a changed property or
    * all properties.
    *
    * @param  action (PageActions.Action, required)
    *         The action to update.
    * @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) {
-    let propertyNames = propertyName ? [propertyName] : [
-      "iconURL",
-      "title",
-      "tooltip",
-    ];
-    for (let name of propertyNames) {
-      let upper = name[0].toUpperCase() + name.substr(1);
-      this[`_updateAction${upper}`](action);
+  updateAction(action, propertyName = null, value) {
+    if (propertyName) {
+      this[this._updateMethods[propertyName]](action, value);
+    } else {
+      for (let name of ["iconURL", "title", "tooltip"]) {
+        this[this._updateMethods[name]](action, value);
+      }
     }
   },
 
-  _updateActionDisabled(action) {
-    this._updateActionDisabledInPanel(action);
+  _updateMethods: {
+    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");
       }
     }
   },
 
-  _updateActionIconURL(action) {
-    let nodes = [
-      this.panelButtonNodeForActionID(action.id),
-      this.urlbarButtonNodeForActionID(action.id),
-    ].filter(n => !!n);
-    for (let node of nodes) {
-      for (let size of [16, 32]) {
-        let url = action.iconURLForSize(size, window);
-        let prop = `--pageAction-image-${size}px`;
-        if (url) {
-          node.style.setProperty(prop, `url("${url}")`);
-        } else {
-          node.style.removeProperty(prop);
-        }
+  _updateActionIconURL(action, properties = action.getIconProperties(window)) {
+    let panelButton = this.panelButtonNodeForActionID(action.id);
+    let urlbarButton = this.urlbarButtonNodeForActionID(action.id);
+
+    for (let [prop, value] of Object.entries(properties)) {
+      if (panelButton) {
+        panelButton.style.setProperty(prop, value);
+      }
+      if (urlbarButton) {
+        urlbarButton.style.setProperty(prop, value);
       }
     }
   },
 
-  _updateActionTitle(action) {
-    let title = action.getTitle(window);
+  _updateActionTitle(action, 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
       // BookmarkingUI.updateBookmarkPageMenuItem().  The purpose of this early
       // return is to ignore that empty title.
       return;
     }
-    let attrNamesByNodeFnName = {
-      panelButtonNodeForActionID: "label",
-      urlbarButtonNodeForActionID: "aria-label",
-    };
-    for (let [fnName, attrName] of Object.entries(attrNamesByNodeFnName)) {
-      let node = this[fnName](action.id);
-      if (node) {
-        node.setAttribute(attrName, title);
-      }
+    let panelButton = this.panelButtonNodeForActionID(action.id);
+    if (panelButton) {
+      panelButton.setAttribute("label", title);
     }
-    // tooltiptext falls back to the title, so update it, too.
-    this._updateActionTooltip(action);
+
+    let urlbarButton = this.urlbarButtonNodeForActionID(action.id);
+    if (urlbarButton) {
+      urlbarButton.setAttribute("aria-label", title);
+
+      // tooltiptext falls back to the title, so update it, too.
+      this._updateActionTooltip(action, undefined, title, urlbarButton);
+    }
   },
 
-  _updateActionTooltip(action) {
-    let node = this.urlbarButtonNodeForActionID(action.id);
+  _updateActionTooltip(action, tooltip = action.getTooltip(window),
+                       title,
+                       node = this.urlbarButtonNodeForActionID(action.id)) {
     if (node) {
-      let tooltip = action.getTooltip(window) || action.getTitle(window);
-      node.setAttribute("tooltiptext", tooltip);
+      if (!tooltip && title === undefined) {
+        title = action.getTitle(window);
+      }
+      node.setAttribute("tooltiptext", tooltip || title);
     }
   },
 
   doCommandForAction(action, event, buttonNode) {
     if (event && event.type == "click" && event.button != 0) {
       return;
     }
     PageActions.logTelemetry("used", action, buttonNode);
--- a/browser/modules/PageActions.jsm
+++ b/browser/modules/PageActions.jsm
@@ -27,16 +27,21 @@ ChromeUtils.defineModuleGetter(this, "Bi
 
 const ACTION_ID_BOOKMARK = "bookmark";
 const ACTION_ID_BOOKMARK_SEPARATOR = "bookmarkSeparator";
 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 = {
   /**
    * Inits.  Call to init.
    */
   init() {
     let callbacks = this._deferredAddActionCalls;
     delete this._deferredAddActionCalls;
@@ -612,16 +617,39 @@ function Action(options) {
     // (either the auto-generated ID or urlbarIDOverride).  That node will be
     // shown when the action is added to the urlbar and hidden when the action
     // is removed from the urlbar.
     _urlbarNodeInMarkup: false,
   });
   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 = {
   /**
    * The ID of the action's parent extension (string, nullable)
    */
   get extensionID() {
     return this._extensionID;
@@ -656,48 +684,80 @@ Action.prototype = {
     }
     return this.pinnedToUrlbar;
   },
 
   /**
    * 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);
   },
 
   /**
    * The action's icon URL string, or an object mapping sizes to URL strings
    * (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);
   },
 
   /**
    * 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);
   },
 
   /**
    * Sets a property, optionally for a particular browser window.
    *
@@ -705,66 +765,55 @@ Action.prototype = {
    *         The (non-underscored) name of the property.
    * @param  value
    *         The value.
    * @param  browserWindow (DOM window, optional)
    *         If given, then the property will be set in this window's state, not
    *         globally.
    */
   _setProperty(name, value, browserWindow) {
-    if (!browserWindow) {
-      // Set the global state.
-      this[`_${name}`] = value;
-    } else {
-      // Set the per-window state.
-      let props = this._propertiesByBrowserWindow.get(browserWindow);
-      if (!props) {
-        props = {};
-        this._propertiesByBrowserWindow.set(browserWindow, props);
-      }
-      props[name] = value;
-    }
+    let props = this._getProperties(browserWindow, !!browserWindow);
+    props[name] = value;
+
+    this._updateProperty(name, value, browserWindow);
+    return value;
+  },
+
+  _updateProperty(name, value, browserWindow) {
     // 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);
+        bpa.updateAction(this, name, value);
       }
     }
-    return value;
   },
 
   /**
-   * Gets a property, optionally for a particular browser window.
+   * Returns the properties object for the given window, if it exists,
+   * or the global properties object if no window-specific properties
+   * exist.
    *
-   * @param  name (string, required)
-   *         The (non-underscored) name of the property.
-   * @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.
+   * @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}
    */
-  _getProperty(name, browserWindow) {
-    if (browserWindow) {
-      // Try the per-window state.
-      let props = this._propertiesByBrowserWindow.get(browserWindow);
-      if (props && name in props) {
-        return props[name];
-      }
+  _getProperties(window, forceWindowSpecific = false) {
+    let props = window && this._windowProps.get(window);
+
+    if (!props && forceWindowSpecific) {
+      props = Object.create(this._globalProps);
+      this._windowProps.set(window, props);
     }
-    // Fall back to the global state.
-    return this[`_${name}`];
-  },
 
-  // maps browser windows => object with properties for that window
-  get _propertiesByBrowserWindow() {
-    if (!this.__propertiesByBrowserWindow) {
-      this.__propertiesByBrowserWindow = new WeakMap();
-    }
-    return this.__propertiesByBrowserWindow;
+    return props || this._globalProps;
   },
 
   /**
    * Override for the ID of the action's activated-action panel anchor (string,
    * nullable)
    */
   get anchorIDOverride() {
     return this._anchorIDOverride;
@@ -808,36 +857,44 @@ Action.prototype = {
     let iconURL = this.getIconURL(browserWindow);
     if (!iconURL) {
       return null;
     }
     if (typeof(iconURL) == "string") {
       return iconURL;
     }
     if (typeof(iconURL) == "object") {
-      // 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 (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 this._iconURLForSize(iconURL, preferredSize);
     }
     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
    * panel is opened, displaying the subview or iframe.
    *
    * @param  browserWindow (DOM window, required)
    *         The browser window in which to perform the action.
    */
   doCommand(browserWindow) {