Closed Bug 1384769 Opened 2 years ago Closed 2 years ago

stylo: Clean up traversal modes

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(6 files, 3 obsolete files)

This is the first set of patches in a grand refactoring that I got sucked into while working on bug 1383332.

There's more to come, but I wanted to get some of the pieces landed to prevent my commit queue from growing too large. There's a lot more cleanup coming, so please keep that in mind during review.

These are green on try except for the last two patches, which I'm running through now.
MozReview-Commit-ID: EVUzgnL5coN
Attachment #8890608 - Flags: review?(emilio+bugs)
MozReview-Commit-ID: I6xeHv65nH2
Attachment #8890610 - Flags: review?(emilio+bugs)
MozReview-Commit-ID: 1IRn2iaPnVc
Attachment #8890611 - Flags: review?(emilio+bugs)
They're redundant and boilerplate. We leave for_animation_only because it's very
commonly-used.

MozReview-Commit-ID: 8S4ohjhokQ9
Attachment #8890612 - Flags: review?(emilio+bugs)
We already have a more-specific check further down in the file, which was added in the same revision.
I think this one was erroneous.

MozReview-Commit-ID: CnP0zCpBtnp
Attachment #8890613 - Flags: review?(emilio+bugs)
These will be useful in followup work.

MozReview-Commit-ID: Dyp9R0PG36v
Attachment #8890615 - Flags: review?(emilio+bugs)
Assignee: nobody → bobbyholley
MozReview-Commit-ID: 1IRn2iaPnVc
Attachment #8890688 - Flags: review?(emilio+bugs)
Attachment #8890611 - Attachment is obsolete: true
Attachment #8890611 - Flags: review?(emilio+bugs)
Comment on attachment 8890608 [details] [diff] [review]
Part 1 - Pass TraversalFlags from C++ into Rust. v1

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

r- because of the transmute-y stuff, which I think it's just not right.

