Bug 1320149 - Prevent loading gcli when opening a toolbox. r=jwalker,miker

MozReview-Commit-ID: KE209SRA15u

--HG--
extra : rebase_source : aa6f8bcc9918b1f192e31b01cf4ecd37b892a49d

devtools/client/definitions.js

@@ -24,6 +24,11 @@ loader.lazyGetter(this, "StoragePanel", () => require("devtools/client/storage/p
 loader.lazyGetter(this, "ScratchpadPanel", () => require("devtools/client/scratchpad/scratchpad-panel").ScratchpadPanel);
 loader.lazyGetter(this, "DomPanel", () => require("devtools/client/dom/dom-panel").DomPanel);
 
+// Other dependencies
+loader.lazyRequireGetter(this, "CommandUtils", "devtools/client/shared/developer-toolbar", true);
+loader.lazyImporter(this, "ResponsiveUIManager", "resource://devtools/client/responsivedesign/responsivedesign.jsm");
+loader.lazyImporter(this, "ScratchpadManager", "resource://devtools/client/scratchpad/scratchpad-manager.jsm");
+
 const {LocalizationHelper} = require("devtools/shared/l10n");
 const L10N = new LocalizationHelper("devtools/client/locales/startup.properties");
 
@@ -470,26 +475,93 @@ exports.defaultThemes = [
 // addons that have manually inserted toolbarbuttons into DOM.
 // (By default, supported target is only local tab)
 exports.ToolboxButtons = [
-  { id: "command-button-pick",
-    isTargetSupported: target => {
-      return target.activeTab && target.activeTab.traits.frames;
-    }
-  },
-  { id: "command-button-frames",
-    isTargetSupported: target => {
-      return target.activeTab && target.activeTab.traits.frames;
-    }
-  },
   { id: "command-button-splitconsole",
-    isTargetSupported: target => !target.isAddon },
-  { id: "command-button-responsive" },
-  { id: "command-button-paintflashing" },
-  { id: "command-button-scratchpad" },
-  { id: "command-button-screenshot" },
-  { id: "command-button-rulers" },
-  { id: "command-button-measure" },
-  { id: "command-button-noautohide",
-    isTargetSupported: target => target.chrome },
+    description: l10n("toolbox.buttons.splitconsole"),
+    isTargetSupported: target => !target.isAddon,
+    onClick(event, toolbox) {
+      toolbox.toggleSplitConsole();
+    },
+    isChecked(toolbox) {
+      return toolbox.splitConsole;
+    },
+    setup(toolbox, onChange) {
+      toolbox.on("split-console", onChange);
+    },
+    teardown(toolbox, onChange) {
+      toolbox.off("split-console", onChange);
+    }
+  },
+  { id: "command-button-paintflashing",
+    description: l10n("toolbox.buttons.paintflashing"),
+    isTargetSupported: target => target.isLocalTab,
+    onClick(event, toolbox) {
+      CommandUtils.executeOnTarget(toolbox.target, "paintflashing toggle");
+    },
+    autoToggle: true
+  },
+  { id: "command-button-scratchpad",
+    description: l10n("toolbox.buttons.scratchpad"),
+    isTargetSupported: target => target.isLocalTab,
+    onClick(event, toolbox) {
+      ScratchpadManager.openScratchpad();
+    }
+  },
+  { id: "command-button-responsive",
+    description: l10n("toolbox.buttons.responsive",
+                      osString == "Darwin" ? "Cmd+Opt+M" : "Ctrl+Shift+M"),
+    isTargetSupported: target => target.isLocalTab,
+    onClick(event, toolbox) {
+      let browserWindow = toolbox.win.top;
+      ResponsiveUIManager.handleGcliCommand(browserWindow,
+        browserWindow.gBrowser.selectedTab,
+        "resize toggle",
+        null);
+    },
+    isChecked(toolbox) {
+      if (!toolbox.target.tab) {
+        return false;
+      }
+      return ResponsiveUIManager.isActiveForTab(toolbox.target.tab);
+    },
+    setup(toolbox, onChange) {
+      ResponsiveUIManager.on("on", onChange);
+      ResponsiveUIManager.on("off", onChange);
+    },
+    teardown(toolbox, onChange) {
+      ResponsiveUIManager.off("on", onChange);
+      ResponsiveUIManager.off("off", onChange);
+    }
+  },
+  { id: "command-button-screenshot",
+    description: l10n("toolbox.buttons.screenshot"),
+    isTargetSupported: target => target.isLocalTab,
+    onClick(event, toolbox) {
+      // Special case for screenshot button to check for clipboard preference
+      const clipboardEnabled = Services.prefs
+        .getBoolPref("devtools.screenshot.clipboard.enabled");
+      let args = "--fullpage --file";
+      if (clipboardEnabled) {
+        args += " --clipboard";
+      }
+      CommandUtils.executeOnTarget(toolbox.target, "screenshot " + args);
+    }
+  },
+  { id: "command-button-rulers",
+    description: l10n("toolbox.buttons.rulers"),
+    isTargetSupported: target => target.isLocalTab,
+    onClick(event, toolbox) {
+      CommandUtils.executeOnTarget(toolbox.target, "rulers");
+    },
+    autoToggle: true
+  },
+  { id: "command-button-measure",
+    description: l10n("toolbox.buttons.measure"),
+    isTargetSupported: target => target.isLocalTab,
+    onClick(event, toolbox) {
+      CommandUtils.executeOnTarget(toolbox.target, "measure");
+    },
+    autoToggle: true
+  },
 ];
 
 /**

devtools/client/framework/toolbox.js

@@ -558,23 +558,54 @@ Toolbox.prototype = {
    * @property {String} description - The value that will display as a tooltip and in
    *                    the options panel for enabling/disabling.
    * @property {Function} onClick - The function to run when the button is activated by
-   *                      click or keyboard shortcut.
+   *                      click or keyboard shortcut. First argument will be the 'click'
+   *                      event, and second argument is the toolbox instance.
    * @property {Boolean} isInStartContainer - Buttons can either be placed at the start
    *                     of the toolbar, or at the end.
+   * @property {Function} setup - Function run immediately to listen for events changing
+   *                      whenever the button is checked or unchecked. The toolbox object
+   *                      is passed as first argument and a callback is passed as second
+   *                       argument, to be called whenever the checked state changes.
+   * @property {Function} teardown - Function run on toolbox close to let a chance to
+   *                      unregister listeners set when `setup` was called and avoid
+   *                      memory leaks. The same arguments than `setup` function are
+   *                      passed to `teardown`.
+   * @property {Function} isTargetSupported - Function to automatically enable/disable
+   *                      the button based on the target. If the target don't support
+   *                      the button feature, this method should return false.
+   * @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 isChecked = false;
-    const {id, className, description, onClick, isInStartContainer} = options;
+    let isCheckedValue = false;
+    const { id, className, description, onClick, isInStartContainer, setup, teardown,
+            isTargetSupported, isChecked, autoToggle } = options;
+    const toolbox = this;
     const button = {
       id,
       className,
       description,
-      onClick,
+      onClick(event) {
+        if (typeof onClick == "function") {
+          onClick(event, toolbox);
+        }
+        if (autoToggle) {
+          button.isChecked = !button.isChecked;
+        }
+      },
+      isTargetSupported,
       get isChecked() {
-        return isChecked;
+        if (typeof isChecked == "function") {
+          return isChecked(toolbox);
+        }
+        return isCheckedValue;
       },
       set isChecked(value) {
-        isChecked = value;
+        // Note that if options.isChecked is given, this is ignored
+        isCheckedValue = value;
         this.emit("updatechecked");
       },
       // The preference for having this button visible.
@@ -583,6 +614,17 @@ Toolbox.prototype = {
       // holding buttons. By default the buttons are placed in the end container.
       isInStartContainer: !!isInStartContainer
     };
+    if (typeof setup == "function") {
+      let onChange = () => {
+        button.emit("updatechecked");
+      };
+      setup(this, onChange);
+      // Save a reference to the cleanup method that will unregister the onChange
+      // callback. Immediately bind the function argument so that we don't have to
+      // also save a reference to them.
+      button.teardown = teardown.bind(options, this, onChange);
+    }
+    button.isVisible = this._commandIsVisible(button);
 
     EventEmitter.decorate(button);
 
@@ -1030,7 +1072,7 @@ Toolbox.prototype = {
   },
 
   /**
-   * Add buttons to the UI as specified in the devtools.toolbox.toolbarSpec pref
+   * Add buttons to the UI as specified in devtools/client/definitions.js
    */
   _buildButtons: Task.async(function* () {
     // Beyond the normal preference filtering
@@ -1040,27 +1082,9 @@ Toolbox.prototype = {
       yield this._buildNoAutoHideButton()
     ];
 
-    // Build buttons from the GCLI commands only if the GCLI actor is supported and the
-    // target isn't chrome.
-    if (this.target.hasActor("gcli") && !this.target.chrome) {
-      const options = {
-        environment: CommandUtils.createEnvironment(this, "_target")
-      };
-
-      this._requisition = yield CommandUtils.createRequisition(this.target, options);
-      const spec = this.getToolbarSpec();
-      const commandButtons = yield CommandUtils.createCommandButtons(
-        spec, this.target, this.doc, this._requisition, this._createButtonState);
-      this.toolbarButtons = [...this.toolbarButtons, ...commandButtons];
-    }
-
-    // Mutate the objects here with their visibility.
-    this.toolbarButtons.forEach(command => {
-      const definition = ToolboxButtons.find(t => t.id === command.id);
-      command.isTargetSupported = definition.isTargetSupported
-        ? definition.isTargetSupported
-        : target => target.isLocalTab;
-      command.isVisible = this._commandIsVisible(command.id);
+    ToolboxButtons.forEach(definition => {
+      let button = this._createButtonState(definition);
+      this.toolbarButtons.push(button);
     });
 
     this.component.setToolboxButtons(this.toolbarButtons);
@@ -1073,7 +1097,10 @@ Toolbox.prototype = {
     this.frameButton = this._createButtonState({
       id: "command-button-frames",
       description: L10N.getStr("toolbox.frames.tooltip"),
-      onClick: this.showFramesMenu
+      onClick: this.showFramesMenu,
+      isTargetSupported: target => {
+        return target.activeTab && target.activeTab.traits.frames;
+      }
     });
 
     return this.frameButton;
@@ -1087,7 +1114,8 @@ Toolbox.prototype = {
     this.autohideButton = this._createButtonState({
       id: "command-button-noautohide",
       description: L10N.getStr("toolbox.noautohide.tooltip"),
-      onClick: this._toggleNoAutohide
+      onClick: this._toggleNoAutohide,
+      isTargetSupported: target => target.chrome
     });
 
     this._isDisableAutohideEnabled().then(enabled => {
@@ -1138,7 +1166,10 @@ Toolbox.prototype = {
       id: "command-button-pick",
       description: L10N.getStr("pickButton.tooltip"),
       onClick: this._onPickerClick,
-      isInStartContainer: true
+      isInStartContainer: true,
+      isTargetSupported: target => {
+        return target.activeTab && target.activeTab.traits.frames;
+      }
     });
 
     return this.pickerButton;
@@ -1178,16 +1209,7 @@ Toolbox.prototype = {
   */
   getToolbarSpec: function () {
     let spec = CommandUtils.getCommandbarSpec("devtools.toolbox.toolbarSpec");
-    // Special case for screenshot command button to check for clipboard preference
-    const clipboardEnabled = Services.prefs
-      .getBoolPref("devtools.screenshot.clipboard.enabled");
-    if (clipboardEnabled) {
-      for (let i = 0; i < spec.length; i++) {
-        if (spec[i] == "screenshot --fullpage --file") {
-          spec[i] += " --clipboard";
-        }
-      }
-    }
+
     return spec;
   },
 
@@ -1199,8 +1221,8 @@ Toolbox.prototype = {
    * Update the visibility of the buttons.
    */
   updateToolboxButtonsVisibility() {
-    this.toolbarButtons.forEach(command => {
-      command.isVisible = this._commandIsVisible(command.id);
+    this.toolbarButtons.forEach(button => {
+      button.isVisible = this._commandIsVisible(button);
     });
     this.component.setToolboxButtons(this.toolbarButtons);
   },
@@ -1208,11 +1230,11 @@ Toolbox.prototype = {
   /**
    * Ensure the visibility of each toolbox button matches the preference value.
    */
-  _commandIsVisible: function (id) {
+  _commandIsVisible: function (button) {
     const {
       isTargetSupported,
       visibilityswitch
-    } = this.toolbarButtons.find(btn => btn.id === id);
+    } = button;
 
     let visible = true;
     try {
@@ -2318,12 +2340,17 @@ Toolbox.prototype = {
     detachThread(this._threadClient);
     this._threadClient = null;
 
+    // Unregister buttons listeners
+    this.toolbarButtons.forEach(button => {
+      if (typeof button.teardown == "function") {
+        // teardown arguments have already been bound in _createButtonState
+        button.teardown();
+      }
+    });
+
     // We need to grab a reference to win before this._host is destroyed.
     let win = this.win;
 
-    if (this._requisition) {
-      CommandUtils.destroyRequisition(this._requisition, this.target);
-    }
     this._telemetry.toolClosed("toolbox");
     this._telemetry.destroy();
 

devtools/client/inspector/inspector.js

@@ -1755,14 +1755,11 @@ Inspector.prototype = {
     const command = Services.prefs.getBoolPref("devtools.screenshot.clipboard.enabled") ?
       "screenshot --file --clipboard --selector" :
       "screenshot --file --selector";
-    CommandUtils.createRequisition(this._target, {
-      environment: CommandUtils.createEnvironment(this, "_target")
-    }).then(requisition => {
-      // Bug 1180314 -  CssSelector might contain white space so need to make sure it is
-      // passed to screenshot as a single parameter.  More work *might* be needed if
-      // CssSelector could contain escaped single- or double-quotes, backslashes, etc.
-      requisition.updateExec(`${command} '${this.selectionCssSelector}'`);
-    });
+    // Bug 1180314 -  CssSelector might contain white space so need to make sure it is
+    // passed to screenshot as a single parameter.  More work *might* be needed if
+    // CssSelector could contain escaped single- or double-quotes, backslashes, etc.
+    CommandUtils.executeOnTarget(this._target,
+      `${command} '${this.selectionCssSelector}'`);
   },
 
   /**

devtools/client/locales/en-US/startup.properties

@@ -260,3 +260,40 @@ dom.accesskey=D
 # displayed inside the developer tools window.
 # Keyboard shortcut for DOM panel will be shown inside the brackets.
 dom.tooltip=DOM (%S)
+
+# LOCALIZATION NOTE (toolbox.buttons.splitconsole):
+# This is the tooltip of the button in the toolbox toolbar used to toggle
+# the split console.
+# Keyboard shortcut will be shown inside brackets.
+toolbox.buttons.splitconsole = Toggle split console (%S)
+
+# LOCALIZATION NOTE (toolbox.buttons.responsive):
+# This is the tooltip of the button in the toolbox toolbar that toggles
+# the Responsive mode.
+# Keyboard shortcut will be shown inside brackets.
+toolbox.buttons.responsive = Responsive Design Mode (%S)
+
+# LOCALIZATION NOTE (toolbox.buttons.paintflashing):
+# This is the tooltip of the paintflashing button in the toolbox toolbar
+# that toggles paintflashing.
+toolbox.buttons.paintflashing = Toggle paint flashing
+
+# LOCALIZATION NOTE (toolbox.buttons.scratchpad):
+# This is the tooltip of the button in the toolbox toolbar that opens
+# the scratchpad window
+toolbox.buttons.scratchpad = Scratchpad
+
+# LOCALIZATION NOTE (toolbox.buttons.screenshot):
+# This is the tooltip of the button in the toolbox toolbar that allows you to
+# take a screenshot of the entire page
+toolbox.buttons.screenshot = Take a screenshot of the entire page
+
+# LOCALIZATION NOTE (toolbox.buttons.rulers):
+# This is the tooltip of the button in the toolbox toolbar that toggles the
+# rulers in the page
+toolbox.buttons.rulers = Toggle rulers for the page
+
+# LOCALIZATION NOTE (toolbox.buttons.measure):
+# This is the tooltip of the button in the toolbox toolbar that toggles the
+# measuring tools
+toolbox.buttons.measure = Measure a portion of the page

devtools/client/menus.js

@@ -148,11 +148,7 @@ exports.menuitems = [
       let window = event.target.ownerDocument.defaultView;
       let target = TargetFactory.forTab(window.gBrowser.selectedTab);
 
-      CommandUtils.createRequisition(target, {
-        environment: CommandUtils.createEnvironment({target})
-      }).then(requisition => {
-        requisition.updateExec("eyedropper --frommenu");
-      }, e => console.error(e));
+      CommandUtils.executeOnTarget(target, "eyedropper --frommenu");
     },
     checkbox: true
   },

devtools/client/preferences/devtools.js

@@ -30,7 +30,6 @@ pref("devtools.toolbox.sidebar.width", 500);
 pref("devtools.toolbox.host", "bottom");
 pref("devtools.toolbox.previousHost", "side");
 pref("devtools.toolbox.selectedTool", "webconsole");
-pref("devtools.toolbox.toolbarSpec", '["splitconsole", "paintflashing toggle","scratchpad","resize toggle","screenshot --fullpage --file", "rulers", "measure"]');
 pref("devtools.toolbox.sideEnabled", true);
 pref("devtools.toolbox.zoomValue", "1");
 pref("devtools.toolbox.splitconsoleEnabled", false);

devtools/client/shared/developer-toolbar.js

@@ -35,6 +35,28 @@ loader.lazyRequireGetter(this, "EventEmitter", "devtools/shared/event-emitter");
  * A collection of utilities to help working with commands
  */
 var CommandUtils = {
+  /**
+   * Caches requisitions created when calling executeOnTarget:
+   * Target => Requisition Promise
+   */
+  _requisitions: new WeakMap(),
+
+  /**
+   * Utility to execute a command string on a given target
+   */
+  executeOnTarget: Task.async(function* (target, command) {
+    let requisitionPromise = this._requisitions.get(target);
+    if (!requisitionPromise) {
+      requisitionPromise = this.createRequisition(target, {
+        environment: CommandUtils.createEnvironment({ target }, "target")
+      });
+      // Store the promise to avoid races by storing the promise immediately
+      this._requisitions.set(target, requisitionPromise);
+    }
+    let requisition = yield requisitionPromise;
+    requisition.updateExec(command);
+  }),
+
   /**
    * Utility to ensure that things are loaded in the correct order
    */

devtools/client/shared/test/browser_telemetry_button_paintflashing.js

@@ -44,16 +44,13 @@ function* delayedClicks(toolbox, node, clicks) {
       setTimeout(() => resolve(), TOOL_DELAY);
     });
 
-    // this event will fire once the command execution starts and
-    // the output object is created
-    let clicked = toolbox._requisition.commandOutputManager.onOutput.once();
+    let PaintFlashingCmd = require("devtools/shared/gcli/commands/paintflashing");
+    let clicked = PaintFlashingCmd.eventEmitter.once("changed");
 
     info("Clicking button " + node.id);
     node.click();
 
-    let outputEvent = yield clicked;
-    // promise gets resolved once execution finishes and output is ready
-    yield outputEvent.output.promise;
+    yield clicked;
   }
 }
 

devtools/shared/fronts/inspector.js

@@ -991,11 +991,7 @@ var InspectorFront = FrontClassWithSpec(inspectorSpec, {
     if (toolbox) {
       // If the eyedropper was already started using the gcli command, hide it so we don't
       // end up with 2 instances of the eyedropper on the page.
-      let {target} = toolbox;
-      let requisition = yield CommandUtils.createRequisition(target, {
-        environment: CommandUtils.createEnvironment({target})
-      });
-      yield requisition.updateExec("eyedropper --hide");
+      CommandUtils.executeOnTarget(toolbox.target, "eyedropper --hide");
     }
 
     yield this._pickColorFromPage(options);

devtools/shared/gcli/commands/paintflashing.js

@@ -19,6 +19,9 @@ try {
 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");