Closed Bug 1347270 Opened 7 years ago Closed 7 years ago

stylo: Need to expose restyled element count to nsIDOMWindowUtils

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(3 files)

Bug 1347270: Rewrite test_media_queries_dynamic.html using restyleGeneration.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1347270: remove the nsIDOMWindowUtils::ElementsRestyled API.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1347270: Also rewrite browser_toolbariconcolor_restyles.js using restyleGeneration.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
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
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.
(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 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-
(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 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 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+
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
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+
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: