Closed
Bug 1395227
Opened 7 years ago
Closed 7 years ago
stylo: stop the cascade if only reset properties change but no children explicitly inherited
Categories
(Core :: CSS Parsing and Computation, enhancement, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: bholley, Assigned: emilio)
References
Details
Attachments
(4 files)
See the discussion around bug 1374135 comment 6. Emilio says he has an idea of how to do this easily. If it's easy, we should get it landed. If it's hard, we should reconsider the priority.
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(emilio)
Reporter | ||
Updated•7 years ago
|
Priority: -- → P2
Updated•7 years ago
|
Summary: stylo: stop the cascade if only reset properties change bug no children explicitly inherited → stylo: stop the cascade if only reset properties change but no children explicitly inherited
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(emilio)
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c1768343d805934db619f5d6a7d28fe830b6fe2&group_state=expanded
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
Try run with the last two patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9205fc76d9d087426e5cd01f40ea067d68fb16b0 As expected this makes the test cases in bug 1374135 run instantly under stylo.
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8904296 [details] Bug 1395227: Stop the cascade when only reset structs change. https://reviewboard.mozilla.org/r/176072/#review181646 ::: servo/components/style/gecko/restyle_damage.rs:48 (Diff revision 1) > /// Note that we could in theory just get two `ComputedValues` here and diff > /// them, but Gecko has an interesting optimization when they mark accessed > /// structs, so they effectively only diff structs that have ever been > /// accessed from layout. Oh, this comment is a bit out of date, I notice... ::: servo/components/style/traversal.rs (Diff revision 1) > - // We must always cascade native anonymous subtrees, since they inherit > - // styles from their first non-NAC ancestor. > - if element.is_native_anonymous() { > - hint |= RECASCADE_SELF; > - } Hmm why do we no longer need this? ::: servo/components/style/traversal.rs:814 (Diff revision 1) > + ChildCascadeRequirement::CanSkipCascade => {} > + ChildCascadeRequirement::MustCascadeDescendants => { > + child_hint |= RECASCADE_SELF | RECASCADE_DESCENDANTS; > + } > + ChildCascadeRequirement::MustCascadeChildrenIfInheritResetStyle => { > + if child_data.styles.primary().flags.contains(::properties::computed_value_flags::INHERITS_RESET_STYLE) { Nit: this line should probably be shorter.
Attachment #8904296 -
Flags: review?(cam) → review+
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8904297 [details] style: Fix the legacy value of justify-items propagation. https://reviewboard.mozilla.org/r/176074/#review181652 ::: servo/components/style/matching.rs:404 (Diff revision 1) > + // > + // TODO(emilio): We could special-case this even more > + // creating a new `ChildCascadeRequirement` variant, but > + // it's unclear it matters. Yeah, it's pretty unlikely to matter.
Attachment #8904297 -
Flags: review?(cam) → review+
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8904342 [details] style: Fix recascading when child blockification depends on our display value. https://reviewboard.mozilla.org/r/176128/#review181656
Attachment #8904342 -
Flags: review?(cam) → review+
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8904343 [details] style: Reindent some code. https://reviewboard.mozilla.org/r/176130/#review181658 ::: servo/components/style/matching.rs:408 (Diff revision 1) > - let old_display = old_values.get_box().clone_display(); > + let old_display = old_values.get_box().clone_display(); > - let new_display = new_values.get_box().clone_display(); > + let new_display = new_values.get_box().clone_display(); > > + // Blockification of children may depend on our display value, > + // so we need to actually do the recascade. We could potentially > + // do better, but it doesn't seem worth. Nit: "worth it" ::: servo/components/style/style_adjuster.rs:377 (Diff revision 1) > - } > + } > > - match self.style.get_inheritedtext().clone_text_align() { > + match self.style.get_inheritedtext().clone_text_align() { > - text_align::_moz_left | > + text_align::_moz_left | > - text_align::_moz_center | > + text_align::_moz_center | > - text_align::_moz_right => {} > + text_align::_moz_right => {}, Nit: no "," after block?
Attachment #8904343 -
Flags: review?(cam) → review+
Comment 11•7 years ago
|
||
Could you also add a comment in style_adjuster.rs pointing out that new adjustments that inspect parent reset property values might need to have some handling added to matching.rs?
Comment 12•7 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/adba302ab807 Stop the cascade when only reset structs change. r=heycam
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/adba302ab807
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•