Bug 1167174 - Assert that custom highlighter do show the highlighter before proceeding. r=pbrosset

browser/devtools/styleinspector/style-inspector-overlays.js

@@ -21,6 +21,7 @@ const {
   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");
@@ -59,6 +60,8 @@ function HighlightersOverlay(view) {
   // 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;
@@ -124,9 +127,13 @@ HighlightersOverlay.prototype = {
     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");
+            }
+          });
     }
   },
 
@@ -176,6 +183,7 @@ HighlightersOverlay.prototype = {
           promise.then(null, Cu.reportError);
         }
         this.highlighterShown = null;
+        this.emit("highlighter-hidden");
       });
     }
   },

browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-02.js

@@ -36,7 +36,9 @@ add_task(function*() {
 
   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");
@@ -57,7 +59,9 @@ add_task(function*() {
 
   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");

browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-03.js

@@ -39,53 +39,64 @@ add_task(function*() {
       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");
 });

browser/devtools/styleinspector/test/browser_styleinspector_transform-highlighter-04.js

@@ -55,7 +55,9 @@ add_task(function*() {
 
   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");

toolkit/devtools/server/actors/highlighter.js

@@ -7,7 +7,7 @@
 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");
@@ -455,17 +455,21 @@ let CustomHighlighterActor = exports.CustomHighlighterActor = protocol.ActorClas
    *
    * @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")
     }
   }),
 
@@ -849,7 +853,7 @@ AutoRefreshHighlighter.prototype = {
     let isSameOptions = this._isSameOptions(options);
 
     if (!isNodeValid(node) || (isSameNode && isSameOptions)) {
-      return;
+      return false;
     }
 
     this.options = options;
@@ -858,9 +862,12 @@ AutoRefreshHighlighter.prototype = {
     this.currentNode = node;
     this._updateAdjustedQuads();
     this._startRefreshLoop();
-    this._show();
 
-    this.emit("shown");
+    let shown = this._show();
+    if (shown) {
+      this.emit("shown");
+    }
+    return shown;
   },
 
   /**
@@ -942,6 +949,7 @@ AutoRefreshHighlighter.prototype = {
     // 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() {
@@ -949,11 +957,13 @@ AutoRefreshHighlighter.prototype = {
     // 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() {
@@ -1229,9 +1239,10 @@ BoxModelHighlighter.prototype = Heritage.extend(AutoRefreshHighlighter.prototype
       this.options.region = "content";
     }
 
-    this._update();
+    let shown = this._update();
     this._trackMutations();
     this.emit("ready");
+    return shown;
   },
 
   /**
@@ -1259,6 +1270,7 @@ BoxModelHighlighter.prototype = Heritage.extend(AutoRefreshHighlighter.prototype
    * Should be called whenever node size or attributes change
    */
   _update: function() {
+    let shown = false;
     setIgnoreLayoutChanges(true);
 
     if (this._updateBoxModel()) {
@@ -1268,12 +1280,15 @@ BoxModelHighlighter.prototype = Heritage.extend(AutoRefreshHighlighter.prototype
         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;
   },
 
   /**
@@ -1775,15 +1790,14 @@ CssTransformHighlighter.prototype = Heritage.extend(AutoRefreshHighlighter.proto
 
   /**
    * 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();
   },
 
   /**
@@ -1830,7 +1844,7 @@ CssTransformHighlighter.prototype = Heritage.extend(AutoRefreshHighlighter.proto
     if (!quads.length ||
         quads[0].bounds.width <= 0 || quads[0].bounds.height <= 0) {
       this._hideShapes();
-      return null;
+      return false;
     }
 
     let [quad] = quads;
@@ -1850,6 +1864,7 @@ CssTransformHighlighter.prototype = Heritage.extend(AutoRefreshHighlighter.proto
     this._showShapes();
 
     setIgnoreLayoutChanges(false, this.currentNode.ownerDocument.documentElement);
+    return true;
   },
 
   /**
@@ -1898,7 +1913,7 @@ SelectorHighlighter.prototype = {
     this.hide();
 
     if (!isNodeValid(node) || !options.selector) {
-      return;
+      return false;
     }
 
     let nodes = [];
@@ -1922,6 +1937,8 @@ SelectorHighlighter.prototype = {
       this._highlighters.push(highlighter);
       i ++;
     }
+
+    return true;
   },
 
   hide: function() {
@@ -1998,7 +2015,7 @@ RectHighlighter.prototype = {
   show: function(node, options) {
     if (!this._hasValidOptions(options) || !node || !node.ownerDocument) {
       this.hide();
-      return;
+      return false;
     }
 
     let contextNode = node.ownerDocument.documentElement;
@@ -2007,7 +2024,7 @@ RectHighlighter.prototype = {
     let quads = this.layoutHelpers.getAdjustedQuads(contextNode);
     if (!quads.length) {
       this.hide();
-      return;
+      return false;
     }
 
     let {bounds} = quads[0];
@@ -2025,6 +2042,8 @@ RectHighlighter.prototype = {
     let rect = this.getElement("highlighted-rect");
     rect.setAttribute("style", style);
     rect.removeAttribute("hidden");
+
+    return true;
   },
 
   hide: function() {
@@ -2366,13 +2385,15 @@ GeometryEditorHighlighter.prototype = Heritage.extend(AutoRefreshHighlighter.pro
     // 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() {
@@ -2832,6 +2853,7 @@ RulersHighlighter.prototype =  {
   show: function() {
     this.markup.removeAttributeForElement(this.ID_CLASS_PREFIX + "elements",
       "hidden");
+    return true;
   },
 
   hide: function() {
@@ -2874,6 +2896,7 @@ SimpleOutlineHighlighter.prototype = {
       installHelperSheet(getWindow(node), SIMPLE_OUTLINE_SHEET);
       DOMUtils.addPseudoClassLock(node, HIGHLIGHTED_PSEUDO_CLASS);
     }
+    return true;
   },
 
   /**