Closed Bug 1318835 Opened 3 years ago Closed 3 years ago

Inspector should own the one instance of the HighlightersOverlay

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: gl, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch 1318835.patch [1.0] (obsolete) — Splinter Review
Attachment #8813555 - Flags: review?(pbrosset)
Attached patch 1318835.patch [2.0] (obsolete) — Splinter Review
Attachment #8813555 - Attachment is obsolete: true
Attachment #8813555 - Flags: review?(pbrosset)
Attachment #8814105 - Flags: review?(pbrosset)
Blocks: 1308260
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.
Attachment #8814105 - Flags: review?(pbrosset)
Inspector bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
Attached patch 1318835.patch [3.0] (obsolete) — Splinter Review
Attachment #8814105 - Attachment is obsolete: true
Attachment #8814449 - Flags: review?(pbrosset)
Attachment #8814449 - Flags: review?(pbrosset)
Attached patch 1318835.patch [4.0] (obsolete) — Splinter Review
Attachment #8814449 - Attachment is obsolete: true
Attachment #8814450 - Flags: review?(pbrosset)
Attachment #8814450 - Attachment is obsolete: true
Attachment #8814450 - Flags: review?(pbrosset)
Attachment #8814641 - Flags: review?(pbrosset)
Attachment #8814641 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da7f7deca3216262b7140e578076ecb2cc65378
Bug 1318835 - Inspector should own the one instance of the HighlightersOverlay. r=pbro
https://hg.mozilla.org/mozilla-central/rev/1da7f7deca32
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.