stylo: Implement ServoRestyleManager::RebuildAllStyleData properly

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: bz, Assigned: emilio)

Tracking

(Blocks: 1 bug)

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

See bug 1298588 comment 30 and the comments in the patch linked therein.
Summary: Implement ServoRestyleManager::RebuildAllStyleData properly → stylo: Implement ServoRestyleManager::RebuildAllStyleData properly
Priority: -- → P2
Priority: P2 → P1
Blocks: 1331301
(Assignee)

Comment 1

5 months ago
This is blocking to much stuff. I think I have something that works for us.
Assignee: nobody → emilio+bugs
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

5 months ago
ni? bz for review, as requested.

I tried to write decent commit messages ;)
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 5

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

So much unexpected pass orange :)
Nice! Thanks for picking this up. :-)
(Reporter)

Comment 7

5 months ago
mozreview-review
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.
Attachment #8846186 - Flags: review+
(Reporter)

Comment 8

5 months ago
mozreview-review
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.  :(
Attachment #8846187 - Flags: review+
(Assignee)

Updated

5 months ago
See Also: → bug 1347270
Blocks: 1346925
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

5 months ago
mozreview-review-reply
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.
(Assignee)

Comment 13

5 months ago
mozreview-review
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.
Attachment #8847247 - Flags: review?(emilio+bugs) → review+

Comment 14

5 months ago
Pushed by ecoal95@gmail.com:
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
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?
Flags: needinfo?(emilio+bugs)

Comment 16

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bbbf2a60b284
https://hg.mozilla.org/mozilla-central/rev/eeb27a3284db
https://hg.mozilla.org/mozilla-central/rev/fb3732eaa103
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Updated

5 months ago
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

5 months ago
Flags: needinfo?(emilio+bugs)
status-firefox53: affected → ---
You need to log in before you can comment on or make changes to this bug.