Bug 1447490 - replace GCLI toolbox buttons with highlighters and appropriate actors; r=ochameau

GCLI has been handling the state of our buttons as well as managing shared highlighters.
Since the gcli removal, we no longer share the rulers and measure with any other UI, so a location
for shared state is no longer needed.

Differential Revision: https://phabricator.services.mozilla.com/D3506

--HG--
extra : moz-landing-system : lando

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.
  *

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;

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;
 };

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

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;
   }
 

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;
   }
 }
 

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;

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);
   },
 
   /**

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;

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;

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