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
--- 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) {