Closed Bug 1403078 Opened 7 years ago Closed 7 years ago

Lazily tweak the traversal root to account for sibling invalidations.

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(1 file)

This is a nice thing we can do, and this allows fixing bug 1403028 properly along the way.
Comment on attachment 8912128 [details] [diff] [review]
0001-Bug-1403078-Lazily-tweak-the-traversal-root-to-accou.patch

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

::: layout/style/ServoStyleSet.cpp
@@ +972,5 @@
> +    MOZ_ASSERT(MayTraverseFrom(root));
> +
> +    Element* parent = root->GetFlattenedTreeParentElementForStyle();
> +    MOZ_ASSERT_IF(parent,
> +                  !parent->HasAnyOfFlags(Element::kAllServoDescendantBits));

If you want to follow dbaron's preference this wouldn't be MOZ_ASSERT_IF. ;-)  (And maybe should have a message, which MOZ_ASSERT_IF doesn't allow.)

::: servo/components/style/traversal.rs
@@ +171,2 @@
>  
> +            if !traversal_flags.for_animation_only() {

We weren't checking this before.  Did that have any bad effect, or we just wastefully looking for later siblings when we knew there couldn't be any there?

@@ +171,3 @@
>  
> +            if !traversal_flags.for_animation_only() {
> +                // Invalidate our style, and the one of our siblings and

(s/the one/that/ to be super correct.)

::: servo/ports/geckolib/glue.rs
@@ +313,5 @@
>             element.borrow_data().unwrap());
>  
> +    let element_was_restyled =
> +        element.borrow_data().unwrap().contains_restyle_data();
> +    element_was_restyled

Since we're changing what the return value means, can you update the comment at the top of the function?  And we can also then remove needs_post_traversal since there aren't any other callers.
Attachment #8912128 - Flags: review?(cam) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3867791bc1d670a448070eb79e816a0bcd6ae12

(In reply to Cameron McCormack (:heycam) from comment #2)
> Comment on attachment 8912128 [details] [diff] [review]
> 0001-Bug-1403078-Lazily-tweak-the-traversal-root-to-accou.patch
> 
> Review of attachment 8912128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/ServoStyleSet.cpp
> @@ +972,5 @@
> > +    MOZ_ASSERT(MayTraverseFrom(root));
> > +
> > +    Element* parent = root->GetFlattenedTreeParentElementForStyle();
> > +    MOZ_ASSERT_IF(parent,
> > +                  !parent->HasAnyOfFlags(Element::kAllServoDescendantBits));
> 
> If you want to follow dbaron's preference this wouldn't be MOZ_ASSERT_IF.
> ;-)  (And maybe should have a message, which MOZ_ASSERT_IF doesn't allow.)

Fair enough. I find MOZ_ASSERT_IF nice for null-checks though, since it's obvious which is the IF, and which the ASSERT (otherwise it's a sure crash).

> ::: servo/components/style/traversal.rs
> @@ +171,2 @@
> >  
> > +            if !traversal_flags.for_animation_only() {
> 
> We weren't checking this before.  Did that have any bad effect, or we just
> wastefully looking for later siblings when we knew there couldn't be any
> there?

Not really, worst case is that we wasted a bit of work. But it's worth being consistent, and it is relevant now, otherwise we may not actually restyle the siblings.

> @@ +171,3 @@
> >  
> > +            if !traversal_flags.for_animation_only() {
> > +                // Invalidate our style, and the one of our siblings and
> 
> (s/the one/that/ to be super correct.)
> 
> ::: servo/ports/geckolib/glue.rs
> @@ +313,5 @@
> >             element.borrow_data().unwrap());
> >  
> > +    let element_was_restyled =
> > +        element.borrow_data().unwrap().contains_restyle_data();
> > +    element_was_restyled
> 
> Since we're changing what the return value means, can you update the comment
> at the top of the function?  And we can also then remove
> needs_post_traversal since there aren't any other callers.

Will do. Also, needs_post_traversal was already gone in the original patch.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d75d1ebf0e99
Lazily tweak the traversal root to account for sibling invalidations. r=heycam
https://hg.mozilla.org/mozilla-central/rev/d75d1ebf0e99
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1405544
You need to log in before you can comment on or make changes to this bug.