Bug 1342928 - Keep the commands / buttons state in sync; r=jwalker

This was a regression given by bug 1320149; in order to keep the performance
gain I created a lightweight object (`CommandState`) that is required from both
gcli's command and toolbox so that the last one doesn't need to be strong
coupled with the first one.

MozReview-Commit-ID: 3NcTt6i4ezx

--HG--
extra : rebase_source : 2c67d1c8fb90551298d8254e0f05bfcb5705d2f6

devtools/client/commandline/test/browser_cmd_paintflashing.js

@@ -16,7 +16,7 @@ var tests = {
   testInput: function (options) {
     let toggleCommand = options.requisition.system.commands.get("paintflashing toggle");
 
-    let _tab = options.tab;
+    let { tab } = options;
 
     let actions = [
       {
@@ -44,7 +44,7 @@ var tests = {
     return helpers.audit(options, actions.map(spec => ({
       setup: spec.command,
       exec: {},
-      post: () => is(toggleCommand.state.isChecked({_tab}), spec.isChecked, spec.label)
+      post: () => is(toggleCommand.state.isChecked({tab}), spec.isChecked, spec.label)
     })));
   },
 };

devtools/client/definitions.js

@@ -26,6 +26,7 @@ loader.lazyGetter(this, "DomPanel", () => require("devtools/client/dom/dom-panel
 
 // Other dependencies
 loader.lazyRequireGetter(this, "CommandUtils", "devtools/client/shared/developer-toolbar", true);
+loader.lazyRequireGetter(this, "CommandState", "devtools/shared/gcli/command-state", true);
 loader.lazyImporter(this, "ResponsiveUIManager", "resource://devtools/client/responsivedesign/responsivedesign.jsm");
 loader.lazyImporter(this, "ScratchpadManager", "resource://devtools/client/scratchpad/scratchpad-manager.jsm");
 
@@ -497,7 +498,15 @@ exports.ToolboxButtons = [
     onClick(event, toolbox) {
       CommandUtils.executeOnTarget(toolbox.target, "paintflashing toggle");
     },
-    autoToggle: true
+    isChecked(toolbox) {
+      return CommandState.isEnabledForTarget(toolbox.target, "paintflashing");
+    },
+    setup(toolbox, onChange) {
+      CommandState.on("changed", onChange);
+    },
+    teardown(toolbox, onChange) {
+      CommandState.off("changed", onChange);
+    }
   },
   { id: "command-button-scratchpad",
     description: l10n("toolbox.buttons.scratchpad"),
@@ -551,7 +560,15 @@ exports.ToolboxButtons = [
     onClick(event, toolbox) {
       CommandUtils.executeOnTarget(toolbox.target, "rulers");
     },
-    autoToggle: true
+    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"),
@@ -559,7 +576,15 @@ exports.ToolboxButtons = [
     onClick(event, toolbox) {
       CommandUtils.executeOnTarget(toolbox.target, "measure");
     },
-    autoToggle: true
+    isChecked(toolbox) {
+      return CommandState.isEnabledForTarget(toolbox.target, "measure");
+    },
+    setup(toolbox, onChange) {
+      CommandState.on("changed", onChange);
+    },
+    teardown(toolbox, onChange) {
+      CommandState.off("changed", onChange);
+    }
   },
 ];
 

devtools/client/framework/toolbox.js

@@ -574,13 +574,11 @@ Toolbox.prototype = {
    * @property {Function} isChecked - Optional function called to known if the button
    *                      is toggled or not. The function should return true when
    *                      the button should be displayed as toggled on.
-   * @property {Boolean}  autoToggle - If true, the checked state is going to be
-   *                      automatically toggled on click.
    */
   _createButtonState: function (options) {
     let isCheckedValue = false;
     const { id, className, description, onClick, isInStartContainer, setup, teardown,
-            isTargetSupported, isChecked, autoToggle } = options;
+            isTargetSupported, isChecked } = options;
     const toolbox = this;
     const button = {
       id,
@@ -590,9 +588,6 @@ Toolbox.prototype = {
         if (typeof onClick == "function") {
           onClick(event, toolbox);
         }
-        if (autoToggle) {
-          button.isChecked = !button.isChecked;
-        }
       },
       isTargetSupported,
       get isChecked() {

devtools/client/shared/test/browser_telemetry_button_paintflashing.js

@@ -44,8 +44,15 @@ function* delayedClicks(toolbox, node, clicks) {
       setTimeout(() => resolve(), TOOL_DELAY);
     });
 
-    let PaintFlashingCmd = require("devtools/shared/gcli/commands/paintflashing");
-    let clicked = PaintFlashingCmd.eventEmitter.once("changed");
+    let { CommandState } = require("devtools/shared/gcli/command-state");
+    let clicked = new Promise(resolve => {
+      CommandState.on("changed", function changed(type, { command }) {
+        if (command === "paintflashing") {
+          CommandState.off("changed", changed);
+          resolve();
+        }
+      });
+    });
 
     info("Clicking button " + node.id);
     node.click();

devtools/shared/event-emitter.js

@@ -84,6 +84,7 @@
    * @param Object objectToDecorate
    *        Bind all public methods of EventEmitter to
    *        the objectToDecorate object.
+   * @return Object the object given.
    */
   EventEmitter.decorate = function (objectToDecorate) {
     let emitter = new EventEmitter();
@@ -91,6 +92,8 @@
     objectToDecorate.off = emitter.off.bind(emitter);
     objectToDecorate.once = emitter.once.bind(emitter);
     objectToDecorate.emit = emitter.emit.bind(emitter);
+
+    return objectToDecorate;
   };
 
   EventEmitter.prototype = {

devtools/shared/gcli/command-state.js

@@ -0,0 +1,80 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+const EventEmitter = require("devtools/shared/event-emitter");
+
+loader.lazyRequireGetter(this, "getBrowserForTab", "sdk/tabs/utils", true);
+
+const getTargetId = ({tab}) => getBrowserForTab(tab).outerWindowID;
+const enabledCommands = new Map();
+
+/**
+ * The `CommandState` is a singleton that provides utility methods to keep the commands'
+ * state in sync between the toolbox, the toolbar and the content.
+ */
+const CommandState = EventEmitter.decorate({
+  /**
+   * Returns if a command is enabled on a given target.
+   *
+   * @param {Object} target
+   *                  The target object must have a tab's reference.
+   * @param {String} command
+   *                  The command's name used in gcli.
+   * @ returns {Boolean} returns `false` if the command is not enabled for the target
+   *                    given, or if the target given hasn't a tab; `true` otherwise.
+   */
+  isEnabledForTarget(target, command) {
+    if (!target.tab || !enabledCommands.has(command)) {
+      return false;
+    }
+
+    return enabledCommands.get(command).has(getTargetId(target));
+  },
+
+  /**
+   * Enables a command on a given target.
+   * Emits a "changed" event to notify potential observers about the new commands state.
+   *
+   * @param {Object} target
+   *                  The target object must have a tab's reference.
+   * @param {String} command
+   *                  The command's name used in gcli.
+   */
+  enableForTarget(target, command) {
+    if (!target.tab) {
+      return;
+    }
+
+    if (!enabledCommands.has(command)) {
+      enabledCommands.set(command, new Set());
+    }
+
+    enabledCommands.get(command).add(getTargetId(target));
+
+    CommandState.emit("changed", {target, command});
+  },
+
+  /**
+   * Disabled a command on a given target.
+   * Emits a "changed" event to notify potential observers about the new commands state.
+   *
+   * @param {Object} target
+   *                  The target object must have a tab's reference.
+   * @param {String} command
+   *                  The command's name used in gcli.
+   */
+  disableForTarget(target, command) {
+    if (!target.tab || !enabledCommands.has(command)) {
+      return;
+    }
+
+    enabledCommands.get(command).delete(getTargetId(target));
+
+    CommandState.emit("changed", {target, command});
+  },
+});
+exports.CommandState = CommandState;
+

devtools/shared/gcli/commands/measure.js

@@ -1,16 +1,13 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
- /* globals getOuterId, getBrowserForTab */
 
 "use strict";
 
-const EventEmitter = require("devtools/shared/event-emitter");
-const eventEmitter = new EventEmitter();
 const events = require("sdk/event/core");
 
-loader.lazyRequireGetter(this, "getOuterId", "sdk/window/utils", true);
-loader.lazyRequireGetter(this, "getBrowserForTab", "sdk/tabs/utils", true);
+loader.lazyRequireGetter(this, "CommandState",
+  "devtools/shared/gcli/command-state", true);
 
 const l10n = require("gcli/l10n");
 require("devtools/server/actors/inspector");
@@ -18,10 +15,6 @@ const { MeasuringToolHighlighter, HighlighterEnvironment } =
   require("devtools/server/actors/highlighters");
 
 const highlighters = new WeakMap();
-const visibleHighlighters = new Set();
-
-const isCheckedFor = (tab) =>
-  tab ? visibleHighlighters.has(getBrowserForTab(tab).outerWindowID) : false;
 
 exports.items = [
   // The client measure command is used to maintain the toolbar button state
@@ -36,34 +29,28 @@ exports.items = [
     buttonClass: "command-button command-button-invertable",
     tooltipText: l10n.lookup("measureTooltip"),
     state: {
-      isChecked: ({_tab}) => isCheckedFor(_tab),
-      onChange: (target, handler) => eventEmitter.on("changed", handler),
-      offChange: (target, handler) => eventEmitter.off("changed", handler)
+      isChecked: (target) => CommandState.isEnabledForTarget(target, "measure"),
+      onChange: (target, handler) => CommandState.on("changed", handler),
+      offChange: (target, handler) => CommandState.off("changed", handler)
     },
     exec: function* (args, context) {
       let { target } = context.environment;
 
       // Pipe the call to the server command.
       let response = yield context.updateExec("measure_server");
-      let { visible, id } = response.data;
+      let isEnabled = response.data;
 
-      if (visible) {
-        visibleHighlighters.add(id);
+      if (isEnabled) {
+        CommandState.enableForTarget(target, "measure");
       } else {
-        visibleHighlighters.delete(id);
+        CommandState.disableForTarget(target, "measure");
       }
 
-      eventEmitter.emit("changed", { target });
-
       // Toggle off the button when the page navigates because the measuring
       // tool is removed automatically by the MeasuringToolHighlighter on the
       // server then.
-      let onNavigate = () => {
-        visibleHighlighters.delete(id);
-        eventEmitter.emit("changed", { target });
-      };
-      target.off("will-navigate", onNavigate);
-      target.once("will-navigate", onNavigate);
+      target.once("will-navigate", () =>
+        CommandState.disableForTarget(target, "measure"));
     }
   },
   // The server measure command is hidden by default, it's just used by the
@@ -76,14 +63,13 @@ exports.items = [
     exec: function (args, context) {
       let env = context.environment;
       let { document } = env;
-      let id = getOuterId(env.window);
 
       // Calling the command again after the measuring tool has been shown once,
       // hides it.
       if (highlighters.has(document)) {
         let { highlighter } = highlighters.get(document);
         highlighter.destroy();
-        return {visible: false, id};
+        return false;
       }
 
       // Otherwise, display the measuring tool.
@@ -106,7 +92,7 @@ exports.items = [
       });
 
       highlighter.show();
-      return {visible: true, id};
+      return true;
     }
   }
 ];

devtools/shared/gcli/commands/paintflashing.js

@@ -5,8 +5,9 @@
 "use strict";
 
 const { Ci } = require("chrome");
-loader.lazyRequireGetter(this, "getOuterId", "sdk/window/utils", true);
-loader.lazyRequireGetter(this, "getBrowserForTab", "sdk/tabs/utils", true);
+
+loader.lazyRequireGetter(this, "CommandState",
+  "devtools/shared/gcli/command-state", true);
 
 var telemetry;
 try {
@@ -16,39 +17,21 @@ try {
   // DevTools Telemetry module only available in Firefox
 }
 
-const EventEmitter = require("devtools/shared/event-emitter");
-const eventEmitter = new EventEmitter();
-
-// exports the event emitter to help test know when this command is toggled
-exports.eventEmitter = eventEmitter;
-
 const gcli = require("gcli/index");
 const l10n = require("gcli/l10n");
 
-const enabledPaintFlashing = new Set();
-
-const isCheckedFor = (tab) =>
-  tab ? enabledPaintFlashing.has(getBrowserForTab(tab).outerWindowID) : false;
-
 /**
  * Fire events and telemetry when paintFlashing happens
  */
-function onPaintFlashingChanged(target, state) {
-  const { flashing, id } = state;
-
+function onPaintFlashingChanged(target, flashing) {
   if (flashing) {
-    enabledPaintFlashing.add(id);
+    CommandState.enableForTarget(target, "paintflashing");
   } else {
-    enabledPaintFlashing.delete(id);
+    CommandState.disableForTarget(target, "paintflashing");
   }
 
-  eventEmitter.emit("changed", { target: target });
-  function fireChange() {
-    eventEmitter.emit("changed", { target: target });
-  }
-
-  target.off("navigate", fireChange);
-  target.once("navigate", fireChange);
+  target.once("will-navigate", () =>
+    CommandState.disableForTarget(target, "paintflashing"));
 
   if (!telemetry) {
     return;
@@ -165,9 +148,9 @@ exports.items = [
     buttonId: "command-button-paintflashing",
     buttonClass: "command-button command-button-invertable",
     state: {
-      isChecked: ({_tab}) => isCheckedFor(_tab),
-      onChange: (_, handler) => eventEmitter.on("changed", handler),
-      offChange: (_, handler) => eventEmitter.off("changed", handler),
+      isChecked: (target) => CommandState.isEnabledForTarget(target, "paintflashing"),
+      onChange: (_, handler) => CommandState.on("changed", handler),
+      offChange: (_, handler) => CommandState.off("changed", handler),
     },
     tooltipText: l10n.lookup("paintflashingTooltip"),
     description: l10n.lookup("paintflashingToggleDesc"),
@@ -195,10 +178,8 @@ exports.items = [
     returnType: "paintFlashingState",
     exec: function (args, context) {
       let { window } = context.environment;
-      let id = getOuterId(window);
-      let flashing = setPaintFlashing(window, args.state);
 
-      return { flashing, id };
+      return setPaintFlashing(window, args.state);
     }
   }
 ];

devtools/shared/gcli/commands/rulers.js

@@ -1,15 +1,14 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-/* globals getBrowserForTab */
 
 "use strict";
 
-const EventEmitter = require("devtools/shared/event-emitter");
-const eventEmitter = new EventEmitter();
 const events = require("sdk/event/core");
+
 loader.lazyRequireGetter(this, "getOuterId", "sdk/window/utils", true);
-loader.lazyRequireGetter(this, "getBrowserForTab", "sdk/tabs/utils", true);
+loader.lazyRequireGetter(this, "CommandState",
+  "devtools/shared/gcli/command-state", true);
 
 const l10n = require("gcli/l10n");
 require("devtools/server/actors/inspector");
@@ -17,10 +16,6 @@ const { RulersHighlighter, HighlighterEnvironment } =
   require("devtools/server/actors/highlighters");
 
 const highlighters = new WeakMap();
-const visibleHighlighters = new Set();
-
-const isCheckedFor = (tab) =>
-  tab ? visibleHighlighters.has(getBrowserForTab(tab).outerWindowID) : false;
 
 exports.items = [
   // The client rulers command is used to maintain the toolbar button state only
@@ -35,33 +30,26 @@ exports.items = [
     buttonClass: "command-button command-button-invertable",
     tooltipText: l10n.lookup("rulersTooltip"),
     state: {
-      isChecked: ({_tab}) => isCheckedFor(_tab),
-      onChange: (target, handler) => eventEmitter.on("changed", handler),
-      offChange: (target, handler) => eventEmitter.off("changed", handler)
+      isChecked: (target) => CommandState.isEnabledForTarget(target, "rulers"),
+      onChange: (target, handler) => CommandState.on("changed", handler),
+      offChange: (target, handler) => CommandState.off("changed", handler)
     },
     exec: function* (args, context) {
       let { target } = context.environment;
 
       // Pipe the call to the server command.
       let response = yield context.updateExec("rulers_server");
-      let { visible, id } = response.data;
+      let isEnabled = response.data;
 
-      if (visible) {
-        visibleHighlighters.add(id);
+      if (isEnabled) {
+        CommandState.enableForTarget(target, "rulers");
       } else {
-        visibleHighlighters.delete(id);
+        CommandState.disableForTarget(target, "rulers");
       }
 
-      eventEmitter.emit("changed", { target });
-
       // Toggle off the button when the page navigates because the rulers are
       // removed automatically by the RulersHighlighter on the server then.
-      let onNavigate = () => {
-        visibleHighlighters.delete(id);
-        eventEmitter.emit("changed", { target });
-      };
-      target.off("will-navigate", onNavigate);
-      target.once("will-navigate", onNavigate);
+      target.once("will-navigate", () => CommandState.disableForTarget(target, "rulers"));
     }
   },
   // The server rulers command is hidden by default, it's just used by the
@@ -74,14 +62,13 @@ exports.items = [
     exec: function (args, context) {
       let env = context.environment;
       let { document } = env;
-      let id = getOuterId(env.window);
 
       // Calling the command again after the rulers have been shown once hides
       // them.
       if (highlighters.has(document)) {
         let { highlighter } = highlighters.get(document);
         highlighter.destroy();
-        return {visible: false, id};
+        return false;
       }
 
       // Otherwise, display the rulers.
@@ -104,7 +91,7 @@ exports.items = [
       });
 
       highlighter.show();
-      return {visible: true, id};
+      return true;
     }
   }
 ];

devtools/shared/gcli/moz.build

@@ -19,5 +19,6 @@ DIRS += [
 ]
 
 DevToolsModules(
+    'command-state.js',
     'templater.js'
 )