diff --git a/devtools/client/definitions.js b/devtools/client/definitions.js index 67fd65b86d22..0a68884a5460 100644 --- a/devtools/client/definitions.js +++ b/devtools/client/definitions.js @@ -29,8 +29,6 @@ loader.lazyGetter(this, "ApplicationPanel", () => require("devtools/client/appli // Other dependencies loader.lazyRequireGetter(this, "AccessibilityStartup", "devtools/client/accessibility/accessibility-startup", true); -loader.lazyRequireGetter(this, "CommandUtils", "devtools/client/shared/developer-toolbar", true); -loader.lazyRequireGetter(this, "CommandState", "devtools/shared/gcli/command-state", true); loader.lazyRequireGetter(this, "ResponsiveUIManager", "devtools/client/responsive.html/manager", true); loader.lazyImporter(this, "ScratchpadManager", "resource://devtools/client/scratchpad/scratchpad-manager.jsm"); loader.lazyRequireGetter(this, "getScreenshotFront", "resource://devtools/shared/fronts/screenshot", true); @@ -541,16 +539,10 @@ exports.ToolboxButtons = [ description: l10n("toolbox.buttons.paintflashing"), isTargetSupported: target => target.isLocalTab, onClick(event, toolbox) { - CommandUtils.executeOnTarget(toolbox.target, "paintflashing toggle"); + toolbox.togglePaintFlashing(); }, isChecked(toolbox) { - return CommandState.isEnabledForTarget(toolbox.target, "paintflashing"); - }, - setup(toolbox, onChange) { - CommandState.on("changed", onChange); - }, - teardown(toolbox, onChange) { - CommandState.off("changed", onChange); + return toolbox.isPaintFlashing; } }, { id: "command-button-scratchpad", @@ -600,40 +592,33 @@ exports.ToolboxButtons = [ await screenshotFront.captureAndSave(toolbox.win, args); } }, - { id: "command-button-rulers", - description: l10n("toolbox.buttons.rulers"), - isTargetSupported: target => target.isLocalTab, - onClick(event, toolbox) { - CommandUtils.executeOnTarget(toolbox.target, "rulers"); - }, - isChecked(toolbox) { - return CommandState.isEnabledForTarget(toolbox.target, "rulers"); - }, - setup(toolbox, onChange) { - CommandState.on("changed", onChange); - }, - teardown(toolbox, onChange) { - CommandState.off("changed", onChange); - } - }, - { id: "command-button-measure", - description: l10n("toolbox.buttons.measure"), - isTargetSupported: target => target.isLocalTab, - onClick(event, toolbox) { - CommandUtils.executeOnTarget(toolbox.target, "measure"); - }, - isChecked(toolbox) { - return CommandState.isEnabledForTarget(toolbox.target, "measure"); - }, - setup(toolbox, onChange) { - CommandState.on("changed", onChange); - }, - teardown(toolbox, onChange) { - CommandState.off("changed", onChange); - } - }, + createHighlightButton("RulersHighlighter", "rulers"), + createHighlightButton("MeasuringToolHighlighter", "measure"), ]; +function createHighlightButton(highlighterName, id) { + return { + id: `command-button-${id}`, + description: l10n(`toolbox.buttons.${id}`), + isTargetSupported: target => !target.chrome, + async onClick(event, toolbox) { + const highlighter = + await toolbox.highlighterUtils.getOrCreateHighlighterByType(highlighterName); + if (highlighter.isShown()) { + return highlighter.hide(); + } + // Starting with FF63, higlighter's spec accept a null first argument. + // Still pass an empty object to fake a domnode front in order to support old + // servers. + return highlighter.show({}); + }, + isChecked(toolbox) { + const highlighter = toolbox.highlighterUtils.getKnownHighlighter(highlighterName); + return highlighter && highlighter.isShown(); + } + }; +} + /** * Lookup l10n string from a string bundle. * diff --git a/devtools/client/framework/components/ToolboxToolbar.js b/devtools/client/framework/components/ToolboxToolbar.js index d9d5a22b8981..29229c6f5ac7 100644 --- a/devtools/client/framework/components/ToolboxToolbar.js +++ b/devtools/client/framework/components/ToolboxToolbar.js @@ -189,8 +189,8 @@ function renderToolboxButtons({focusedButton, toolboxButtons, focusButton}, isSt id, description, disabled, - onClick, isChecked, + onClick, className: buttonClass, onKeyDown } = command; diff --git a/devtools/client/framework/toolbox-highlighter-utils.js b/devtools/client/framework/toolbox-highlighter-utils.js index 10c6eaa6db88..4f93de46c745 100644 --- a/devtools/client/framework/toolbox-highlighter-utils.js +++ b/devtools/client/framework/toolbox-highlighter-utils.js @@ -287,13 +287,13 @@ exports.getHighlighterUtils = function(toolbox) { }; /** - * If the main, box-model, highlighter isn't enough, or if multiple - * highlighters are needed in parallel, this method can be used to return a - * new instance of a highlighter actor, given a type. + * If the main, box-model, highlighter isn't enough, or if multiple highlighters + * are needed in parallel, this method can be used to return a new instance of a + * highlighter actor, given a type. * The type of the highlighter passed must be known by the server. * The highlighter actor returned will have the show(nodeFront) and hide() * methods and needs to be released by the consumer when not needed anymore. - * @return a promise that resolves to the highlighter + * @return Promise a promise that resolves to the highlighter */ exported.getHighlighterByType = requireInspector(async function(typeName) { const highlighter = await toolbox.inspector.getHighlighterByType(typeName); @@ -302,6 +302,26 @@ exports.getHighlighterUtils = function(toolbox) { `creating highlighters by types or ${typeName} is unknown`); }); + /** + * Similar to getHighlighterByType, however it automatically memoizes and + * destroys highlighters with the inspector, instead of having to be manually + * managed by consumers. + * The type of the highlighter passed must be known by the server. + * The highlighter actor returned will have the show(nodeFront) and hide() + * methods and needs to be released by the consumer when not needed anymore. + * @return Promise a promise that resolves to the highlighter + */ + exported.getOrCreateHighlighterByType = requireInspector(async function(typeName) { + const highlighter = await toolbox.inspector.getOrCreateHighlighterByType(typeName); + + return highlighter || promise.reject("The target doesn't support " + + `creating highlighters by types or ${typeName} is unknown`); + }); + + exported.getKnownHighlighter = function(typeName) { + return toolbox.inspector && toolbox.inspector.getKnownHighlighter(typeName); + }; + // Return the public API return exported; }; diff --git a/devtools/client/framework/toolbox.js b/devtools/client/framework/toolbox.js index fe846e1efffd..f15d8d4a31b5 100644 --- a/devtools/client/framework/toolbox.js +++ b/devtools/client/framework/toolbox.js @@ -154,6 +154,7 @@ function Toolbox(target, selectedTool, hostType, contentWindow, frameId, this._onNewSelectedNodeFront = this._onNewSelectedNodeFront.bind(this); this._onToolSelected = this._onToolSelected.bind(this); this.updateToolboxButtonsVisibility = this.updateToolboxButtonsVisibility.bind(this); + this.updateToolboxButtons = this.updateToolboxButtons.bind(this); this.selectTool = this.selectTool.bind(this); this._pingTelemetrySelectTool = this._pingTelemetrySelectTool.bind(this); this.toggleSplitConsole = this.toggleSplitConsole.bind(this); @@ -799,9 +800,9 @@ Toolbox.prototype = { className, description, disabled, - onClick(event) { + async onClick(event) { if (typeof onClick == "function") { - onClick(event, toolbox); + await onClick(event, toolbox); button.emit("updatechecked"); } }, @@ -1357,6 +1358,27 @@ Toolbox.prototype = { this.component.setToolboxButtons(this.toolbarButtons); }, + /** + * Update the buttons. + */ + updateToolboxButtons() { + const inspector = this.inspector; + // two of the buttons have highlighters that need to be cleared + // on will-navigate, otherwise we hold on to the stale highlighter + const hasHighlighters = inspector && + (inspector.hasHighlighter("RulersHighlighter") || + inspector.hasHighlighter("MeasuringToolHighlighter")); + if (hasHighlighters || this.isPaintFlashing) { + if (this.isPaintFlashing) { + this.togglePaintFlashing(); + } + if (hasHighlighters) { + inspector.destroyHighlighters(); + } + this.component.setToolboxButtons(this.toolbarButtons); + } + }, + /** * Set paintflashing to enabled or disabled for this toolbox's tab. */ @@ -1367,7 +1389,7 @@ Toolbox.prototype = { this.telemetry.toolClosed("paintflashing"); } this.isPaintFlashing = !this.isPaintFlashing; - this.target.activeTab.reconfigure({"paintFlashing": this.isPaintFlashing}); + return this.target.activeTab.reconfigure({"paintFlashing": this.isPaintFlashing}); }, /** @@ -2179,6 +2201,7 @@ Toolbox.prototype = { * Fired when user just started navigating away to another web page. */ async _onWillNavigate() { + this.updateToolboxButtons(); const toolId = this.currentToolId; // For now, only inspector, webconsole and netmonitor fire "reloaded" event if (toolId != "inspector" && toolId != "webconsole" && toolId != "netmonitor") { diff --git a/devtools/client/inspector/shared/highlighters-overlay.js b/devtools/client/inspector/shared/highlighters-overlay.js index c8234ff1cdac..947588d8c97d 100644 --- a/devtools/client/inspector/shared/highlighters-overlay.js +++ b/devtools/client/inspector/shared/highlighters-overlay.js @@ -1007,9 +1007,6 @@ class HighlightersOverlay { this.editors = {}; } - /** - * Destroy and clean-up all instances of highlighters. - */ destroyHighlighters() { for (const type in this.highlighters) { if (this.highlighters[type]) { @@ -1017,7 +1014,6 @@ class HighlightersOverlay { this.highlighters[type] = null; } } - this.highlighters = null; } diff --git a/devtools/client/shared/test/browser_telemetry_button_paintflashing.js b/devtools/client/shared/test/browser_telemetry_button_paintflashing.js index c1eb1aaf16a8..8fb69748cfcc 100644 --- a/devtools/client/shared/test/browser_telemetry_button_paintflashing.js +++ b/devtools/client/shared/test/browser_telemetry_button_paintflashing.js @@ -44,21 +44,8 @@ async function delayedClicks(toolbox, node, clicks) { // See TOOL_DELAY for why we need setTimeout here setTimeout(() => resolve(), TOOL_DELAY); }); - - const { CommandState } = require("devtools/shared/gcli/command-state"); - const clicked = new Promise(resolve => { - CommandState.on("changed", function changed({command}) { - if (command === "paintflashing") { - CommandState.off("changed", changed); - resolve(); - } - }); - }); - info("Clicking button " + node.id); node.click(); - - await clicked; } } diff --git a/devtools/client/styleeditor/StyleEditorUI.jsm b/devtools/client/styleeditor/StyleEditorUI.jsm index c5457e1a83c2..25ac9217a4f1 100644 --- a/devtools/client/styleeditor/StyleEditorUI.jsm +++ b/devtools/client/styleeditor/StyleEditorUI.jsm @@ -1029,11 +1029,6 @@ StyleEditorUI.prototype = { }, destroy: function() { - if (this._highlighter) { - this._highlighter.finalize(); - this._highlighter = null; - } - this._clearStyleSheetEditors(); this._seenSheets = null; diff --git a/devtools/server/actors/highlighters.js b/devtools/server/actors/highlighters.js index b52bc928efe4..ff17e60900c9 100644 --- a/devtools/server/actors/highlighters.js +++ b/devtools/server/actors/highlighters.js @@ -498,11 +498,13 @@ exports.CustomHighlighterActor = protocol.ActorClassWithSpec(customHighlighterSp * (FF41+) */ show: function(node, options) { - if (!node || !this._highlighter) { - return false; + if (!this._highlighter) { + return null; } - return this._highlighter.show(node.rawNode, options); + const rawNode = node && node.rawNode; + + return this._highlighter.show(rawNode, options); }, /** diff --git a/devtools/shared/fronts/highlighters.js b/devtools/shared/fronts/highlighters.js index a7b13b737709..27573d79fdc7 100644 --- a/devtools/shared/fronts/highlighters.js +++ b/devtools/shared/fronts/highlighters.js @@ -24,11 +24,31 @@ const HighlighterFront = FrontClassWithSpec(highlighterSpec, { return this._pick(); }, { impl: "_pick" - }) + }), }); exports.HighlighterFront = HighlighterFront; -const CustomHighlighterFront = FrontClassWithSpec(customHighlighterSpec, {}); +const CustomHighlighterFront = FrontClassWithSpec(customHighlighterSpec, { + _isShown: false, + + show: custom(function(...args) { + this._isShown = true; + return this._show(...args); + }, { + impl: "_show" + }), + + hide: custom(function() { + this._isShown = false; + return this._hide(); + }, { + impl: "_hide" + }), + + isShown: function() { + return this._isShown; + } +}); exports.CustomHighlighterFront = CustomHighlighterFront; diff --git a/devtools/shared/fronts/inspector.js b/devtools/shared/fronts/inspector.js index 12793f3ef985..8f5a7a6485ee 100644 --- a/devtools/shared/fronts/inspector.js +++ b/devtools/shared/fronts/inspector.js @@ -452,17 +452,45 @@ var InspectorFront = FrontClassWithSpec(inspectorSpec, { initialize: function(client, tabForm) { Front.prototype.initialize.call(this, client); this.actorID = tabForm.inspectorActor; + this._highlighters = new Map(); // XXX: This is the first actor type in its hierarchy to use the protocol // library, so we're going to self-own on the client side for now. this.manage(this); }, + hasHighlighter(type) { + return this._highlighters.has(type); + }, + destroy: function() { + this.destroyHighlighters(); delete this.walker; Front.prototype.destroy.call(this); }, + destroyHighlighters: function() { + for (const type of this._highlighters.keys()) { + if (this._highlighters.has(type)) { + this._highlighters.get(type).finalize(); + this._highlighters.delete(type); + } + } + }, + + getKnownHighlighter: function(type) { + return this._highlighters.get(type); + }, + + getOrCreateHighlighterByType: async function(type) { + let front = this._highlighters.get(type); + if (!front) { + front = await this.getHighlighterByType(type); + this._highlighters.set(type, front); + } + return front; + }, + getWalker: custom(function(options = {}) { return this._getWalker(options).then(walker => { this.walker = walker; diff --git a/devtools/shared/specs/highlighters.js b/devtools/shared/specs/highlighters.js index 3b6ae371b6e6..0e2109ce3726 100644 --- a/devtools/shared/specs/highlighters.js +++ b/devtools/shared/specs/highlighters.js @@ -51,7 +51,7 @@ const customHighlighterSpec = generateActorSpec({ }, show: { request: { - node: Arg(0, "domnode"), + node: Arg(0, "nullable:domnode"), options: Arg(1, "nullable:json") }, response: {