1328652 - stylo: Implement ServoRestyleManager::RebuildAllStyleData properly

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 1298588 comment 30 and the comments in the patch linked therein.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This is blocking to much stuff. I think I have something that works for us.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1328652: Assume medium feature changes in nsPresContext::MediaFeatureValuesChanged.

This allows us to test our media query stuff at least, and works around the fact
that we don't set mUsesViewportUnits.

This will gets us better test coverage, at the expense of more expensive
window resizes and similar.

I believe it's worth it.

Review commit: https://reviewboard.mozilla.org/r/119286/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119286/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1328652: Ensure we do actual restyling work after calling RebuildAllStyleData.

The Gecko restyle manager does this synchronously, along with a content flush.

In my testing there's no need to do so, and Boris couldn't think off-hand of
why, except the fact that we have this mRebuildAllStyleData thing that takes
care of rebuilding the rule tree, which is quite sensitive.

Also, Boris made a good point about non-inheriting anon boxes, that could
technically change style. I've left a note about it too.

Review commit: https://reviewboard.mozilla.org/r/119288/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119288/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

ni? bz for review, as requested.

I tried to write decent commit messages ;)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Try run with this and bug 1303229: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d2f6a8cafb83afc0f4e755353f6e36b10c2c4c9

So much unexpected pass orange :)

Comment 6

[Bobby Holley (:bholley)]

Nice! Thanks for picking this up. :-)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8846186 [details]
Bug 1328652: Assume medium feature changes in nsPresContext::MediaFeatureValuesChanged.

https://reviewboard.mozilla.org/r/119286/#review122114

I guess the point is this is temporary until we do bug 1290228?

r=me if that's the idea.

::: commit-message-8cf42:6
(Diff revision 1)
> +Bug 1328652: Assume medium feature changes in nsPresContext::MediaFeatureValuesChanged. r?bz
> +
> +This allows us to test our media query stuff at least, and works around the fact
> +that we don't set mUsesViewportUnits.
> +
> +This will gets us better test coverage, at the expense of more expensive

s/gets/get/

::: layout/base/nsPresContext.cpp:2029
(Diff revision 1)
>    }
>  
>    if (mUsesViewportUnits && mPendingViewportChange) {
>      // Rebuild all style data without rerunning selector matching.
> +    //
> +    // TODO(emilio): We don't seet mUsesViewportUnits yet. This is wallpapered

Is there a bug tracking that?  File as needed and reference here, please.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8846187 [details]
Bug 1328652: Ensure we do actual restyling work after calling RebuildAllStyleData.

https://reviewboard.mozilla.org/r/119288/#review122116

::: layout/base/ServoRestyleManager.cpp:100
(Diff revision 1)
> +  // NOTE(emilio): GeckoRestlyeManager does a sync style flush, which seems
> +  // not to be needed in my testing.
> +  //
> +  // If it is, we can just do a content flush and call ProcessPendingRestyles.
> +  if (Element* root = mPresContext->Document()->GetRootElement()) {
> +    PostRestyleEvent(root, aRestyleHint, aExtraHint);

This won't handle restyling the root frame and such.  In general, I guess we'll do that during our restyle traversal.  The question is whether we ever need to restyle them in cases when we have no root element....

I guess in the Gecko case we're trying to kill off the old ruletree, but here we don't care about that, right?

r=me, but I have at best medium confidence in this being entirely correct.  :(

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1328652: Reftest expectation adjustments.

Review commit: https://reviewboard.mozilla.org/r/120258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120258/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8846186 [details]
Bug 1328652: Assume medium feature changes in nsPresContext::MediaFeatureValuesChanged.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119286/diff/1-2/

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8846187 [details]
Bug 1328652: Ensure we do actual restyling work after calling RebuildAllStyleData.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119288/diff/1-2/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8846186 [details]
Bug 1328652: Assume medium feature changes in nsPresContext::MediaFeatureValuesChanged.

https://reviewboard.mozilla.org/r/119286/#review122114

> I guess the point is this is temporary until we do bug 1290228?

Yeah. We support media queries quite well (I'm filling bugs for the remaining stuff I'm seeing after a try run with these patches), we just need to keep the old matching result around in order to implement MediumFeaturesChanged properly.

> Is there a bug tracking that?  File as needed and reference here, please.

Filed bug 1328652.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8847247 [details]
Bug 1328652: Reftest expectation adjustments.

https://reviewboard.mozilla.org/r/120258/#review122158

Self-reviewing test expectations since there are only new passes, and they're consistent across three try runs.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bbbf2a60b284
Assume medium feature changes in nsPresContext::MediaFeatureValuesChanged. r=bz
https://hg.mozilla.org/integration/autoland/rev/eeb27a3284db
Ensure we do actual restyling work after calling RebuildAllStyleData. r=bz
https://hg.mozilla.org/integration/autoland/rev/fb3732eaa103
Reftest expectation adjustments. r=emilio

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Bug 1346829 has been very nearly permafailing on stylo since these patches landed. Can you take a look and see if https://hg.mozilla.org/integration/autoland/rev/fb3732eaa10364d4b6d074e2b475c1f16238cd5e#l7.12 needs changed in some way?

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bbbf2a60b284
https://hg.mozilla.org/mozilla-central/rev/eeb27a3284db
https://hg.mozilla.org/mozilla-central/rev/fb3732eaa103