Skip to content

/ 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

Implement state-based restyle hints #8274

Merged
merged 3 commits into from Nov 1, 2015
Merged

Implement state-based restyle hints #8274

merged 3 commits into from Nov 1, 2015
+439 −29

Conversation

@bholley
Copy link
Contributor

bholley commented Oct 30, 2015

Review on Reviewable
@highfive
Copy link

highfive commented Oct 30, 2015

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@bholley bholley force-pushed the bholley:state_restyle_hints branch from 64d29d9 to dd7a60e Oct 30, 2015
@pcwalton
Copy link
Contributor

pcwalton commented Oct 30, 2015

Looks good overall; just had a few minor nits.
@bors-servo
Copy link
Contributor

bors-servo commented Oct 30, 2015

☔ The latest upstream changes (presumably #8273) made this pull request unmergeable. Please resolve the merge conflicts.
@bholley bholley force-pushed the bholley:state_restyle_hints branch 2 times, most recently from 534c936 to f346632 Oct 31, 2015
@bholley bholley removed the S-needs-rebase label Oct 31, 2015
@bholley
Copy link
Contributor Author

bholley commented Oct 31, 2015

r? @pcwalton
@bholley bholley force-pushed the bholley:state_restyle_hints branch from f346632 to 1417f68 Oct 31, 2015
components/style/restyle_hints.rs
/// the selector matching logic. This only works for boolean states though, so we still need to
/// take the ElementWrapper approach for attribute-dependent style. So we do it the same both ways for
/// now to reduce complexity, but it's worth measuring the performance impact (if any) of the
/// mStateMask approach.

This comment has been minimized.

Sign in to view
@bholley

bholley Oct 31, 2015

Author Contributor

@SimonSapin Just FYI here - after discussing with @bzbarsky, we decided to try using the ElementWrapper approach for state restyle hints, which may remove the need to have ElementState in rust-selectors (see the comment above). I think it'd be good to leave it there for the time being though, so that we can try it both ways at some point and measure.

This comment has been minimized.

Sign in to view
@SimonSapin

SimonSapin Nov 3, 2015

Member

@bholley I don’t mind having ElementState in rust-selectors. (servo/rust-selectors#56 adressed the concern I had.) Feel free to move it back later if you want.
@bholley bholley force-pushed the bholley:state_restyle_hints branch from 1417f68 to 6022b1d Oct 31, 2015
@bholley
Copy link
Contributor Author

bholley commented Oct 31, 2015

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Oct 31, 2015

⌛ Trying commit 6022b1d with merge 5668fc5...
bors-servo added a commit that referenced this pull request Oct 31, 2015
@bors-servo
Auto merge of #8274 - bholley:state_restyle_hints, r=<try>
5668fc5 
Implement state-based restyle hints



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8274)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 31, 2015

💔 Test failed - mac-dev-ref-unit
@bholley
Copy link
Contributor Author

bholley commented Oct 31, 2015

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Oct 31, 2015

⌛ Trying commit 6022b1d with merge 56b3a98...
bors-servo added a commit that referenced this pull request Oct 31, 2015
@bors-servo
Auto merge of #8274 - bholley:state_restyle_hints, r=<try>
56b3a98 
Implement state-based restyle hints



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8274)
<!-- Reviewable:end -->
@pcwalton
Copy link
Contributor

pcwalton commented Oct 31, 2015

This looks good to me. r=me when try passes
@bors-servo
Copy link
Contributor

bors-servo commented Oct 31, 2015

☔ The latest upstream changes (presumably #8114) made this pull request unmergeable. Please resolve the merge conflicts.
@bors-servo
Copy link
Contributor

bors-servo commented Oct 31, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@jdm jdm removed the S-awaiting-review label Oct 31, 2015
@jdm jdm removed the S-tests-failed label Oct 31, 2015
bholley added 3 commits Oct 30, 2015
@bholley
Rev rust-selectors.
41dfd07
@bholley
Implement restyle hints for state changes.
45fc7f2
@bholley
State restyle hint tests.
Loading status checks…
564170f 
This handles the state part of #6942.
@bholley bholley force-pushed the bholley:state_restyle_hints branch from 6022b1d to 564170f Oct 31, 2015
@bholley bholley removed the S-needs-rebase label Oct 31, 2015
@Manishearth
Copy link
Member

Manishearth commented Oct 31, 2015

@bors-servo r=pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Oct 31, 2015

📌 Commit 564170f has been approved by pcwalton
@bors-servo
Copy link
Contributor

bors-servo commented Nov 1, 2015

⌛ Testing commit 564170f with merge fa2090a...
bors-servo added a commit that referenced this pull request Nov 1, 2015
@bors-servo
Auto merge of #8274 - bholley:state_restyle_hints, r=pcwalton
fa2090a 
Implement state-based restyle hints



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8274)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 1, 2015

💔 Test failed - linux-rel
@Manishearth
Copy link
Member

Manishearth commented Nov 1, 2015

@bors-servo retry #7791
@bors-servo
Copy link
Contributor

bors-servo commented Nov 1, 2015

⌛ Testing commit 564170f with merge 959ae86...
bors-servo added a commit that referenced this pull request Nov 1, 2015
@bors-servo
Auto merge of #8274 - bholley:state_restyle_hints, r=pcwalton
Loading status checks…
959ae86 
Implement state-based restyle hints



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8274)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 1, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 564170f into servo:master Nov 1, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@pcwalton
Copy link
Contributor

pcwalton commented Nov 1, 2015

\o/
components/style/restyle_hints.rs
}
}

impl<E> Element for ElementWrapper<E> where E: Element {

This comment has been minimized.

Sign in to view
@SimonSapin

SimonSapin Nov 3, 2015

Member

It’s slightly unfortunate to have a third implementation of this trait where I’d prefer just one (there’s a small risk their behavior will diverge) but oh well.
@bholley bholley deleted the bholley:state_restyle_hints branch Oct 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pcwalton pcwalton

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
@bholley @highfive @pcwalton @bors-servo @Manishearth @SimonSapin @jdm
You can’t perform that action at this time.