Skip to content
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: More traversal cleanup. #19164

Merged
merged 4 commits into from Nov 15, 2017
Merged

Conversation

emilio
Copy link
Member

@emilio emilio commented Nov 9, 2017

This is cleanup that allows me to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1415013 in a more straight-forward way.


This change is Reviewable

@highfive
Copy link

highfive commented Nov 9, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/data.rs, components/style/traversal.rs, components/style/context.rs, components/style/matching.rs
  • @canaltinova: components/style/data.rs, components/style/traversal.rs, components/style/context.rs, components/style/matching.rs

@highfive
Copy link

highfive commented Nov 9, 2017

warning Warning warning

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

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 9, 2017
@emilio
Copy link
Member Author

emilio commented Nov 9, 2017

r? @bzbarsky

@emilio emilio force-pushed the more-traversal-cleanup branch 2 times, most recently from 79afd0a to 6732759 Compare November 9, 2017 15:22
@emilio
Copy link
Member Author

emilio commented Nov 9, 2017

@emilio
Copy link
Member Author

emilio commented Nov 9, 2017

@bors-servo try

(Also, @nox or @SimonSapin may want to stamp it if they feel confident about it)

@bors-servo
Copy link
Contributor

⌛ Trying commit cd51bcb with merge 55686a4...

bors-servo pushed a commit that referenced this pull request Nov 9, 2017
style: More traversal cleanup.

This is cleanup that allows me to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1415013 in a more straight-forward way.

<!-- 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/19164)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - linux-rel-wpt

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-tests-failed The changes caused existing tests to fail. labels Nov 9, 2017
@emilio
Copy link
Member Author

emilio commented Nov 10, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 7714390 with merge fa72cc4...

bors-servo pushed a commit that referenced this pull request Nov 10, 2017
style: More traversal cleanup.

This is cleanup that allows me to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1415013 in a more straight-forward way.

<!-- 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/19164)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-css2

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Nov 10, 2017
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Nov 10, 2017
@emilio
Copy link
Member Author

emilio commented Nov 10, 2017

@bors-servo try

This should be ready for review, I had to fix a Servo bug (39f6bb7).

@bors-servo
Copy link
Contributor

⌛ Trying commit 17e0ec4 with merge d0d6f54...

bors-servo pushed a commit that referenced this pull request Nov 10, 2017
style: More traversal cleanup.

This is cleanup that allows me to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1415013 in a more straight-forward way.

<!-- 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/19164)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

💔 Test failed - mac-rel-wpt4

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Nov 10, 2017
emilio added a commit to emilio/servo that referenced this pull request Nov 10, 2017
…pposed from add_child / remove_child.

add_child / remove_child aren't called when a node character data changed.

The only test that tests this is now wallpapered, but won't be after servo#19164.

Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>
emilio added a commit to emilio/servo that referenced this pull request Nov 10, 2017
…ad of add_child / remove_child.

add_child / remove_child aren't called when a node character data changed. This
is important for finer grained invalidation.

The only test that tests this is now wallpapered, but won't be after servo#19164.
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Nov 10, 2017
@emilio
Copy link
Member Author

emilio commented Nov 10, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 9d8e92a with merge 5f4c94f...

bors-servo pushed a commit that referenced this pull request Nov 10, 2017
style: More traversal cleanup.

This is cleanup that allows me to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1415013 in a more straight-forward way.

<!-- 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/19164)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

jdm
jdm previously requested changes Nov 10, 2017
// rooting? I'm pretty sure this cannot GC!
let mut current = Some(DomRoot::from_ref(self));

while let Some(node) = current.take() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no good answer for the rooting question, unfortunately. As for this code, let's use:

for node in self.ancestors() {
    if node.get_flag(...) {
        return;
    }
    node.set_flag(...);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to inclusive_ancestors, which is what it is intended to do.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Nov 10, 2017
…ramed.

We only need to do this when display changes from none to non-none, so handle it
explicitly when computing the cascade requirement.

This patch also removes a few conditions that are redundant because they're
handled also by the cascade requirement check, like the initial styling.
@emilio
Copy link
Member Author

emilio commented Nov 14, 2017

Heycam's back!

r? @heycam (if @bzbarsky is busy)

@nox
Copy link
Contributor

nox commented Nov 15, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 3a5d70f has been approved by nox

@highfive highfive assigned nox and unassigned heycam Nov 15, 2017
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Nov 15, 2017
@emilio
Copy link
Member Author

emilio commented Nov 15, 2017

@bors-servo p=1

@bors-servo
Copy link
Contributor

⌛ Testing commit 3a5d70f with merge d117694...

bors-servo pushed a commit that referenced this pull request Nov 15, 2017
style: More traversal cleanup.

This is cleanup that allows me to fix https://bugzilla.mozilla.org/show_bug.cgi?id=1415013 in a more straight-forward way.

<!-- 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/19164)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: nox
Pushing d117694 to master...

@bors-servo bors-servo merged commit 3a5d70f into servo:master Nov 15, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants