stylo: More refactoring of the traversal in preparation for restyle roots

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(7 attachments)

Never ends. :-)
MozReview-Commit-ID: CPVnZjSwRpN
Attachment #8896077 - Flags: review?(emilio+bugs)
Doing anything else is non-sensical, since we're not guaranteed to reach all of
the bits from traversal Y when doing traversal X.

MozReview-Commit-ID: FQliRxBan70
Attachment #8896079 - Flags: review?(emilio+bugs)
MozReview-Commit-ID: 4BK0JfkgjdC
Attachment #8896080 - Flags: review?(emilio+bugs)
This makes things a bit easier to follow, and sets the stage for eliminating
PrepareAndTraverseSubtree and making StyleDocument restyle-root-aware.

MozReview-Commit-ID: 40ORrqAuXni
Attachment #8896081 - Flags: review?(emilio+bugs)
The buggy animation handling isn't a regression, since currently we pass
UnstyledChildrenOnly in those cases, which blocks the animation traversal
in Servo_TraverseSubtree.

In general I really wanted to handle these two paths together. But there's
enough broken with the NewChildren path that I wanted to scope the buginess
as tightly as possible. And I really need to separate the handling here from
StyleDocument() to make the restyle root stuff work.

MozReview-Commit-ID: 9F0mcQl7AAX
Attachment #8896082 - Flags: review?(emilio+bugs)
MozReview-Commit-ID: Kza0gGqvvmM
Attachment #8896083 - Flags: review?(emilio+bugs)
MozReview-Commit-ID: 83DgB1sZnCb
Attachment #8896085 - Flags: review?(emilio+bugs)
Blocks: 1389385
Comment on attachment 8896077 [details] [diff] [review]
Part 1 - Hoist various bits of PrepareAndTraverse functionality into an RAII class. v1

Review of attachment 8896077 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSet.h
@@ +482,5 @@
>      ServoStyleSet* mSet;
>    };
>  
> +  // Sets up for one or more calls to Servo_TraverseSubtree.
> +  class MOZ_STACK_CLASS AutoPrepareTraversal

Can we move this to the cpp file? That will make it easier to modify.

Also, MOZ_RAAI?
Attachment #8896077 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896079 [details] [diff] [review]
Part 2 - Only clear the descendants bit for which we're traversing. v1

Review of attachment 8896079 [details] [diff] [review]:
-----------------------------------------------------------------

Does this cause any failures or anything? It'd be nice getting a test where this behaves incorrectly.
Attachment #8896079 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896080 [details] [diff] [review]
Part 3 - Tidy up flags handling in recalc_style_at a bit. v1

