Bug 1295008 - Hide the previous eyedropper when we request to show a new one; r=miker draft
authorPatrick Brosset <pbrosset@mozilla.com>
Wed, 24 Aug 2016 10:29:49 +0200
changeset 404846 005ad4e22db0c18262ec1ef120f583ce846b3d44
parent 403843 205f92dede4de933d13c19af1b65eb8383a56af8
child 404859 b3640cf03f57708a93ab5da720bd21929f0343da
push id27323
push userpbrosset@mozilla.com
push dateWed, 24 Aug 2016 08:32:06 +0000
reviewersmiker
bugs1295008
milestone51.0a1
Bug 1295008 - Hide the previous eyedropper when we request to show a new one; r=miker The eyedropper can be shown in 2 distinct ways: - the 'eyedropper' gcli command can be called (from the dev toolboar or from the browser devtools menu), - or the inspector's 'pickColorFromPage' method can be called (from the inspector toolbar or from the color-picker in the ruleview). Before this change, it was possible to show several eyedropper because these 2 codepaths didn't know about each other. Now, when executing the gcli command, the inspector's 'cancelPickColorFromPage' method is called to hide it first. And, when the 'pickColorFromPage' method is called, the 'eyedropper --hide' gcli command is called too. This way, there's only one eyedropper shown. There was also a problem where the gcli command would create a new eyedropper everytime it was called. This was fixed too, by maintaining a WeakMap of all eyedroppers opened so far. MozReview-Commit-ID: F6fBP5R7ZTJ
devtools/client/inspector/inspector-commands.js
devtools/client/inspector/inspector-panel.js
devtools/client/shared/widgets/Tooltip.js
devtools/server/actors/highlighters/eye-dropper.js
devtools/shared/fronts/inspector.js
--- a/devtools/client/inspector/inspector-commands.js
+++ b/devtools/client/inspector/inspector-commands.js
@@ -6,16 +6,18 @@
 
 const l10n = require("gcli/l10n");
 const {gDevTools} = require("devtools/client/framework/devtools");
 /* eslint-disable mozilla/reject-some-requires */
 const {EyeDropper, HighlighterEnvironment} = require("devtools/server/actors/highlighters");
 /* eslint-enable mozilla/reject-some-requires */
 const Telemetry = require("devtools/client/shared/telemetry");
 
