Closed Bug 1351791 Opened 7 years ago Closed 7 years ago

stylo: Get rid of PostRestyleEventInternal in Servo's restyle manager.

Categories

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

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, 1 obsolete file)

Bug 1351791: Make NoteDirtyDescendantsForServo observe the refresh driver.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1351791: Move PostRestyleEventForLazyConstruction and PostRestyleEventInternal to GeckoRestyleManager.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1351791: Simplify PostRestyleEventForLazyConstruction.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
We can replace it from SetOwnerDocumentNeedsStyleFlush called from servo in the appropriate places.

This is followup work from bug 1351275.
Priority: -- → P1
Note: Your try push syntax isn't running stylo tests. You want:

-b do -p all -u all[linux64,linux64-stylo] -t none
Comment on attachment 8852858 [details]
Bug 1351791: Make NoteDirtyDescendantsForServo observe the refresh driver.

https://reviewboard.mozilla.org/r/125020/#review127722

::: dom/base/ElementInlines.h:69
(Diff revision 1)
>      shell->SetNeedStyleFlush();
> +    shell->ObserveStyleFlushes();

Nit - we call these together in various places, so would be nice to make it a helper.
Attachment #8852858 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> Note: Your try push syntax isn't running stylo tests. You want:
> 
> -b do -p all -u all[linux64,linux64-stylo] -t none

Yup, I realised of that a while ago, there's one with stylo tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0416a0a8650cd3f1465309a1d4dffeb5ff8eca7
Comment on attachment 8852859 [details]
Bug 1351791: Move PostRestyleEventForLazyConstruction and PostRestyleEventInternal to GeckoRestyleManager.

https://reviewboard.mozilla.org/r/125022/#review127724

We also need a patch to fix TElement::note_dirty_descendants, no?

r=me with an additional patch to do that, as well as the those two blocks removed below.

Also, don't forget to do a stylo try push. :-)

::: layout/base/nsCSSFrameConstructor.cpp:7489
(Diff revision 1)
> +      } else {
> +        // Lazy frame construction is done by the restyle flushes, so we need to
> +        // ensure a refresh happens.
> +        mPresShell->SetNeedStyleFlush();
> +        mPresShell->ObserveStyleFlushes();

Hm, I don't understand why we need these. If isNewlyAddedContentForServo is false, then either (a) we're Gecko-backed, or (b) we're in a style flush.

For (a), the MaybeConstructLazily call should already trigger a style flush. For (b), we're already flushing styles, and so don't need to set up this additional state.

So unless I'm missing something, this change should be removed.

::: layout/base/nsCSSFrameConstructor.cpp:7971
(Diff revision 1)
> +      } else {
> +        // Lazy frame construction is done by the restyle flushes, so we need to
> +        // ensure a refresh happens.
> +        mPresShell->SetNeedStyleFlush();
> +        mPresShell->ObserveStyleFlushes();

This one too.
Attachment #8852859 - Flags: review?(bobbyholley) → review+
Comment on attachment 8852860 [details]
Fix linking error.

https://reviewboard.mozilla.org/r/125024/#review127728

Oh, I see what you're doing here. This makes the previous patch make more sense.

It seems to me like it's a lot nicer to keep this Gecko-only complexity in the GeckoRestyleManager, since then it's easier to ignore when we're looking at the servo path. Moreover, the current behavior (which uses isNewlyAddedContentForServo rather than IsStyledByServo) will cause us to observe the refresh driver during servo style flushes, which isn't what we want.
Attachment #8852860 - Flags: review-
Comment on attachment 8852860 [details]
Fix linking error.

Please ignore this, it's from bug 1351700 and I had it locally in order to build.
Attachment #8852860 - Flags: review?(cam)
Comment on attachment 8853110 [details]
Bug 1351791: Simplify PostRestyleEventForLazyConstruction.

https://reviewboard.mozilla.org/r/125184/#review127756
Attachment #8853110 - Flags: review?(bobbyholley) → review+
Attachment #8852860 - Attachment is obsolete: true
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9)
> Comment on attachment 8852860 [details]
> Fix linking error.
> 
> https://reviewboard.mozilla.org/r/125024/#review127728
> 
> Oh, I see what you're doing here. This makes the previous patch make more
> sense.
> 
> It seems to me like it's a lot nicer to keep this Gecko-only complexity in
> the GeckoRestyleManager, since then it's easier to ignore when we're looking
> at the servo path. Moreover, the current behavior (which uses
> isNewlyAddedContentForServo rather than IsStyledByServo) will cause us to
> observe the refresh driver during servo style flushes, which isn't what we
> want.

For the record, we discussed this on IRC and this was wrong, we would observe the refresh driver even in a style refresh. But turns out we don't allow lazy construction during a style refresh, so it can be cleaned up instead.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6df7d7fb8b7e
Make NoteDirtyDescendantsForServo observe the refresh driver. r=bholley
https://hg.mozilla.org/integration/autoland/rev/1b66676f665f
Move PostRestyleEventForLazyConstruction and PostRestyleEventInternal to GeckoRestyleManager. r=bholley
https://hg.mozilla.org/integration/autoland/rev/494c291dbfcb
Simplify PostRestyleEventForLazyConstruction. r=bholley
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: