Closed Bug 1187777 Opened 4 years ago Closed 4 years ago

[rule view] Lazily create computed styles list

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: simon.lindholm10, Assigned: simon.lindholm10)

Details

Attachments

(1 file, 2 obsolete files)

Testing on about:home, this saves ~5ms out of 50 for me when selecting a new element.
Comment on attachment 8639131 [details] [diff] [review]
0001-Bug-1187777-Lazily-create-computed-styles-list.-r.patch

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

Redirecting review to :gl - this looks generally sane, but since it's touching filter stuff I'd like him to take a look at it

::: browser/devtools/styleinspector/rule-view.js
@@ +2816,5 @@
>    this.prop = aProperty;
>    this.prop.editor = this;
>    this.browserWindow = this.doc.defaultView.top;
>    this.removeOnRevert = this.prop.value === "";
> +  this._populatedComputed = true;

Shouldn't this be false by default?  Nothing's been populated yet on intialization
Attachment #8639131 - Flags: review?(gabriel.luong)
Comment on attachment 8639131 [details] [diff] [review]
0001-Bug-1187777-Lazily-create-computed-styles-list.-r.patch

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

Approach overall looks very good. Thank you for the patch Simon!

Looking at the available unit tests, I don't believe we have any test coverage on whether or not the expander button is displayed for a property that has a computed list. I would like to see test cases to ensure we get the expected behavior and prevent regressions.

Taking a quick look at all the tests. I don't see any unit tests to test the correctness of the items in the computed list, so, that is a bit troubling.

::: browser/devtools/styleinspector/rule-view.js
@@ +3152,3 @@
>     */
>    _updateComputed: function() {
> +    this.computed.textContent = "";

I am not sure if this is the preferred way of clearing the element over using removeChild.

@bgrins: can you comment on this approach?
Attachment #8639131 - Flags: review?(gabriel.luong) → feedback+
(In reply to Gabriel Luong [:gl] from comment #3)
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +3152,3 @@
> >     */
> >    _updateComputed: function() {
> > +    this.computed.textContent = "";
> 
> I am not sure if this is the preferred way of clearing the element over
> using removeChild.
> 
> @bgrins: can you comment on this approach?

We've used different approaches for this across the code base, but I'm happy to remove the older verbose way.  I would switch it to `this.computed.innerHTML = "";` though since I think that's used a bit more thoughout the devtools code base.
Comment on attachment 8639131 [details] [diff] [review]
0001-Bug-1187777-Lazily-create-computed-styles-list.-r.patch

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

Clearing review - can you please add a test as suggested in Comment 3?  Probably a new test in browser/devtools/styleinspector/test that tests if the expanded is visible / computed is populated, maybe called browser_ruleview_expanded_01.js.  Using the _01.js with the anticipation that we will add another one either now or later for general correctness as well
Attachment #8639131 - Flags: review?(bgrinstead)
Will do as soon as I get time. Could take up to a week.

> Shouldn't this be false by default?  Nothing's been populated yet on intialization

Well, the empty state has been populated. This makes it valid to call _populateComputed() even before _updateComputed has been called for the first time. (It seems likely that this never happens, though.)

I rather prefer `textContent = ""` over `innerHTML = ""` personally, because innerHTML is otherwise evil and it's nice to be able to grep for it and see only valid matches. But I can change for consistency.
Something like this, I hope.
Attachment #8639131 - Attachment is obsolete: true
Attachment #8643969 - Flags: review?(gabriel.luong)
Comment on attachment 8643969 [details] [diff] [review]
0001-Bug-1187777-Lazily-create-computed-styles-list.-r-gl.patch

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

Everything looks good to me! I believe we should address the initial value of this._populatedComputed.

Please flag bgrins for the final review. Unfortunately, I am not a review peer.

::: browser/devtools/styleinspector/rule-view.js
@@ +2843,5 @@
>    this.prop = aProperty;
>    this.prop.editor = this;
>    this.browserWindow = this.doc.defaultView.top;
>    this.removeOnRevert = this.prop.value === "";
> +  this._populatedComputed = true;

I think this should be set to false. In _updateComputed, this value will be set to false, and leaving it true leads to the confusion that is we believe it is populated but it really is just an empty container.
Attachment #8643969 - Flags: review?(gabriel.luong) → feedback+
With "this._populatedComputed = false" and corrected reviewer.

(I do still think "true" would more correct, with _populateComputed initially being a no-op rather than throwing, but since it doesn't matter whatsoever because _populatedComputed is set to a proper value later in the constructor, I'll go with whatever is less confusing.)
Attachment #8643969 - Attachment is obsolete: true
Attachment #8644251 - Flags: review?(bgrinstead)
Comment on attachment 8644251 [details] [diff] [review]
0001-Bug-1187777-Lazily-create-computed-styles-list.-r-bg.patch

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

Needs a simple rebase on rule-view.js.  Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9d82f54debe

::: browser/devtools/styleinspector/test/browser_ruleview_computed_02.js
@@ +6,5 @@
> +
> +// Tests that the rule view computed lists can be expanded/collapsed,
> +// and contain the right subproperties.
> +
> +let TEST_URI = `

Nice use of multiline strings for the test URI
Attachment #8644251 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/6e532fe19bb7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.