1318835 - Inspector should own the one instance of the HighlightersOverlay

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

We should only create one instance of the HighlightersOverlay on the Inspector so that it can manage all the states of the highlighters across the inspector and its sidebar views.

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1318835.patch [1.0]

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=011c9e912d5d

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1318835.patch [2.0]

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99c4a246e8e4

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8814105 [details] [diff] [review]
1318835.patch [2.0]

Review of attachment 8814105 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Gabriel. This looks good. I have only a few comments.

::: devtools/client/inspector/shared/highlighters-overlay.js
@@ +47,5 @@
>  }
>  
>  HighlightersOverlay.prototype = {
>    /**
> +   * Add the highlighters overlay to the view. This will start tracking mouse movements

s/movements/events since we now also use this for clicks.

@@ +50,5 @@
>    /**
> +   * Add the highlighters overlay to the view. This will start tracking mouse movements
> +   * and display highlighters when needed.
> +   *
> +   * @param  {CssRuleView|CssComputedView} view

Maybe also add LayoutView to the list of types? You intend to use this overlay from there too, right?

@@ +65,5 @@
>      el.addEventListener("mousemove", this._onMouseMove, false);
>      el.addEventListener("mouseout", this._onMouseOut, false);
>      el.ownerDocument.defaultView.addEventListener("mouseout", this._onMouseOut, false);
>  
> +    if (view.type === "CssRuleView") {

I don't like this new `type` property very much, it makes it harder to introduce new panels and reuse the highlighterOverlay. It's something we have to add to the rule-view and computed-view for an obscure reason, so we will forget about it.
I'm wondering if anything bad happens if we just remove the if? The content of the _onWillNavigate method doesn't seem to be doing anything too dangerous for other views. So maybe we just remove the if and get rid of the `type`.

@@ +199,5 @@
>                        nodeInfo.value.property === "transform";
>      let isEnabled = nodeInfo.value.enabled &&
>                      !nodeInfo.value.overridden &&
>                      !nodeInfo.value.pseudoElement;
> +    return this.inspector.sidebar.getCurrentTabID() == "ruleview" && isTransform &&

Instead of changing each and every `this.isRuleView` with `this.inspector.sidebar.getCurrentTabID() == "ruleview"` in this file (which is quite verbose), let's declare a getter:

get isRuleView() {
  return this.inspector.sidebar.getCurrentID() == "ruleview";
}

This way you don't need to change all of these this.isRuleView.

Comment 6

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1318835.patch [3.0]

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1318835.patch [4.0]

Comment 9

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eecc965845e

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1318835.patch [5.0]

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1da7f7deca3216262b7140e578076ecb2cc65378
Bug 1318835 - Inspector should own the one instance of the HighlightersOverlay. r=pbro

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1da7f7deca32