1187777 - [rule view] Lazily create computed styles list

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Simon Lindholm]

Testing on about:home, this saves ~5ms out of 50 for me when selecting a new element.

Comment 1

[Simon Lindholm]

Attachment: 0001-Bug-1187777-Lazily-create-computed-styles-list.-r.patch

(green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=464fbcf2b23c )

Comment 2

[Brian Grinstead [:bgrins]]

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

Comment 3

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

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?

Comment 4

[Brian Grinstead [:bgrins]]

(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 5

[Brian Grinstead [:bgrins]]

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

Comment 6

[Simon Lindholm]

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.

Comment 7

[Simon Lindholm]

Attachment: 0001-Bug-1187777-Lazily-create-computed-styles-list.-r-gl.patch

Something like this, I hope.

Comment 8

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

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.

Comment 9

[Simon Lindholm]

Attachment: 0001-Bug-1187777-Lazily-create-computed-styles-list.-r-bg.patch

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.)

Comment 10

[Brian Grinstead [:bgrins]]

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6e532fe19bb7

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6e532fe19bb7