servo / servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

style: Make restyle hints handle child combinators better than restyling the entire subtree #16906

Closed

heycam wants to merge 6 commits into servo:master from heycam:child-combinator

Member

heycam commented May 17, 2017 •

edited

This PR reworks RestyleHint so that it can store the depth of the tree whose elements we need to re-run selector matching on, when selectors with > child combinators are involved. For example, with div.x > span > em, if we toggle the x attribute on a div, we'll re-run selector matching on grandchildren of the div, rather than (as we do currently) on every descendants of the div.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive assigned pcwalton

highfive commented May 17, 2017

Heads up! This PR modifies the following files:

@bholley: components/style/data.rs, components/style/restyle_hints.rs, components/style/traversal.rs, components/style/matching.rs
@emilio: components/style/data.rs, components/style/restyle_hints.rs, ports/geckolib/glue.rs, components/style/traversal.rs, components/style/matching.rs

highfive commented May 17, 2017

Warning

These commits modify style code, but no tests are modified. Please consider adding a test!

highfive added the S-awaiting-review label

Member Author

heycam commented May 17, 2017

highfive assigned emilio and unassigned pcwalton

emilio reviewed

Member

emilio left a comment

Looks great! mostly minor nits and naming shenanigans :)

components/style/restyle_hints.rs Outdated

  
            /// restyle work, and thus any `insert()` calls will have no effect.

            #[inline]

            pub fn is_maximum(&self) -> bool {

                self.match_self == 0xff && self.match_later_siblings && self.replacements.is_all()

emilio May 17, 2017

Member

what about using std::u8::MAX instead of 0xff?

heycam May 18, 2017

Author Member

I decided to leave this as the numeric literal, as I think it's slightly clearer given we're using it as a bitfield.

components/style/restyle_hints.rs Outdated

  
                        trace!(" > hint was already there");

                        return true;

                    }

                    // We can ignore the selector flags, since they would have already

                    // We can ignore the selector replacements, since they would have already

emilio May 17, 2017

Member

This comment is talking about the slow selector flags, so I don't think it should be changed.

heycam May 17, 2017

Author Member

Yeah, overzelaous find and replace.

components/style/restyle_hints.rs Outdated

  
            /// children, the third its grandchildren, and so on.  If the most significant

            /// bit it set, it indicates that that selector matching must be re-run for

            /// elements at that depth and all of their descendants.

            match_self: u8,

emilio May 17, 2017

Member

Could we abstract this and make it a newtype (MatchingDepth(u8)?) that enforces this? I really don't want to think a lot about what 0x01 means if I could do something like: MatchingDepth::just_self() or MatchingDepth::at_depth(x).

Also, I don't think the name of match_self is representative anymore :)

heycam May 17, 2017

Author Member

Great idea about the newtype!

Regarding the name, WDYT about match_under_self? (I want a name that I can adjust to have a similar field to represent depth to restyle under later siblings, e.g. match_later_siblings or match_under_later_siblings.)

emilio May 17, 2017

Member

match_under_self sounds good to me :)

