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

Stylist accessors #16968

Merged
merged 3 commits into from May 23, 2017

Conversation

Projects
None yet
5 participants
@HeyZoos
Copy link
Contributor

commented May 20, 2017

Add accessor methods for the device and ruleset fields in the Stylist struct.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #16857 (github issue number if applicable).
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented May 20, 2017

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon.

@highfive

This comment has been minimized.

Copy link

commented May 20, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/gecko/data.rs, components/style/matching.rs, components/style/animation.rs, components/style/context.rs, components/style/stylist.rs
  • @emilio: components/style/gecko/data.rs, ports/geckolib/glue.rs, components/style/matching.rs, components/style/animation.rs, components/style/context.rs and 1 more
@highfive

This comment has been minimized.

Copy link

commented May 20, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@emilio

emilio approved these changes May 20, 2017

Copy link
Member

left a comment

Looks great! r=me with fn device changed to return &Device.

Let's not worry about the OwningRef for now. We can do it as a followup if needed.

Thanks a lot!

@@ -1065,6 +1065,21 @@ impl Stylist {
CascadeFlags::empty(),
self.quirks_mode))
}

/// Accessor for a shared reference to the device.
pub fn device(&self) -> &Arc<Device> {

This comment has been minimized.

Copy link
@emilio

emilio May 20, 2017

Member

Let's make this just return &Device. The Arc should ideally just be an implementation detail.


/// Accessor for a mutable reference to the device.
pub fn device_mut(&mut self) -> &mut Arc<Device> {
&mut self.device

This comment has been minimized.

Copy link
@emilio

emilio May 20, 2017

Member

I'm not sure it's possible to return &mut Device, right? I think we could make this work using OwningRef. This would return an OwningRefMut<Arc<Device>, Device>, I think.

I don't know if it's worth the churn though, so perhaps this is fine for now.

@@ -110,7 +110,7 @@ pub struct Stylist {
/// The rule tree, that stores the results of selector matching.
///
/// FIXME(emilio): Not `pub`!

This comment has been minimized.

Copy link
@emilio

emilio May 20, 2017

Member

This FIXME can go away now :)

@HeyZoos

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2017

@emilio Yo! Thanks for reviewing my PR. I returned &Arc<Device> originally because of a comment @canaltinova made in the issue thread. I wasn't sure what the changes would affect in the grander scheme of things so I chose to stick as closely as possible to what they described.

@emilio

This comment has been minimized.

Copy link
Member

commented May 21, 2017

@bors-servo r+

Looks great, thanks!

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 21, 2017

📌 Commit fb75583 has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented May 23, 2017

⌛️ Testing commit fb75583 with merge 1306b16...

bors-servo added a commit that referenced this pull request May 23, 2017

Auto merge of #16968 - HeyZoos:stylist-accessors, r=emilio
Stylist accessors

<!-- Please describe your changes on the following line: -->
Add accessor methods for the `device` and `ruleset` fields in the `Stylist` struct.

---
<!-- 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 #16857 (github issue number if applicable).
<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

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

This comment has been minimized.

Copy link
Contributor

commented May 23, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: emilio
Pushing 1306b16 to master...

@bors-servo bors-servo merged commit fb75583 into servo:master May 23, 2017

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
You can’t perform that action at this time.