Never ends. :-)

MozReview-Commit-ID: CPVnZjSwRpN

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

MozReview-Commit-ID: 4BK0JfkgjdC

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

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

MozReview-Commit-ID: Kza0gGqvvmM

MozReview-Commit-ID: 83DgB1sZnCb

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?

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.

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.

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 :(

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?

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?

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.

(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

https://hg.mozilla.org/mozilla-central/rev/a299e11ece73 https://hg.mozilla.org/mozilla-central/rev/8eaaf3be604e https://hg.mozilla.org/mozilla-central/rev/d01d9b48a068 https://hg.mozilla.org/mozilla-central/rev/53644622a1e9 https://hg.mozilla.org/mozilla-central/rev/1a8d86baaabc