Bug 1167174 - Assert that custom highlighter do show the highlighter before proceeding. r=pbrosset
authorAlexandre Poirot <poirot.alex@gmail.com>
Mon, 25 May 2015 18:54:08 +0200
changeset 245592 1c0ecccc191530210d6a69405dcbfe2fedb5a125
parent 245591 ddd02b236cc273090f793a682c9d0552076f9557
child 245593 0e6d986389d26b49124057d243909a7ea40d418f
push id13162
push userapoirot@mozilla.com
push dateTue, 26 May 2015 14:49:07 +0000
treeherderfx-team@1c0ecccc1915 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbrosset
bugs1167174
milestone41.0a1
Bug 1167174 - Assert that custom highlighter do show the highlighter before proceeding. r=pbrosset
browser/devtools/styleinspector/style-inspector-overlays.js
browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-02.js
browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-03.js
browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-04.js
toolkit/devtools/server/actors/highlighter.js
--- a/browser/devtools/styleinspector/style-inspector-overlays.js
+++ b/browser/devtools/styleinspector/style-inspector-overlays.js
@@ -16,16 +16,17 @@ const {Cc, Ci, Cu} = require("chrome");
 const {
   Tooltip,
   SwatchColorPickerTooltip,
   SwatchCubicBezierTooltip,
   CssDocsTooltip,
   SwatchFilterTooltip
 } = require("devtools/shared/widgets/Tooltip");
 const {CssLogic} = require("devtools/styleinspector/css-logic");
+const EventEmitter = require("devtools/toolkit/event-emitter");
 const {Promise:promise} = Cu.import("resource://gre/modules/Promise.jsm", {});
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 
 const PREF_IMAGE_TOOLTIP_SIZE = "devtools.inspector.imagePreviewTooltipSize";
 
 // Types of existing tooltips
 const TOOLTIP_IMAGE_TYPE = "image";