::: servo/components/style/traversal.rs
@@ +37,5 @@
> +#[repr(u32)]
> +#[derive(Copy, Clone, Debug, PartialEq)]
> +pub enum TraversalFlags {
> +    /// No flags.
> +    Empty = 0x00,

nit: While you're here, I'd change these to 0, 1 << 0, 1 << 1, and so on.

@@ +97,5 @@
> +
> +impl BitOr for TraversalFlags {
> +    type Output = Self;
> +    fn bitor(self, other: Self) -> Self {
> +        unsafe { mem::transmute(self as u32 | other as u32) }

This is ub in rust, afaict. Let's keep using bitflags, and use from_bits_truncate in the other places.
Attachment #8890608 - Flags: review?(emilio+bugs) → review-
Comment on attachment 8890610 [details] [diff] [review]
Part 2 - Clean up ForThrottledAnimationFlush stuff. v1

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

301 Hiro on this one. I think the ForThrottledAnimationFlush name is useful in the post traversal, but...
Attachment #8890610 - Flags: review?(emilio+bugs) → review?(hikezoe)
Comment on attachment 8890612 [details] [diff] [review]
Part 4 - Remove most of the TraversalFlag accessors. v1

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

This probably changes substantially given part 1.
Attachment #8890612 - Flags: review?(emilio+bugs)
Attachment #8890613 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8890615 [details] [diff] [review]
Part 6 - Break TraversalFlags::ForReconstruct down into several independent pieces. v1

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

So, I'm not in love with this patch because your patches make TraversalFlags a completely public API, and this makes that much more easy to misuse... But if it helps in followup work, I guess it's fine...

::: servo/components/style/traversal.rs
@@ +42,2 @@
>      /// Traverse only unstyled children.
> +    UnstyledChildrenOnly = 1 << 0,

Oh, you did that here :)
Attachment #8890615 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8890688 [details] [diff] [review]
Part 3 - Get rid of the ForNewlyBoundElement mode. v2

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

r=me % comments

::: layout/style/ServoStyleSet.cpp
@@ +1186,5 @@
> +ServoStyleSet::MayTraverseFrom(Element* aElement)
> +{
> +  Element* parent = aElement->GetFlattenedTreeParentElementForStyle();
> +  if (!parent) {
> +    return true;

This will return true for stuff outside of the document. There's no caller that does this, I think, but either assert it, or check it?

@@ +1193,5 @@
> +  if (!parent->HasServoData()) {
> +    return false;
> +  }
> +
> +  RefPtr<ServoStyleContext> sc = Servo_ResolveStyleAllowStale(parent).Consume();

I wonder if instead of this we should just expose a Servo_IsDisplayNone of some sort? That'd be slightly easier to use I think, and also avoid the refcount traffic, though that's arguably not a huge deal.
Attachment #8890688 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8890610 [details] [diff] [review]
Part 2 - Clean up ForThrottledAnimationFlush stuff. v1

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

Yeah, Emilio is right. The name is pretty important to avoid confusion in post traversal. So please leave the variable names in post traversal as it is. r=me with that.

::: layout/base/ServoRestyleManager.cpp
@@ +660,5 @@
>    const bool traverseElementChildren =
>      NeedsToTraverseElementChildren(*aElement, aFlags);
>    const bool descendantsNeedFrames =
>      aElement->HasFlag(NODE_DESCENDANTS_NEED_FRAMES);
> +  const bool animationOnly =

Here, please leave |forThrottledAnimationFlush| name.

@@ +831,5 @@
>    AnimationsWithDestroyedFrame animationsWithDestroyedFrame(this);
>  
>    ServoStyleSet* styleSet = StyleSet();
>    nsIDocument* doc = PresContext()->Document();
> +  bool animationOnly = !!(aFlags & ServoTraversalFlags::AnimationOnly);

Also here.

::: layout/style/ServoStyleSet.cpp
@@ +271,5 @@
>  ServoStyleSet::PrepareAndTraverseSubtree(
>    RawGeckoElementBorrowed aRoot,
>    ServoTraversalFlags aFlags)
>  {
> +  bool animationOnly = !!(aFlags & ServoTraversalFlags::AnimationOnly);

Also here.

@@ +778,5 @@
>  bool
>  ServoStyleSet::StyleDocument(ServoTraversalFlags aFlags)
>  {
> +  if (!!(aFlags & ServoTraversalFlags::AnimationOnly)) {
> +    PreTraverse(nullptr, EffectCompositor::AnimationRestyleType::Full);

Nice! I've never thought we can use EffectCompositor::AnimationRestyleType here.
Attachment #8890610 - Flags: review?(hikezoe) → review+
Here is a patch that we (I and Bobby and Brian) discussed on IRC. I tried three approaches to make display property animation in SMIL work somehow.

In case of display property animation;
1) Resolving style for SMIL element subtree right before we set the animation dirty bit to the SMIL element in nsSMILAnimationController::PreTraverseInSubtree()
2) Set normal dirty bit in a SequentialTask
3) Set normal dirty bit with eRestyle_Subtree and nsChangeHint_ReconstructFrame innsSMILAnimationController::PreTraverseInSubtree()

Initially I thought 1) will work fine, but it actually didn't since we end up calling the PreTraverseInSubtree() recursively....

2) should work but for now we need more work there since we can't call Servo_NoteExplicitHints() in SequentialTask. Unlike CSS kind animations we don't have the second traversal for SMIL so we need to modify ServoStyleSet to call nsSMILAnimationController::PreTraverse() twice or more.

3) is ugly but it worked. In this patch I just check the animating property is display or not, but I guess we can also check it's about to change from none to other (Brian knows SMIL stuff better than I do).

Anyway, I could not finish this up today, so I am just posting this work-in-progress patch that includes half-baked 2) and 3) for a reference.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> Comment on attachment 8890688 [details] [diff] [review]
> Part 3 - Get rid of the ForNewlyBoundElement mode. v2
> 
> Review of attachment 8890688 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me % comments
> 
> ::: layout/style/ServoStyleSet.cpp
> @@ +1186,5 @@
> > +ServoStyleSet::MayTraverseFrom(Element* aElement)
> > +{
> > +  Element* parent = aElement->GetFlattenedTreeParentElementForStyle();
> > +  if (!parent) {
> > +    return true;
> 
> This will return true for stuff outside of the document. There's no caller
> that does this, I think, but either assert it, or check it?

Yeah, I meant to assert it. Thanks for the reminder.

> 
> @@ +1193,5 @@
> > +  if (!parent->HasServoData()) {
> > +    return false;
> > +  }
> > +
> > +  RefPtr<ServoStyleContext> sc = Servo_ResolveStyleAllowStale(parent).Consume();
> 
> I wonder if instead of this we should just expose a Servo_IsDisplayNone of
> some sort? That'd be slightly easier to use I think, and also avoid the
> refcount traffic, though that's arguably not a huge deal.

Yeah, I went back and forth on this. I eventually decided to do this approach because it was more likely to be useful for other random things, and because the refcount traffic is probably ok since this is only called from XBL-y callsites on opt builds. Also the semantics of whether Servo_IsDisplayNone should assert against stale styles or not seemed kinda muddy.
I rolled part 4 into this.

MozReview-Commit-ID: EVUzgnL5coN
Attachment #8891029 - Flags: review?(emilio+bugs)
Attachment #8890608 - Attachment is obsolete: true
Attachment #8890612 - Attachment is obsolete: true
Comment on attachment 8891029 [details] [diff] [review]
Part 1 - Pass TraversalFlags from C++ into Rust. v2

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

::: layout/style/ServoStyleSet.cpp
@@ +274,3 @@
>  {
>    bool forThrottledAnimationFlush =
> +    !!(aFlags & ServoTraversalFlags::ForThrottledAnimationFlush);

nit: I think it's clear enough without the !!, but your choice.

::: layout/style/ServoStyleSet.h
@@ +263,5 @@
>     *
>     * This will traverse all of the document's style roots (that is, its document
>     * element, and the roots of the document-level native anonymous content).
>     *
> +   * The only allowed flag (for now ) is `ForCSSRuleChanges`.

nit: s/(for now )/(for now)/

::: servo/components/style/traversal_flags.rs
@@ +5,5 @@
> +
> +//! Flags that control the traversal process.
> +//!
> +//! We CamelCase rather than UPPER_CASING so that we can grep for the same
> +//! strings across gecko and servo.

If someone gets mad about this, I think the other sane thing to do would be to just UPPER_CASE the C++ ones, which is not _that_ uncommon.
Attachment #8891029 - Flags: review?(emilio+bugs) → review+
Blocks: 1385089
I moved the display:none animation stuff to bug 1385089, where we can continue it as a followup.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #17)
> Comment on attachment 8891029 [details] [diff] [review]
> Part 1 - Pass TraversalFlags from C++ into Rust. v2
> 
> Review of attachment 8891029 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/ServoStyleSet.cpp
> @@ +274,3 @@
> >  {
> >    bool forThrottledAnimationFlush =
> > +    !!(aFlags & ServoTraversalFlags::ForThrottledAnimationFlush);
> 
> nit: I think it's clear enough without the !!, but your choice.

Per IRC discussion, this doesn't work currently because the enum macro stuff doesn't support operator bool for some reason.
> 
> ::: layout/style/ServoStyleSet.h
> @@ +263,5 @@
> >     *
> >     * This will traverse all of the document's style roots (that is, its document
> >     * element, and the roots of the document-level native anonymous content).
> >     *
> > +   * The only allowed flag (for now ) is `ForCSSRuleChanges`.
> 
> nit: s/(for now )/(for now)/

It goes away in a later patch so I just left it.

> 
> ::: servo/components/style/traversal_flags.rs
> @@ +5,5 @@
> > +
> > +//! Flags that control the traversal process.
> > +//!
> > +//! We CamelCase rather than UPPER_CASING so that we can grep for the same
> > +//! strings across gecko and servo.
> 
> If someone gets mad about this, I think the other sane thing to do would be
> to just UPPER_CASE the C++ ones, which is not _that_ uncommon.

Yeah fair.
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81049c04c4ad
Pass TraversalFlags from C++ into Rust. r=emilio
https://hg.mozilla.org/integration/autoland/rev/b393ef5e7bec
Clean up ForThrottledAnimationFlush stuff. r=hiro
https://hg.mozilla.org/integration/autoland/rev/6948acff0d83
Get rid of the ForNewlyBoundElement mode. r=emilio
https://hg.mozilla.org/integration/autoland/rev/6eedbd4bd808
Break TraversalFlags::ForReconstruct down into several independent pieces. r=emilio
https://hg.mozilla.org/integration/autoland/rev/402f1e901cd7
Skip two smil tests due to crashes until bug 1385089 is sorted out. r=me
You need to log in before you can comment on or make changes to this bug.