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)

enhancement

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.
Flags: needinfo?(emilio)
Priority: -- → P2
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
Flags: needinfo?(emilio)
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 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 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 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 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+
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?
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/adba302ab807
Stop the cascade when only reset structs change. r=heycam
https://hg.mozilla.org/mozilla-central/rev/adba302ab807
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1417781
You need to log in before you can comment on or make changes to this bug.