Closed
Bug 1318835
Opened 8 years ago
Closed 8 years ago
Inspector should own the one instance of the HighlightersOverlay
Categories
(DevTools :: Inspector, defect, P3)
DevTools
Inspector
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)
25.36 KB,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8813555 -
Flags: review?(pbrosset)
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=011c9e912d5d
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8813555 -
Attachment is obsolete: true
Attachment #8813555 -
Flags: review?(pbrosset)
Attachment #8814105 -
Flags: review?(pbrosset)
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=99c4a246e8e4
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8814105 -
Attachment is obsolete: true
Attachment #8814449 -
Flags: review?(pbrosset)
Assignee | ||
Updated•8 years ago
|
Attachment #8814449 -
Flags: review?(pbrosset)
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8814449 -
Attachment is obsolete: true
Attachment #8814450 -
Flags: review?(pbrosset)
Assignee | ||
Comment 9•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eecc965845e
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8814450 -
Attachment is obsolete: true
Attachment #8814450 -
Flags: review?(pbrosset)
Attachment #8814641 -
Flags: review?(pbrosset)
Updated•8 years ago
|
Attachment #8814641 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da7f7deca3216262b7140e578076ecb2cc65378 Bug 1318835 - Inspector should own the one instance of the HighlightersOverlay. r=pbro
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1da7f7deca32
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•