Bug 1295008 - Hide the previous eyedropper when we request to show a new one; r=miker a=ritu
authorPatrick Brosset <pbrosset@mozilla.com>
Wed, 24 Aug 2016 10:29:49 +0200
changeset 349984 1bc4cc8271058198af1652b2fdf2445a0fdfabe5
parent 349983 21e634320cc7519d011ca8e28dbb8be116388897
child 349985 1b935d2bbf0b7dc4d58dd176e28f06bb177b558e
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmiker, ritu
bugs1295008
milestone50.0a2
Bug 1295008 - Hide the previous eyedropper when we request to show a new one; r=miker a=ritu 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");
 loader.lazyRequireGetter(this, "gDevTools", "devtools/client/framework/devtools", true);
 /* 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
@@ -1305,17 +1305,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
@@ -840,34 +840,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 page as an image.
     let imageData = getWindowAsImageData(this.win);
     let image = new this.win.Image();
     image.src = imageData;
 
--- 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 = FrontClassWithSpec(
         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;