+const windowEyeDroppers = new WeakMap();
+
 exports.items = [{
   item: "command",
   runAt: "client",
   name: "inspect",
   description: l10n.lookup("inspectDesc"),
   manual: l10n.lookup("inspectManual"),
   params: [
     {
@@ -43,33 +45,70 @@ exports.items = [{
     // This hidden parameter is only set to true when the eyedropper browser menu item is
     // used. It is useful to log a different telemetry event whether the tool was used
     // from the menu, or from the gcli command line.
     group: "hiddengroup",
     params: [{
       name: "frommenu",
       type: "boolean",
       hidden: true
+    }, {
+      name: "hide",
+      type: "boolean",
+      hidden: true
     }]
   }],
-  exec: function (args, context) {
+  exec: function* (args, context) {
+    if (args.hide) {
+      context.updateExec("eyedropper_server_hide").catch(e => console.error(e));
+      return;
+    }
+
+    // If the inspector is already picking a color from the page, cancel it.
+    let target = context.environment.target;
+    let toolbox = gDevTools.getToolbox(target);
+    if (toolbox) {
+      let inspector = toolbox.getPanel("inspector");
+      if (inspector) {
+        yield inspector.hideEyeDropper();
+      }
+    }
+
     let telemetry = new Telemetry();
     telemetry.toolOpened(args.frommenu ? "menueyedropper" : "eyedropper");
     context.updateExec("eyedropper_server").catch(e => console.error(e));
   }
 }, {
   item: "command",
   runAt: "server",
   name: "eyedropper_server",
   hidden: true,
   exec: function (args, {environment}) {
-    let env = new HighlighterEnvironment();
-    env.initFromWindow(environment.window);
-    let eyeDropper = new EyeDropper(env);
+    let eyeDropper = windowEyeDroppers.get(environment.window);
+
+    if (!eyeDropper) {
+      let env = new HighlighterEnvironment();
+      env.initFromWindow(environment.window);
+
+      eyeDropper = new EyeDropper(env);
+      eyeDropper.once("hidden", () => {
+        eyeDropper.destroy();
+        env.destroy();
+        windowEyeDroppers.delete(environment.window);
+      });
+
+      windowEyeDroppers.set(environment.window, eyeDropper);
+    }
 
     eyeDropper.show(environment.document.documentElement, {copyOnSelect: true});
-
-    eyeDropper.once("hidden", () => {
-      eyeDropper.destroy();
-      env.destroy();
-    });
+  }
+}, {
+  item: "command",
+  runAt: "server",
+  name: "eyedropper_server_hide",
+  hidden: true,
+  exec: function (args, {environment}) {
+    let eyeDropper = windowEyeDroppers.get(environment.window);
+    if (eyeDropper) {
+      eyeDropper.hide();
+    }
   }
 }];
--- a/devtools/client/inspector/inspector-panel.js
+++ b/devtools/client/inspector/inspector-panel.js
@@ -1299,17 +1299,17 @@ InspectorPanel.prototype = {
   /**
    * Show the eyedropper on the page.
    * @return {Promise} resolves when the eyedropper is visible.
    */
   showEyeDropper: function () {
     this.telemetry.toolOpened("toolbareyedropper");
     this.eyeDropperButton.setAttribute("checked", "true");
     this.startEyeDropperListeners();
-    return this.inspector.pickColorFromPage({copyOnSelect: true})
+    return this.inspector.pickColorFromPage(this.toolbox, {copyOnSelect: true})
                          .catch(e => console.error(e));
   },
 
   /**
    * Hide the eyedropper.
    * @return {Promise} resolves when the eyedropper is hidden.
    */
   hideEyeDropper: function () {
--- a/devtools/client/shared/widgets/Tooltip.js
+++ b/devtools/client/shared/widgets/Tooltip.js
@@ -771,34 +771,34 @@ Heritage.extend(SwatchBasedEditorTooltip
         this.commit();
       }
     }
   },
 
   _openEyeDropper: function () {
     let {inspector, toolbox, telemetry} = this.inspector;
     telemetry.toolOpened("pickereyedropper");
-    inspector.pickColorFromPage({copyOnSelect: false}).catch(e => console.error(e));
+    inspector.pickColorFromPage(toolbox, {copyOnSelect: false}).then(() => {
+      this.eyedropperOpen = true;
+
+      // close the colorpicker tooltip so that only the eyedropper is open.
+      this.hide();
+
+      this.tooltip.emit("eyedropper-opened");
+    }, e => console.error(e));
 
     inspector.once("color-picked", color => {
       toolbox.win.focus();
       this._selectColor(color);
       this._onEyeDropperDone();
     });
 
     inspector.once("color-pick-canceled", () => {
       this._onEyeDropperDone();
     });
-
-    this.eyedropperOpen = true;
-
-    // close the colorpicker tooltip so that only the eyedropper is open.
-    this.hide();
-
-    this.tooltip.emit("eyedropper-opened");
   },
 
   _onEyeDropperDone: function () {
     this.eyedropperOpen = false;
     this.activeSwatch = null;
   },
 
   _colorToRgba: function (color) {
--- a/devtools/server/actors/highlighters/eye-dropper.js
+++ b/devtools/server/actors/highlighters/eye-dropper.js
@@ -173,16 +173,18 @@ EyeDropper.prototype = {
     pageListenerTarget.removeEventListener("mousemove", this);
     pageListenerTarget.removeEventListener("click", this);
     pageListenerTarget.removeEventListener("keydown", this);
     pageListenerTarget.removeEventListener("DOMMouseScroll", this);
     pageListenerTarget.removeEventListener("FullZoomChange", this);
 
     this.getElement("root").setAttribute("hidden", "true");
     this.getElement("root").removeAttribute("drawn");
+
+    this.emit("hidden");
   },
 
   prepareImageCapture() {
     // Get the image data from the content window.
     let imageData = getWindowAsImageData(this.win);
 
     // We need to transform imageData to something drawWindow will consume. An ImageBitmap
     // works well. We could have used an Image, but doing so results in errors if the page
--- a/devtools/shared/fronts/inspector.js
+++ b/devtools/shared/fronts/inspector.js
@@ -21,16 +21,18 @@ const {
 } = require("devtools/shared/specs/inspector");
 const promise = require("promise");
 const defer = require("devtools/shared/defer");
 const { Task } = require("devtools/shared/task");
 const { Class } = require("sdk/core/heritage");
 const events = require("sdk/event/core");
 const object = require("sdk/util/object");
 const nodeConstants = require("devtools/shared/dom-node-constants.js");
+loader.lazyRequireGetter(this, "CommandUtils",
+  "devtools/client/shared/developer-toolbar", true);
 
 const HIDDEN_CLASS = "__fx-devtools-hide-shortcut__";
 
 /**
  * Convenience API for building a list of attribute modifications
  * for the `modifyAttributes` request.
  */
 const AttributeModificationList = Class({
@@ -972,12 +974,28 @@ var InspectorFront = FrontClass(inspecto
         return pageStyle;
       }
       return this.getWalker().then(() => {
         return pageStyle;
       });
     });
   }, {
     impl: "_getPageStyle"
+  }),
+
+  pickColorFromPage: custom(Task.async(function* (toolbox, options) {
+    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");
+    }
+
+    yield this._pickColorFromPage(options);
+  }), {
+    impl: "_pickColorFromPage"
   })
 });
 
 exports.InspectorFront = InspectorFront;