components/style/restyle_hints.rs Outdated

  
      @@ -651,6 +819,8 @@ impl DependencySet {
    
                let mut combinator = None;

                let mut iter = selector.inner.complex.iter();

                let mut index = 0;

                let mut child_count = 0;

                let mut descendants = false;

emilio May 17, 2017

Member

I'd rename this to saw_descendant_combinator.

heycam May 18, 2017

Author Member

Just used short names to avoid wrapping lines that used them. Will rename.

components/style/restyle_hints.rs Outdated

  
                                if !descendants => RestyleHint::descendants_at_depth(child_count),

                            Some(Combinator::Child) => RestyleHint::descendants(),

                            Some(Combinator::Descendant) => RestyleHint::descendants(),

                            Some(Combinator::NextSibling) => RestyleHint::later_siblings(),

emilio May 17, 2017

Member

You can merge some branches here if you want.

components/style/restyle_hints.rs Outdated

  
      @@ -651,6 +819,8 @@ impl DependencySet {
    
                let mut combinator = None;

                let mut iter = selector.inner.complex.iter();

                let mut index = 0;

                let mut child_count = 0;

emilio May 17, 2017

Member

And perhaps these one to child_combinators_seen?

Contributor

bors-servo commented May 17, 2017

☔ The latest upstream changes (presumably #16900) made this pull request unmergeable. Please resolve the merge conflicts.

highfive added the S-needs-rebase label

heycam force-pushed the heycam:child-combinator branch 2 times, most recently from f37f1f4 to 99264ce

emilio approved these changes

Member

emilio left a comment

r=me, thanks!

Member

jdm commented May 18, 2017

   Compiling style_tests v0.0.1 (file:///home/travis/build/servo/servo/tests/unit/style)

error: unexpected close delimiter: `}`

  --> /home/travis/build/servo/servo/tests/unit/style/restyle_hints.rs:27:1

   |

27 | }

   | ^

error: Could not compile `style_tests`.

jdm added S-needs-code-changes and removed S-awaiting-review labels

heycam force-pushed the heycam:child-combinator branch from 99264ce to 2c7a117

highfive added S-awaiting-review and removed S-needs-code-changes labels

heycam force-pushed the heycam:child-combinator branch from 2c7a117 to ac4fb5d

Member Author

heycam commented May 20, 2017

I'm including the https://bugzilla.mozilla.org/show_bug.cgi?id=1289868 changes in this PR, which have also been reviewed by @emilio over there.

Member Author

heycam commented May 20, 2017

@bors-servo r=emilio

Contributor

bors-servo commented May 20, 2017

📌 Commit ad6a448 has been approved by emilio

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels

heycam force-pushed the heycam:child-combinator branch from ad6a448 to 4819dd9

highfive added S-awaiting-review and removed S-awaiting-merge labels

Member Author

heycam commented May 20, 2017

@bors-servo r=emilio

highfive added S-tests-failed and removed S-awaiting-merge labels

Member Author

heycam commented May 20, 2017

@bors-servo retry

Member Author

heycam commented May 20, 2017

Actually I think that's a real test failure. :(

Member Author

heycam commented May 20, 2017

I kind of suspect that Servo's damage calculation might be missing some properties, and that's making our decision to stop restyling invalid. For now I'm going to just always return StyleChange::Changed from compute_style_difference in Servo, and file a followup issue.

Member Author

heycam commented May 20, 2017

@bors-servo r=emilio

Contributor

bors-servo commented May 20, 2017

📌 Commit 3b769a0 has been approved by emilio

highfive added S-awaiting-merge and removed S-tests-failed labels

Member Author

heycam commented May 20, 2017

@bors-servo p=1

Contributor

bors-servo commented May 20, 2017

⌛ Testing commit 3b769a0 with merge 2a8c46c...

bors-servo added a commit that referenced this pull request


        Auto merge of #16906 - heycam:child-combinator, r=emilio

2a8c46c

style: Make restyle hints handle child combinators better than restyling the entire subtree

<!-- Please describe your changes on the following line: -->

This PR reworks `RestyleHint` so that it can store the depth of the tree whose elements we need to re-run selector matching on, when selectors with `>` child combinators are involved.  For example, with `div.x > span > em`, if we toggle the `x` attribute on a `div`, we'll re-run selector matching on grandchildren of the `div`, rather than (as we do currently) on every descendants of the `div`.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16906)
<!-- Reviewable:end -->

Contributor

bors-servo commented May 20, 2017

💔 Test failed - linux-rel-css

highfive added S-tests-failed and removed S-awaiting-merge labels

Member

emilio commented May 20, 2017

@bors-servo retry

Contributor

bors-servo commented May 20, 2017

⌛ Testing commit 3b769a0 with merge ff76c4f...

bors-servo added a commit that referenced this pull request


        Auto merge of #16906 - heycam:child-combinator, r=emilio

ff76c4f

style: Make restyle hints handle child combinators better than restyling the entire subtree

<!-- Please describe your changes on the following line: -->

This PR reworks `RestyleHint` so that it can store the depth of the tree whose elements we need to re-run selector matching on, when selectors with `>` child combinators are involved.  For example, with `div.x > span > em`, if we toggle the `x` attribute on a `div`, we'll re-run selector matching on grandchildren of the `div`, rather than (as we do currently) on every descendants of the `div`.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16906)
<!-- Reviewable:end -->

highfive added S-awaiting-merge and removed S-tests-failed labels

Contributor

bors-servo commented May 20, 2017

💔 Test failed - linux-rel-css

highfive added S-tests-failed and removed S-awaiting-merge labels

emilio mentioned this pull request

Bug 1366144: Correctly diff ::before and ::after pseudo-element styles if there's no generated content. r=heycam #16967

Merged

Member

emilio commented May 20, 2017

Rebased at #16967.

@heycam, you should be able to push to that branch, if I haven't managed to land it, please feel free to take over it.

Member

emilio commented May 20, 2017

I posted the analysis of the failing test there, too, btw

emilio mentioned this pull request

Incremental table layout border-width updates are broken. #16969

Open

Member

emilio commented May 20, 2017

Landed #16967 :)

emilio closed this

Member Author

heycam commented May 21, 2017

Thanks so much, @emilio!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

You can’t perform that action at this time.