Closed
Bug 1384769
Opened 7 years ago
Closed 7 years ago
stylo: Clean up traversal modes
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(6 files, 3 obsolete files)
(deleted),
patch
|
hiro
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
MozReview-Commit-ID: EVUzgnL5coN
Attachment #8890608 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 2•7 years ago
|
||
MozReview-Commit-ID: I6xeHv65nH2
Attachment #8890610 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 3•7 years ago
|
||
MozReview-Commit-ID: 1IRn2iaPnVc
Attachment #8890611 -
Flags: review?(emilio+bugs)
Assignee | ||
Comment 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
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)
Assignee | ||
Comment 6•7 years ago
|
||
These will be useful in followup work. MozReview-Commit-ID: Dyp9R0PG36v
Attachment #8890615 -
Flags: review?(emilio+bugs)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bobbyholley
Assignee | ||
Comment 7•7 years ago
|
||
MozReview-Commit-ID: 1IRn2iaPnVc
Attachment #8890688 -
Flags: review?(emilio+bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8890611 -
Attachment is obsolete: true
Attachment #8890611 -
Flags: review?(emilio+bugs)
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
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 10•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8890613 -
Flags: review?(emilio+bugs) → review+
Comment 11•7 years ago
|
||
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 12•7 years ago
|
||
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 13•7 years ago
|
||
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+
Comment 14•7 years ago
|
||
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.
Assignee | ||
Comment 15•7 years ago
|
||
(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.
Assignee | ||
Comment 16•7 years ago
|
||
I rolled part 4 into this. MozReview-Commit-ID: EVUzgnL5coN
Attachment #8891029 -
Flags: review?(emilio+bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8890608 -
Attachment is obsolete: true
Attachment #8890612 -
Attachment is obsolete: true
Comment 17•7 years ago
|
||
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+
Assignee | ||
Comment 18•7 years ago
|
||
I moved the display:none animation stuff to bug 1385089, where we can continue it as a followup.
Assignee | ||
Comment 19•7 years ago
|
||
(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.
Comment 20•7 years ago
|
||
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
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/81049c04c4ad https://hg.mozilla.org/mozilla-central/rev/b393ef5e7bec https://hg.mozilla.org/mozilla-central/rev/6948acff0d83 https://hg.mozilla.org/mozilla-central/rev/6eedbd4bd808 https://hg.mozilla.org/mozilla-central/rev/402f1e901cd7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•