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

CSSOM: Make Stylesheet fields have their own synchronization #14232

Merged
merged 5 commits into from Nov 22, 2016
Merged

CSSOM: Make Stylesheet fields have their own synchronization #14232

merged 5 commits into from Nov 22, 2016

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Nov 15, 2016 •
edited by larsbergstrom

r? @upsuper

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix bug 1314208.
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable
@bors-servo
Copy link
Contributor

bors-servo commented Nov 16, 2016

☔ The latest upstream changes (presumably #14190) made this pull request unmergeable. Please resolve the merge conflicts.
@upsuper
Copy link
Member

upsuper commented Nov 16, 2016

I wonder whether we still want to wrap Stylesheet into Arc<RwLock<_>>, given that in #14190 rules is now wrapped in Arc<RwLock<_>>.

Wrapping Stylesheet.media is still needed, though.
@SimonSapin
Copy link
Member Author

SimonSapin commented Nov 18, 2016

You’re right. With other fields being already behind their own RwLock or immutable in CSSOM, this leaves dirty_on_viewport_size_change. I’ll make it an AtomicBool. We’ll need to add code to update it in various CSSOM mutating methods, though.
SimonSapin added 5 commits Nov 15, 2016
@SimonSapin
CSSOM: Add Arc<RwLock<_>> around Stylesheet.media
8eeaef5
@SimonSapin
Remove unused Stylesheet::from_bytes_iter
f4dcc38
@SimonSapin
Replace Stylesheet::set_media with a constructor argument
236c575
@SimonSapin
Make Stylesheet.dirty_on_viewport_size_change an AtomicBool
cf7adcd
@SimonSapin
Remove unused dependency
Loading status checks…
dca1772
@SimonSapin SimonSapin force-pushed the moar-locks branch from 6bfc1c3 to dca1772 Nov 18, 2016
@SimonSapin SimonSapin changed the title CSSOM: Add Arc<RwLock<_>> around Stylesheet and Stylesheet.media CSSOM: Make Stylesheet fields have their own synchronization Nov 18, 2016
@SimonSapin
Copy link
Member Author

SimonSapin commented Nov 18, 2016

Updated.
@upsuper
upsuper approved these changes Nov 20, 2016
View changes
Copy link
Member

upsuper left a comment

This looks good to me, but I cannot really approve this. Probably r? @Manishearth
@KiChjang
Copy link
Member

KiChjang commented Nov 20, 2016

@bors-servo delegate=upsuper

Now you can!
@bors-servo
Copy link
Contributor

bors-servo commented Nov 20, 2016

✌️ @upsuper can now approve this pull request
@upsuper
Copy link
Member

upsuper commented Nov 20, 2016

I'll try to do a more careful review then.
@upsuper
Copy link
Member

upsuper commented Nov 21, 2016

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

bors-servo commented Nov 21, 2016

📌 Commit dca1772 has been approved by upsuper
@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2016

⌛ Testing commit dca1772 with merge 8d69b90...
bors-servo added a commit that referenced this pull request Nov 21, 2016
@bors-servo
Auto merge of #14232 - servo:moar-locks, r=upsuper
Loading status checks…
8d69b90 
CSSOM: Make Stylesheet fields have their own synchronization

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

r? @upsuper

---
<!-- 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
- [x] These changes fix bug 1314208.

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

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

bors-servo commented Nov 21, 2016

💔 Test failed - linux-rel-wpt
@SimonSapin
Copy link
Member Author

SimonSapin commented Nov 21, 2016

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

bors-servo commented Nov 21, 2016

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-wpt...
@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2016

💔 Test failed - linux-rel-wpt
@KiChjang
Copy link
Member

KiChjang commented Nov 21, 2016

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

bors-servo commented Nov 21, 2016

⌛ Testing commit dca1772 with merge d795bcd...
bors-servo added a commit that referenced this pull request Nov 21, 2016
@bors-servo
Auto merge of #14232 - servo:moar-locks, r=upsuper
Loading status checks…
d795bcd 
CSSOM: Make Stylesheet fields have their own synchronization

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

r? @upsuper

---
<!-- 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
- [x] These changes fix bug 1314208.

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

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

bors-servo commented Nov 22, 2016

💔 Test failed - linux-rel-wpt
@KiChjang
Copy link
Member

KiChjang commented Nov 22, 2016

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

bors-servo commented Nov 22, 2016

⌛ Testing commit dca1772 with merge 3086700...
bors-servo added a commit that referenced this pull request Nov 22, 2016
@bors-servo
Auto merge of #14232 - servo:moar-locks, r=upsuper
Loading status checks…
3086700 
CSSOM: Make Stylesheet fields have their own synchronization

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

r? @upsuper

---
<!-- 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
- [x] These changes fix bug 1314208.

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

<!-- 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/14232)
<!-- Reviewable:end -->
@Manishearth
Copy link
Member

Manishearth commented Nov 22, 2016

@bors-servo p=20
@bors-servo
Copy link
Contributor

bors-servo commented Nov 22, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit dca1772 into master Nov 22, 2016
4 checks passed
4 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
dependency-ci Dependencies checked
Details
homu Test successful
Details
@Ms2ger Ms2ger deleted the moar-locks branch Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@upsuper upsuper

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@SimonSapin @bors-servo @upsuper @KiChjang @Manishearth @highfive
You can’t perform that action at this time.