@@ -54,16 +55,18 @@ function HighlightersOverlay(view) {
   this._onMouseLeave = this._onMouseLeave.bind(this);
 
   this.promises = {};
   this.highlighters = {};
 
   // Only initialize the overlay if at least one of the highlighter types is
   // supported
   this.supportsHighlighters = this.highlighterUtils.supportsCustomHighlighters();
+
+  EventEmitter.decorate(this);
 }
 
 exports.HighlightersOverlay = HighlightersOverlay;
 
 HighlightersOverlay.prototype = {
   /**
    * Add the highlighters overlay to the view. This will start tracking mouse
    * movements and display highlighters when needed
@@ -119,19 +122,23 @@ HighlightersOverlay.prototype = {
     if (this._isRuleViewTransform(nodeInfo) ||
         this._isComputedViewTransform(nodeInfo)) {
       type = "CssTransformHighlighter";
     }
 
     if (type) {
       this.highlighterShown = type;
       let node = this.view.inspector.selection.nodeFront;
-      this._getHighlighter(type).then(highlighter => {
-        highlighter.show(node);
-      });
+      this._getHighlighter(type)
+          .then(highlighter => highlighter.show(node))
+          .then(shown => {
+            if (shown) {
+              this.emit("highlighter-shown");
+            }
+          });
     }
   },
 
   _onMouseLeave: function(event) {
     this._lastHovered = null;
     this._hideCurrent();
   },
 
@@ -171,16 +178,17 @@ HighlightersOverlay.prototype = {
         // promise. This causes some tests to fail when trying to install a
         // rejection handler on the result of the call. To avoid this, check
         // whether the result is truthy before installing the handler.
         let promise = highlighter.hide();
         if (promise) {
           promise.then(null, Cu.reportError);
         }
         this.highlighterShown = null;
+        this.emit("highlighter-hidden");
       });
     }
   },
 
   /**
    * Get a highlighter front given a type. It will only be initialized once
    * @param {String} type The highlighter type. One of this.highlighters
    * @return a promise that resolves to the highlighter
--- a/browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-02.js
+++ b/browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-02.js
@@ -31,17 +31,19 @@ add_task(function*() {
   info("Faking a mousemove on a non-transform property");
   let {valueSpan} = getRuleViewProperty(rView, "body", "color");
   hs._onMouseMove({target: valueSpan});
   ok(!hs.highlighters[TYPE], "No highlighter exists in the rule-view (2)");
   ok(!hs.promises[TYPE], "No highlighter is being created in the rule-view (2)");
 
   info("Faking a mousemove on a transform property");
   ({valueSpan} = getRuleViewProperty(rView, "body", "transform"));
+  let onHighlighterShown = hs.once("highlighter-shown");
   hs._onMouseMove({target: valueSpan});
+  yield onHighlighterShown;
   ok(hs.promises[TYPE], "The highlighter is being initialized");
   let h = yield hs.promises[TYPE];
   is(h, hs.highlighters[TYPE], "The initialized highlighter is the right one");
 
   let onComputedViewReady = inspector.once("computed-view-refreshed");
   let {view: cView} = yield openComputedView();
   yield onComputedViewReady;
   hs = cView.highlighters;
@@ -52,13 +54,15 @@ add_task(function*() {
   info("Faking a mousemove on a non-transform property");
   ({valueSpan} = getComputedViewProperty(cView, "color"));
   hs._onMouseMove({target: valueSpan});
   ok(!hs.highlighters[TYPE], "No highlighter exists in the computed-view (2)");
   ok(!hs.promises[TYPE], "No highlighter is being created in the computed-view (2)");
 
   info("Faking a mousemove on a transform property");
   ({valueSpan} = getComputedViewProperty(cView, "transform"));
+  onHighlighterShown = hs.once("highlighter-shown");
   hs._onMouseMove({target: valueSpan});
+  yield onHighlighterShown;
   ok(hs.promises[TYPE], "The highlighter is being initialized");
   h = yield hs.promises[TYPE];
   is(h, hs.highlighters[TYPE], "The initialized highlighter is the right one");
 });
--- a/browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-03.js
+++ b/browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-03.js
@@ -34,58 +34,69 @@ add_task(function*() {
   let HighlighterFront = {
     isShown: false,
     nodeFront: null,
     nbOfTimesShown: 0,
     show: function(nodeFront) {
       this.nodeFront = nodeFront;
       this.isShown = true;
       this.nbOfTimesShown ++;
+      return promise.resolve(true);
     },
     hide: function() {
       this.nodeFront = null;
       this.isShown = false;
+      return promise.resolve();
     }
   };
 
   // Inject the mock highlighter in the rule-view
-  rView.highlighters.promises[TYPE] = {
-    then: function(cb) {
-      cb(HighlighterFront);
-    }
-  };
+  let hs = rView.highlighters;
+  hs.promises[TYPE] = promise.resolve(HighlighterFront);
 
   let {valueSpan} = getRuleViewProperty(rView, "body", "transform");
 
   info("Checking that the HighlighterFront's show/hide methods are called");
-  rView.highlighters._onMouseMove({target: valueSpan});
+  let onHighlighterShown = hs.once("highlighter-shown");
+  hs._onMouseMove({target: valueSpan});
+  yield onHighlighterShown;
   ok(HighlighterFront.isShown, "The highlighter is shown");
-  rView.highlighters._onMouseLeave();
+  let onHighlighterHidden = hs.once("highlighter-hidden");
+  hs._onMouseLeave();
+  yield onHighlighterHidden;
   ok(!HighlighterFront.isShown, "The highlighter is hidden");
 
   info("Checking that hovering several times over the same property doesn't" +
     " show the highlighter several times");
   let nb = HighlighterFront.nbOfTimesShown;
-  rView.highlighters._onMouseMove({target: valueSpan});
+  onHighlighterShown = hs.once("highlighter-shown");
+  hs._onMouseMove({target: valueSpan});
+  yield onHighlighterShown;
   is(HighlighterFront.nbOfTimesShown, nb + 1, "The highlighter was shown once");
-  rView.highlighters._onMouseMove({target: valueSpan});
-  rView.highlighters._onMouseMove({target: valueSpan});
+  hs._onMouseMove({target: valueSpan});
+  hs._onMouseMove({target: valueSpan});
   is(HighlighterFront.nbOfTimesShown, nb + 1,
     "The highlighter was shown once, after several mousemove");
 
   info("Checking that the right NodeFront reference is passed");
   yield selectNode("html", inspector);
   ({valueSpan} = getRuleViewProperty(rView, "html", "transform"));
-  rView.highlighters._onMouseMove({target: valueSpan});
+  onHighlighterShown = hs.once("highlighter-shown");
+  hs._onMouseMove({target: valueSpan});
+  yield onHighlighterShown;
   is(HighlighterFront.nodeFront.tagName, "HTML",
     "The right NodeFront is passed to the highlighter (1)");
 
   yield selectNode("body", inspector);
   ({valueSpan} = getRuleViewProperty(rView, "body", "transform"));
-  rView.highlighters._onMouseMove({target: valueSpan});
+  onHighlighterShown = hs.once("highlighter-shown");
+  hs._onMouseMove({target: valueSpan});
+  yield onHighlighterShown;
   is(HighlighterFront.nodeFront.tagName, "BODY",
     "The right NodeFront is passed to the highlighter (2)");
 
   info("Checking that the highlighter gets hidden when hovering a non-transform property");
   ({valueSpan} = getRuleViewProperty(rView, "body", "color"));
-  rView.highlighters._onMouseMove({target: valueSpan});
+  onHighlighterHidden = hs.once("highlighter-hidden");
+  hs._onMouseMove({target: valueSpan});
+  yield onHighlighterHidden;
   ok(!HighlighterFront.isShown, "The highlighter is hidden");
 });
--- a/browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-04.js
+++ b/browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-04.js
@@ -50,13 +50,15 @@ add_task(function*() {
   info("Faking a mousemove on the disabled property");
   ({valueSpan} = getRuleViewProperty(rView, ".test", "transform"));
   hs._onMouseMove({target: valueSpan});
   ok(!hs.highlighters[TYPE], "No highlighter was created for the disabled property");
   ok(!hs.promises[TYPE], "And no highlighter is being initialized either");
 
   info("Faking a mousemove on the now unoverriden property");
   ({valueSpan} = getRuleViewProperty(rView, "div", "transform"));
+  let onHighlighterShown = hs.once("highlighter-shown");
   hs._onMouseMove({target: valueSpan});
+  yield onHighlighterShown;
   ok(hs.promises[TYPE], "The highlighter is being initialized now");
   let h = yield hs.promises[TYPE];
   is(h, hs.highlighters[TYPE], "The initialized highlighter is the right one");
 });
--- a/toolkit/devtools/server/actors/highlighter.js
+++ b/toolkit/devtools/server/actors/highlighter.js
@@ -2,17 +2,17 @@
  * 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 {Cu, Cc, Ci} = require("chrome");
 const Services = require("Services");
 const protocol = require("devtools/server/protocol");
-const {Arg, Option, method} = protocol;
+const {Arg, Option, method, RetVal} = protocol;
 const events = require("sdk/event/core");
 const Heritage = require("sdk/core/heritage");
 const {CssLogic} = require("devtools/styleinspector/css-logic");
 const EventEmitter = require("devtools/toolkit/event-emitter");
 const {setIgnoreLayoutChanges} = require("devtools/server/actors/layout");
 
 Cu.import("resource://gre/modules/devtools/LayoutHelpers.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
@@ -450,27 +450,31 @@ let CustomHighlighterActor = exports.Cus
    * NodeActor argument (NodeActor as in toolkit/devtools/server/actor/inspector).
    * Note however that some highlighters use this argument merely as a context
    * node: the RectHighlighter for instance uses it to calculate the absolute
    * position of the provided rect. The SelectHighlighter uses it as a base node
    * to run the provided CSS selector on.
    *
    * @param NodeActor The node to be highlighted
    * @param Object Options for the custom highlighter
+   * @return Boolean True, if the highlighter has been successfully shown (FF41+)
    */
   show: method(function(node, options) {
     if (!node || !isNodeValid(node.rawNode) || !this._highlighter) {
-      return;
+      return false;
     }
 
-    this._highlighter.show(node.rawNode, options);
+    return this._highlighter.show(node.rawNode, options);
   }, {
     request: {
       node: Arg(0, "domnode"),
       options: Arg(1, "nullable:json")
+    },
+    response: {
+      value: RetVal("nullable:boolean")
     }
   }),
 
   /**
    * Hide the highlighter if it was shown before
    */
   hide: method(function() {
     if (this._highlighter) {
@@ -844,28 +848,31 @@ AutoRefreshHighlighter.prototype = {
    * @param {Object} options
    *        Object used for passing options
    */
   show: function(node, options={}) {
     let isSameNode = node === this.currentNode;
     let isSameOptions = this._isSameOptions(options);
 
     if (!isNodeValid(node) || (isSameNode && isSameOptions)) {
-      return;
+      return false;
     }
 
     this.options = options;
 
     this._stopRefreshLoop();
     this.currentNode = node;
     this._updateAdjustedQuads();
     this._startRefreshLoop();
-    this._show();
-
-    this.emit("shown");
+
+    let shown = this._show();
+    if (shown) {
+      this.emit("shown");
+    }
+    return shown;
   },
 
   /**
    * Hide the highlighter
    */
   hide: function() {
     if (!isNodeValid(this.currentNode)) {
       return;
@@ -937,28 +944,31 @@ AutoRefreshHighlighter.prototype = {
     this._update();
     this.emit("updated");
   },
 
   _show: function() {
     // To be implemented by sub classes
     // When called, sub classes should actually show the highlighter for
     // this.currentNode, potentially using options in this.options
+    throw new Error("Custom highlighter class had to implement _show method");
   },
 
   _update: function() {
     // To be implemented by sub classes
     // When called, sub classes should update the highlighter shown for
     // this.currentNode
     // This is called as a result of a page scroll, zoom or repaint
+    throw new Error("Custom highlighter class had to implement _update method");
   },
 
   _hide: function() {
     // To be implemented by sub classes
     // When called, sub classes should actually hide the highlighter
+    throw new Error("Custom highlighter class had to implement _hide method");
   },
 
   _startRefreshLoop: function() {
     let win = getWindow(this.currentNode);
     this.rafID = win.requestAnimationFrame(this._startRefreshLoop.bind(this));
     this.rafWin = win;
     this.update();
   },
@@ -1224,19 +1234,20 @@ BoxModelHighlighter.prototype = Heritage
   /**
    * Show the highlighter on a given node
    */
   _show: function() {
     if (BOX_MODEL_REGIONS.indexOf(this.options.region) == -1)  {
       this.options.region = "content";
     }
 
-    this._update();
+    let shown = this._update();
     this._trackMutations();
     this.emit("ready");
+    return shown;
   },
 
   /**
    * Track the current node markup mutations so that the node info bar can be
    * updated to reflects the node's attributes
    */
   _trackMutations: function() {
     if (isNodeValid(this.currentNode)) {
@@ -1254,31 +1265,35 @@ BoxModelHighlighter.prototype = Heritage
   },
 
   /**
    * Update the highlighter on the current highlighted node (the one that was
    * passed as an argument to show(node)).
    * Should be called whenever node size or attributes change
    */
   _update: function() {
+    let shown = false;
     setIgnoreLayoutChanges(true);
 
     if (this._updateBoxModel()) {
       if (!this.options.hideInfoBar) {
         this._showInfobar();
       } else {
         this._hideInfobar();
       }
       this._showBoxModel();
+      shown = true;
     } else {
       // Nothing to highlight (0px rectangle like a <script> tag for instance)
       this._hide();
     }
 
     setIgnoreLayoutChanges(false, this.currentNode.ownerDocument.documentElement);
+
+    return shown;
   },
 
   /**
    * Hide the highlighter, the outline and the infobar.
    */
   _hide: function() {
     setIgnoreLayoutChanges(true);
 
@@ -1770,25 +1785,24 @@ CssTransformHighlighter.prototype = Heri
   },
 
   getElement: function(id) {
     return this.markup.getElement(this.ID_CLASS_PREFIX + id);
   },
 
   /**
    * Show the highlighter on a given node
-   * @param {DOMNode} node
    */
   _show: function() {
     if (!this._isTransformed(this.currentNode)) {
       this.hide();
-      return;
+      return false;
     }
 
-    this._update();
+    return this._update();
   },
 
   /**
    * Checks if the supplied node is transformed and not inline
    */
   _isTransformed: function(node) {
     let style = CssLogic.getComputedStyle(node);
     return style && (style.transform !== "none" && style.display !== "inline");
@@ -1825,17 +1839,17 @@ CssTransformHighlighter.prototype = Heri
   _update: function() {
     setIgnoreLayoutChanges(true);
 
     // Getting the points for the transformed shape
     let quads = this.currentQuads.border;
     if (!quads.length ||
         quads[0].bounds.width <= 0 || quads[0].bounds.height <= 0) {
       this._hideShapes();
-      return null;
+      return false;
     }
 
     let [quad] = quads;
 
     // Getting the points for the untransformed shape
     let untransformedQuad = this.layoutHelpers.getNodeBounds(this.currentNode);
 
     this._setPolygonPoints(quad, "transformed");
@@ -1845,16 +1859,17 @@ CssTransformHighlighter.prototype = Heri
     }
 
     // Adapt to the current zoom
     this.markup.scaleRootElement(this.currentNode, this.ID_CLASS_PREFIX + "root");
 
     this._showShapes();
 
     setIgnoreLayoutChanges(false, this.currentNode.ownerDocument.documentElement);
+    return true;
   },
 
   /**
    * Hide the highlighter, the outline and the infobar.
    */
   _hide: function() {
     setIgnoreLayoutChanges(true);
     this._hideShapes();
@@ -1893,17 +1908,17 @@ SelectorHighlighter.prototype = {
    * @param {Object} options Should at least contain the 'selector' option, a
    * string that will be used in querySelectorAll. On top of this, all of the
    * valid options to BoxModelHighlighter.show are also valid here.
    */
   show: function(node, options={}) {
     this.hide();
 
     if (!isNodeValid(node) || !options.selector) {
-      return;
+      return false;
     }
 
     let nodes = [];
     try {
       nodes = [...node.ownerDocument.querySelectorAll(options.selector)];
     } catch (e) {}
 
     delete options.selector;
@@ -1917,16 +1932,18 @@ SelectorHighlighter.prototype = {
       let highlighter = new BoxModelHighlighter(this.tabActor);
       if (options.fill) {
         highlighter.regionFill[options.region || "border"] = options.fill;
       }
       highlighter.show(matchingNode, options);
       this._highlighters.push(highlighter);
       i ++;
     }
+
+    return true;
   },
 
   hide: function() {
     for (let highlighter of this._highlighters) {
       highlighter.destroy();
     }
     this._highlighters = [];
   },
@@ -1993,26 +2010,26 @@ RectHighlighter.prototype = {
    * highlighter correctly.
    * @param {Object} options Accepts the following options:
    * - rect: mandatory object that should have the x, y, width, height properties
    * - fill: optional fill color for the rect
    */
   show: function(node, options) {
     if (!this._hasValidOptions(options) || !node || !node.ownerDocument) {
       this.hide();
-      return;
+      return false;
     }
 
     let contextNode = node.ownerDocument.documentElement;
 
     // Caculate the absolute rect based on the context node's adjusted quads.
     let quads = this.layoutHelpers.getAdjustedQuads(contextNode);
     if (!quads.length) {
       this.hide();
-      return;
+      return false;
     }
 
     let {bounds} = quads[0];
     let x = "left:" + (bounds.x + options.rect.x) + "px;";
     let y = "top:" + (bounds.y + options.rect.y) + "px;";
     let width = "width:" + options.rect.width + "px;";
     let height = "height:" + options.rect.height + "px;";
 
@@ -2020,16 +2037,18 @@ RectHighlighter.prototype = {
     if (options.fill) {
       style += "background:" + options.fill + ";";
     }
 
     // Set the coordinates of the highlighter and show it
     let rect = this.getElement("highlighted-rect");
     rect.setAttribute("style", style);
     rect.removeAttribute("hidden");
+
+    return true;
   },
 
   hide: function() {
     this.getElement("highlighted-rect").setAttribute("hidden", "true");
   }
 };
 register(RectHighlighter);
 exports.RectHighlighter = RectHighlighter;
@@ -2361,23 +2380,25 @@ GeometryEditorHighlighter.prototype = He
   },
 
   _show: function() {
     this.computedStyle = CssLogic.getComputedStyle(this.currentNode);
     let pos = this.computedStyle.position;
     // XXX: sticky positioning is ignored for now. To be implemented next.
     if (pos === "sticky") {
       this.hide();
-      return;
+      return false;
     }
 
     let hasUpdated = this._update();
     if (!hasUpdated) {
       this.hide();
+      return false;
     }
+    return true;
   },
 
   _update: function() {
     // At each update, the position or/and size may have changed, so get the
     // list of defined properties, and re-position the arrows and highlighters.
     this.definedProperties = this.getDefinedGeometryProperties();
 
     let isStatic = this.computedStyle.position === "static";
@@ -2827,16 +2848,17 @@ RulersHighlighter.prototype =  {
     this.markup.destroy();
 
     events.emit(this, "destroy");
   },
 
   show: function() {
     this.markup.removeAttributeForElement(this.ID_CLASS_PREFIX + "elements",
       "hidden");
+    return true;
   },
 
   hide: function() {
     this.markup.setAttributeForElement(this.ID_CLASS_PREFIX + "elements",
       "hidden", "true");
   }
 };
 
@@ -2869,16 +2891,17 @@ SimpleOutlineHighlighter.prototype = {
    */
   show: function(node) {
     if (!this.currentNode || node !== this.currentNode) {
       this.hide();
       this.currentNode = node;
       installHelperSheet(getWindow(node), SIMPLE_OUTLINE_SHEET);
       DOMUtils.addPseudoClassLock(node, HIGHLIGHTED_PSEUDO_CLASS);
     }
+    return true;
   },
 
   /**
    * Hide the highlighter, the outline and the infobar.
    */
   hide: function() {
     if (this.currentNode) {
       DOMUtils.removePseudoClassLock(this.currentNode, HIGHLIGHTED_PSEUDO_CLASS);