Review of attachment 8896080 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/traversal.rs
@@ +483,5 @@
>      E: TElement,
>      D: DomTraversal<E>,
>      F: FnMut(E::ConcreteNode),
>  {
> +    use traversal_flags::*;

I think I prefer not having this, but... whatever.
Attachment #8896080 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896081 [details] [diff] [review]
Part 4 - Separate StyleSubtreeForReconstruct into its own path. v1

Review of attachment 8896081 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but you just bitrotted me quite hard :(
Attachment #8896081 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896082 [details] [diff] [review]
Part 5 - Hoist StyleNew{Children,Subtree} into their own paths. v1

Review of attachment 8896082 [details] [diff] [review]:
-----------------------------------------------------------------

So this looks fine... But I think we're adding a lot of special paths where we only had a single one before, which makes me very nervous... Also, the dirty descendants dance in the unstyled patch feels a lot like a hack :(

I don't understand the propagated_hint stuff, why is it needed at all? I really want that answered before granting r+.

::: layout/style/ServoStyleSet.cpp
@@ +948,5 @@
> +                        ServoTraversalFlags::UnstyledOnly);
> +
> +  // Restore the old state of the dirty descendants bit.
> +  if (!hadDirtyDescendants) {
> +    aParent->UnsetHasDirtyDescendantsForServo();

Can we assert that we have no dirty bits around here?

::: servo/components/style/traversal.rs
@@ +205,5 @@
>          debug!("element_needs_traversal({:?}, {:?}, {:?}, {:?})",
>                 el, traversal_flags, data, parent_data);
> +
> +        //
> +        // Handle our special traversal types.

This comment doesn't make much sense I think.

@@ +228,5 @@
>              return true;
>          }
>  
> +        //
> +        // Handle regular traversals.

ditto. I think this is mostly noise.

@@ +525,5 @@
>  
>      // Now that matching and cascading is done, clear the bits corresponding to
>      // those operations and compute the propagated restyle hint.
> +    let mut propagated_hint = if flags.contains(UnstyledOnly) {
> +        RestyleHint::empty()

Why?
Attachment #8896082 - Flags: review?(emilio+bugs)
Comment on attachment 8896083 [details] [diff] [review]
Part 6 - Inline PrepareAndTraverseSubtree into StyleDocument. v1

Review of attachment 8896083 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSet.cpp
@@ +827,5 @@
> +    bool isInitial = !root->HasServoData();
> +    auto flags = aBaseFlags;
> +
> +    // Do the first traversal.
> +    bool pt = Servo_TraverseSubtree(root, mRawSet.get(), &snapshots, flags);

call this something meaningful. postTraversalRequiredForThisTraversal?
Attachment #8896083 - Flags: review?(emilio+bugs) → review+
Attachment #8896085 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8896082 [details] [diff] [review]
Part 5 - Hoist StyleNew{Children,Subtree} into their own paths. v1

Review of attachment 8896082 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/traversal.rs
@@ +525,5 @@
>  
>      // Now that matching and cascading is done, clear the bits corresponding to
>      // those operations and compute the propagated restyle hint.
> +    let mut propagated_hint = if flags.contains(UnstyledOnly) {
> +        RestyleHint::empty()

r=me, with a comment about why we can't touch the restyle data (because we traverse the root, and because we follow dirty bits anyway here and we don't want to drop hints on the floor... Still, this seems super-fragile.
Attachment #8896082 - Flags: review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)
> Comment on attachment 8896077 [details] [diff] [review]
> Part 1 - Hoist various bits of PrepareAndTraverse functionality into an RAII
> class. v1
> 
> Review of attachment 8896077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/ServoStyleSet.h
> @@ +482,5 @@
> >      ServoStyleSet* mSet;
> >    };
> >  
> > +  // Sets up for one or more calls to Servo_TraverseSubtree.
> > +  class MOZ_STACK_CLASS AutoPrepareTraversal
> 
> Can we move this to the cpp file? That will make it easier to modify.

Good point.

> 
> Also, MOZ_RAAI?

Oh nice, I forgot we had that.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Does this cause any failures or anything? It'd be nice getting a test where
> this behaves incorrectly.

Unfortunately it doesn't - it was just a drive-by fix when I realized that I'd done it incorrectly in the other bug. A testcase for this would probably be tricky to construct.


(In reply to Emilio Cobos Álvarez [:emilio] from comment #13)
> > +                        ServoTraversalFlags::UnstyledOnly);
> > +
> > +  // Restore the old state of the dirty descendants bit.
> > +  if (!hadDirtyDescendants) {
> > +    aParent->UnsetHasDirtyDescendantsForServo();
> 
> Can we assert that we have no dirty bits around here?

Not sure what you mean. If there are pending invalidations in the already-styled children of |aParent|, the function will enter/exit with the dirty bit set. That's why we need to save/restore the old value.

> 
> ::: servo/components/style/traversal.rs
> @@ +205,5 @@
> >          debug!("element_needs_traversal({:?}, {:?}, {:?}, {:?})",
> >                 el, traversal_flags, data, parent_data);
> > +
> > +        //
> > +        // Handle our special traversal types.
> 
> This comment doesn't make much sense I think.
> 
> @@ +228,5 @@
> >              return true;
> >          }
> >  
> > +        //
> > +        // Handle regular traversals.
> 
> ditto. I think this is mostly noise.

Shrug - ok I'll remove them.

> 
> @@ +525,5 @@
> >  
> >      // Now that matching and cascading is done, clear the bits corresponding to
> >      // those operations and compute the propagated restyle hint.
> > +    let mut propagated_hint = if flags.contains(UnstyledOnly) {
> > +        RestyleHint::empty()
> 
> Why?

I'll add a comment.(In reply to Emilio Cobos Álvarez [:emilio] from comment #14)
> call this something meaningful. postTraversalRequiredForThisTraversal?

I wanted to make it short to prevent spilling lines. I'll call it |required|, since the rest is clear enough from context.

Thanks for the reviews!
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a299e11ece73
Hoist various bits of PrepareAndTraverse functionality into an RAII class. r=emilio
https://hg.mozilla.org/integration/autoland/rev/8eaaf3be604e
Separate StyleSubtreeForReconstruct into its own path. r=emilio
https://hg.mozilla.org/integration/autoland/rev/d01d9b48a068
Hoist StyleNew{Children,Subtree} into their own paths. r=emilio
https://hg.mozilla.org/integration/autoland/rev/53644622a1e9
Inline PrepareAndTraverseSubtree into StyleDocument. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1a8d86baaabc
Make the parallel traversal an explicit flag instead of guessing from Servo. r=emilio
Depends on: 1402472
You need to log in before you can comment on or make changes to this bug.