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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
Details
Attachments
(1 file)
15.18 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
This is a nice thing we can do, and this allows fixing bug 1403028 properly along the way.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8912128 -
Flags: review?(cam)
Comment 2•7 years ago
|
||
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+
Assignee | ||
Comment 3•7 years ago
|
||
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
Comment 5•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d75d1ebf0e99
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•