stylo: Need to expose restyled element count to nsIDOMWindowUtils

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

4 months ago
I was checking out the remaining test failures in test_media_queries_dynamic.html after bug 1328652, and turns out we don't change nsPresContext::mElementsRestyled, so tests fail (even though the result is correct).

We should probably do this.
Priority: -- → P2
Blocks: 1321197
FWIW, we talked about this in taipei, and I think we concluded that the tests really just wanted to know whether any elements were restyled at all, so we might be able to simplify things a bit. Otherwise we'd need to turn traversal statistics on by default, which isn't so bad, except for aggregating the statistics for all the worker threads might be expensive.
(Assignee)

Comment 2

2 months ago
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> FWIW, we talked about this in taipei, and I think we concluded that the
> tests really just wanted to know whether any elements were restyled at all,
> so we might be able to simplify things a bit. Otherwise we'd need to turn
> traversal statistics on by default, which isn't so bad, except for
> aggregating the statistics for all the worker threads might be expensive.

We could also do it in the post-traversal I guess, and we wouldn't have to worry about worker threads and all that.
Comment hidden (mozreview-request)

Comment 4

2 months ago
mozreview-review
Comment on attachment 8870425 [details]
Bug 1347270: Rewrite test_media_queries_dynamic.html using restyleGeneration.

https://reviewboard.mozilla.org/r/141870/#review145546

As trivial as this is, I'm not really happy about keeping inaccurate statistics.

How about we just remove the field and have the getter return the restyle generation? That seems to be all the tests needs.

::: layout/base/ServoRestyleManager.cpp:325
(Diff revision 1)
>    // which consumes the new style and expects the old style to be on the frame.
>    //
>    // XXXbholley: We should teach the frame constructor how to clear the dirty
>    // descendants bit to avoid the traversal here.
>    if (changeHint & nsChangeHint_ReconstructFrame) {
> +    mPresContext->RestyledElement();

This isn't really accurate - we restyled all the elements in the subtree too.

::: layout/base/ServoRestyleManager.cpp:368
(Diff revision 1)
>    if (recreateContext) {
>      MOZ_ASSERT(styleFrame || displayContentsNode);
> +    mPresContext->RestyledElement();

This also isn't accurate, because we may still have restyled other things and ended up with the same style context.
Attachment #8870425 - Flags: review?(bobbyholley) → review-
(Assignee)

Comment 5

2 months ago
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #4)
> Comment on attachment 8870425 [details]
> Bug 1347270: Count restyled elements in ServoRestyleManager.
> 
> https://reviewboard.mozilla.org/r/141870/#review145546
> 
> As trivial as this is, I'm not really happy about keeping inaccurate
> statistics.
> 
> How about we just remove the field and have the getter return the restyle
> generation? That seems to be all the tests needs.
> 
> ::: layout/base/ServoRestyleManager.cpp:325
> (Diff revision 1)
> >    // which consumes the new style and expects the old style to be on the frame.
> >    //
> >    // XXXbholley: We should teach the frame constructor how to clear the dirty
> >    // descendants bit to avoid the traversal here.
> >    if (changeHint & nsChangeHint_ReconstructFrame) {
> > +    mPresContext->RestyledElement();
> 
> This isn't really accurate - we restyled all the elements in the subtree too.
> 
> ::: layout/base/ServoRestyleManager.cpp:368
> (Diff revision 1)
> >    if (recreateContext) {
> >      MOZ_ASSERT(styleFrame || displayContentsNode);
> > +    mPresContext->RestyledElement();
> 
> This also isn't accurate, because we may still have restyled other things
> and ended up with the same style context.

I added an API to access the restyle generation in bug 1357583, but looking again into this, this isn't really that much accurate in Gecko (Gecko also doesn't account for styles resolved from frame construction either, AFAICT, for example).

I think we it may be interesting to have the number of elements to restyle to test some of the restyle optimizations I'm planning to do in bug 1368240, but I guess I can also write them with the restyle generation... shrug
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

2 months ago
mozreview-review
Comment on attachment 8870425 [details]
Bug 1347270: Rewrite test_media_queries_dynamic.html using restyleGeneration.

https://reviewboard.mozilla.org/r/141870/#review148010

Thanks.
Attachment #8870425 - Flags: review?(bobbyholley) → review+

Comment 9

2 months ago
mozreview-review
Comment on attachment 8872565 [details]
Bug 1347270: remove the nsIDOMWindowUtils::ElementsRestyled API.

https://reviewboard.mozilla.org/r/144092/#review148012

r=me with that one fix.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
(Diff revision 1)
> -   * May throw NS_ERROR_NOT_AVAILABLE.
> -   */
> -  readonly attribute unsigned long long elementsRestyled;

Need to bump the IID of this interface.
Attachment #8872565 - Flags: review?(bobbyholley) → review+
Comment hidden (mozreview-request)

Comment 11

2 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f2b7f1d599e3
Rewrite test_media_queries_dynamic.html using restyleGeneration. r=bholley
https://hg.mozilla.org/integration/autoland/rev/445cb4d47d4f
remove the nsIDOMWindowUtils::ElementsRestyled API. r=bholley
Backed out for browser_toolbariconcolor_restyles.js permafail.
https://hg.mozilla.org/integration/autoland/rev/5b2c84104a7f4774fd120ab38d21b6483bcb63c7

https://treeherder.mozilla.org/logviewer.html#?job_id=103173111&repo=autoland
Assignee: nobody → emilio+bugs
Comment hidden (mozreview-request)

Comment 14

2 months ago
mozreview-review
Comment on attachment 8872798 [details]
Bug 1347270: Also rewrite browser_toolbariconcolor_restyles.js using restyleGeneration.

https://reviewboard.mozilla.org/r/144294/#review148106
Attachment #8872798 - Flags: review?(bobbyholley) → review+

Comment 15

2 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ac5d12953214
Rewrite test_media_queries_dynamic.html using restyleGeneration. r=bholley
https://hg.mozilla.org/integration/autoland/rev/bc65c7c5fd80
remove the nsIDOMWindowUtils::ElementsRestyled API. r=bholley
https://hg.mozilla.org/integration/autoland/rev/81ec0f138ba4
Also rewrite browser_toolbariconcolor_restyles.js using restyleGeneration. r=bholley
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9)
> Need to bump the IID of this interface.

Per this thread https://groups.google.com/forum/#!searchin/mozilla.dev.platform/interface$20uuid%7Csort:relevance/mozilla.dev.platform/n6qBpyxoI6I/4qxwFvBOAgAJ I don't think we need to bump XPCOM interface IIDs any more.

Comment 17

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ac5d12953214
https://hg.mozilla.org/mozilla-central/rev/bc65c7c5fd80
https://hg.mozilla.org/mozilla-central/rev/81ec0f138ba4
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.