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: Handle correctly document state invalidation inside negation. #19805

Merged
merged 3 commits into from Jan 20, 2018

Conversation

@emilio
Copy link
Member

commented Jan 18, 2018

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jan 18, 2018

Heads up! This PR modifies the following files:

  • @bholley: components/style/gecko/generated/bindings.rs, components/style/gecko/wrapper.rs, components/selectors/matching.rs, components/selectors/context.rs, ports/geckolib/tests/build.rs
  • @canaltinova: components/style/gecko/generated/bindings.rs, components/style/gecko/wrapper.rs
@highfive

This comment has been minimized.

Copy link

commented Jan 18, 2018

warning Warning warning

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

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2018

r? @upsuper or @nox

@highfive highfive assigned upsuper and unassigned Manishearth Jan 18, 2018

@upsuper
Copy link
Member

left a comment

I don't particularly like this approach... but I cannot think out a better way either, so fine...

r=me with the comment addressed.

@@ -620,6 +620,15 @@ where
E: Element,
F: FnMut(&E, ElementSelectorFlags),
{
if cfg!(debug_assertions) {
if context.shared.nesting_level == 0 {
debug_assert!(!context.shared.in_negation, "How?");

This comment has been minimized.

Copy link
@upsuper

upsuper Jan 19, 2018

Member

It seems to me that the whole if block is equivalent to

debug_assert!(context.shared.nesting_level > 0 || !context.shared.in_negation);

I cannot think out a case which this single line wouldn't assert but the code here would, and vice verse

This comment has been minimized.

Copy link
@emilio

emilio Jan 19, 2018

Author Member

Oh, nice... Indeed :)

@emilio emilio force-pushed the emilio:doc-state-fix branch from c936cf1 to 2f96de4 Jan 19, 2018

@emilio

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2018

@bors-servo r=upsuper

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

📌 Commit 2f96de4 has been approved by upsuper

@emilio

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2018

@bors-servo r=upsuper

  • trying to unstuck homu
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

💡 This pull request was already approved, no need to approve it again.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

📌 Commit 2f96de4 has been approved by upsuper

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

⌛️ Testing commit 2f96de4 with merge 478b587...

bors-servo added a commit that referenced this pull request Jan 19, 2018
Auto merge of #19805 - emilio:doc-state-fix, r=upsuper
style: Handle correctly document state invalidation inside negation.

<!-- 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/19805)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

💔 Test failed - linux-dev

@emilio emilio force-pushed the emilio:doc-state-fix branch from 2f96de4 to d8fb084 Jan 19, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2018

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

emilio added 3 commits Jan 18, 2018

@emilio emilio force-pushed the emilio:doc-state-fix branch from d8fb084 to d14c979 Jan 20, 2018

@emilio

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2018

@bors-servo r=xidorn

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2018

📌 Commit d14c979 has been approved by xidorn

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2018

⌛️ Testing commit d14c979 with merge 3b07be5...

bors-servo added a commit that referenced this pull request Jan 20, 2018
Auto merge of #19805 - emilio:doc-state-fix, r=xidorn
style: Handle correctly document state invalidation inside negation.

<!-- 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/19805)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2018

@bors-servo bors-servo merged commit d14c979 into servo:master Jan 20, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.