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

style: Kill some style sharing code. #17713

Merged
merged 3 commits into from Jul 14, 2017
Merged

style: Kill some style sharing code. #17713

merged 3 commits into from Jul 14, 2017

Conversation

@emilio
Copy link
Member

emilio commented Jul 13, 2017 •
edited by larsbergstrom

It's trivial to do so after #17688.

This change is Reviewable
@highfive
Copy link

highfive commented Jul 13, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/data.rs, components/style/sharing/mod.rs, components/style/traversal.rs
  • @canaltinova: components/style/data.rs, components/style/sharing/mod.rs, components/style/traversal.rs
@highfive
Copy link

highfive commented Jul 13, 2017

warning Warning warning

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

emilio commented Jul 13, 2017

r? @heycam or @SimonSapin
@highfive highfive assigned heycam and unassigned jdm Jul 13, 2017
emilio added 2 commits Jul 13, 2017
@emilio
style: Kill some style sharing code.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
36310e3 
MozReview-Commit-ID: 42MyHx3QHuu
@emilio
style: Don't use the current element info for cached validation data.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
Loading status checks…
62310d9 
We can take it straight from the validation target.
@emilio emilio force-pushed the emilio:less-code-is-lovely branch from bd84ea0 to 62310d9 Jul 13, 2017
@emilio
Copy link
Member Author

emilio commented Jul 13, 2017

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19d54f008cbc0a91bad523ffb837020b4d060830
@emilio
Copy link
Member Author

emilio commented Jul 13, 2017

The failures in the above try run are fixed by #17710
@heycam
heycam reviewed Jul 14, 2017
View changes
components/style/traversal.rs Outdated

// This is only relevant for animations as of right now.
important_rules_changed = true;

This comment has been minimized.

Sign in to view
@heycam

heycam Jul 14, 2017

Member

Hmm, was this a bug that we weren't setting this if we got the styles from the cache?

This comment has been minimized.

Sign in to view
@emilio

emilio Jul 14, 2017

Author Member

Well, we never cache styles with animations, so it was probably OK (except for web animations maybe?)
components/style/traversal.rs Outdated
@@ -773,48 +773,52 @@ where

// Now that our bloom filter is set up, try the style sharing
// cache. If we get a match we can skip the rest of the work.

This comment has been minimized.

Sign in to view
@heycam

heycam Jul 14, 2017

Member

Nit: Maybe move this comment down to just before we look up the cache?

This comment has been minimized.

Sign in to view
@emilio

emilio Jul 14, 2017

Author Member

Sounds good, will do
components/style/sharing/mod.rs
@@ -584,14 +525,7 @@ impl<E: TElement> StyleSharingCandidateCache<E> {

match sharing_result {
Ok(shared_styles) => {
// Yay, cache hit. Share the style.
let child_cascade_requirement =
target.accumulate_damage_when_sharing(shared_context,

This comment has been minimized.

Sign in to view
@heycam

heycam Jul 14, 2017

Member

Can you explain for me when we no longer need to do this? (I assume it's already done elsewhere.)

This comment has been minimized.

Sign in to view
@emilio

emilio Jul 14, 2017

Author Member

Right, with this patch se arrive to finish_restyle instead of early-returning, which does all this :)
@heycam
Copy link
Member

heycam commented Jul 14, 2017

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2017

📌 Commit 62310d9 has been approved by heycam
@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2017

⌛ Testing commit 62310d9 with merge f98f2b4...
bors-servo added a commit that referenced this pull request Jul 14, 2017
@bors-servo
Auto merge of #17713 - emilio:less-code-is-lovely, r=heycam
Loading status checks…
f98f2b4 
style: Kill some style sharing code.

It's trivial to do so after #17688.

<!-- 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/17713)
<!-- Reviewable:end -->
@emilio
style: Move comment to a closer location.
Unverified
This user has not uploaded their public key yet.
GPG key ID: 056B727BB9C1027C Learn about signing commits
Loading status checks…
bd10081
@emilio
Copy link
Member Author

emilio commented Jul 14, 2017

@bors-servo r=heycam p=1

Moved the comment
@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2017

📌 Commit bd10081 has been approved by heycam
@bors-servo
Copy link
Contributor

bors-servo commented Jul 14, 2017

⌛ Testing commit bd10081 with merge 1c85c55...
bors-servo added a commit that referenced this pull request Jul 14, 2017
@bors-servo
Auto merge of #17713 - emilio:less-code-is-lovely, r=heycam
Loading status checks…
1c85c55 
style: Kill some style sharing code.

It's trivial to do so after #17688.

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

bors-servo commented Jul 14, 2017

☀️ 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: heycam
Pushing 1c85c55 to master...
@bors-servo bors-servo merged commit bd10081 into servo:master Jul 14, 2017
3 checks passed
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
Reviewers

@heycam heycam

Assignees

@heycam heycam

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@emilio @highfive @heycam @bors-servo @jdm
You can’t perform that action at this time.