style: Isolate the soon-to-be style-backend-specific code from the media_query module. #14833

Merged
merged 2 commits into from Jan 7, 2017

Projects

None yet

6 participants

@emilio
Member
emilio commented Jan 3, 2017 edited

The idea here is having (for convenience) different Expression and Device representations for Gecko.

Both will be implemented in rust, but will use nsMediaFeatures as a source of media features instead of the Servo bits.

Does it sound good?

r? @heycam or @Manishearth


This change is Reviewable

@heycam heycam was assigned by highfive Jan 3, 2017
@highfive
highfive commented Jan 3, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/servo/mod.rs, components/style/lib.rs, components/style/gecko/mod.rs, components/style/media_queries.rs, components/style/stylist.rs
@highfive
highfive commented Jan 3, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@jdm
Member
jdm commented Jan 3, 2017
error: missing documentation for an enum
   --> C:\projects\servo\components\style\media_queries.rs:178:1
    |
178 |   pub enum MediaType {
    |  _^ starting here...
179 | |     /// The "screen" media type.
180 | |     Screen,
181 | |     /// The "print" media type.
182 | |     Print,
183 | | }
    | |_^ ...ending here
    |
note: lint level defined here
   --> C:\projects\servo\components\style\lib.rs:29:9
    |
29  | #![deny(missing_docs)]
    |         ^^^^^^^^^^^^
error: missing documentation for a function
   --> C:\projects\servo\components\style\media_queries.rs:235:1
    |
235 | pub fn parse_media_query_list(input: &mut Parser) -> MediaList {
    | ^
error: aborting due to 2 previous errors
Build failed, waiting for other jobs to finish...
@bors-servo
Contributor

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

@heycam
Member
heycam commented Jan 6, 2017

This looks fine to me. In future, if you can separate out logically separate changes into separate patches (such as the moving and the Expression/ExpressionKind changes), it will help to make reviewing easier. Also, if moving and changing code at the same time, it's helpful to separate those two steps out so reviewers can see which parts have changed more easily.

@bors-servo r+

@bors-servo
Contributor

📌 Commit 3b7a13e has been approved by heycam

@emilio
Member
emilio commented Jan 6, 2017

The rebase wasn't as straight-forward due to @bzbarsky's default computed values patches.

Boris, mind signing-off the ViewportRule bits?

@bors-servo try

@bors-servo
Contributor

⌛️ Trying commit e31dd64 with merge 722b661...

@bors-servo bors-servo added a commit that referenced this pull request Jan 6, 2017
@bors-servo bors-servo Auto merge of #14833 - emilio:mq-refactor, r=<try>
style: Isolate the soon-to-be style-backend-specific code from the media_query module.

The idea here is having (for convenience) different `Expression` and `Device` representations for Gecko.

Both will be implemented in rust, but will use `nsMediaFeatures` as a source of media features instead of the Servo bits.

Does it sound good?

r? @heycam or @Manishearth

<!-- 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/14833)
<!-- Reviewable:end -->
722b661
@bors-servo
Contributor

💔 Test failed - linux-dev

@bzbarsky
Contributor
bzbarsky commented Jan 6, 2017

@emilio The ViewportConstraints changes look good.

@emilio
Member
emilio commented Jan 6, 2017

@bors-servo r=heycam,bz

@bors-servo
Contributor

📌 Commit b0cf8f3 has been approved by heycam,bz

@bors-servo
Contributor

⌛️ Testing commit b0cf8f3 with merge 5500af1...

@bors-servo bors-servo added a commit that referenced this pull request Jan 6, 2017
@bors-servo bors-servo Auto merge of #14833 - emilio:mq-refactor, r=heycam,bz
style: Isolate the soon-to-be style-backend-specific code from the media_query module.

The idea here is having (for convenience) different `Expression` and `Device` representations for Gecko.

Both will be implemented in rust, but will use `nsMediaFeatures` as a source of media features instead of the Servo bits.

Does it sound good?

r? @heycam or @Manishearth

<!-- 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/14833)
<!-- Reviewable:end -->
5500af1
@bors-servo
Contributor

💔 Test failed - linux-dev

@emilio
Member
emilio commented Jan 6, 2017

@bors-servo r=heycam,bz

  • Fixed tidy
@bors-servo
Contributor

📌 Commit 7b20f45 has been approved by heycam,bz

@bors-servo
Contributor

⌛️ Testing commit 7b20f45 with merge 05dfd5a...

@bors-servo bors-servo added a commit that referenced this pull request Jan 6, 2017
@bors-servo bors-servo Auto merge of #14833 - emilio:mq-refactor, r=heycam,bz
style: Isolate the soon-to-be style-backend-specific code from the media_query module.

The idea here is having (for convenience) different `Expression` and `Device` representations for Gecko.

Both will be implemented in rust, but will use `nsMediaFeatures` as a source of media features instead of the Servo bits.

Does it sound good?

r? @heycam or @Manishearth

<!-- 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/14833)
<!-- Reviewable:end -->
05dfd5a
@bors-servo
Contributor

💔 Test failed - linux-rel-css

@emilio
Member
emilio commented Jan 6, 2017
@bors-servo
Contributor

⌛️ Testing commit 7b20f45 with merge 8148b94...

@bors-servo bors-servo added a commit that referenced this pull request Jan 6, 2017
@bors-servo bors-servo Auto merge of #14833 - emilio:mq-refactor, r=heycam,bz
style: Isolate the soon-to-be style-backend-specific code from the media_query module.

The idea here is having (for convenience) different `Expression` and `Device` representations for Gecko.

Both will be implemented in rust, but will use `nsMediaFeatures` as a source of media features instead of the Servo bits.

Does it sound good?

r? @heycam or @Manishearth

<!-- 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/14833)
<!-- Reviewable:end -->
8148b94
@bors-servo
Contributor

💔 Test failed - mac-rel-wpt1

@emilio
Member
emilio commented Jan 6, 2017
@bors-servo
Contributor

⚡️ Previous build results for android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev are reusable. Rebuilding only mac-rel-wpt1...

@bors-servo
Contributor

💥 Test timed out

@emilio
Member
emilio commented Jan 7, 2017

Bots is seriously trolling...

@bors-servo retry force

@heycam
Member
heycam commented Jan 7, 2017

Is bors stuck?

@bors-servo retry

emilio added some commits Jan 2, 2017
@emilio emilio style: Remove unneeded allow's.
They're useless right now.
7788f43
@emilio emilio style: Isolate the soon-to-be style-backend-specific from the media_q…
…uery module.
c0cf847
@emilio
Member
emilio commented Jan 7, 2017

Rebased to give the PR another commit hash.

@bors-servo r=heycam,bz

@bors-servo
Contributor

📌 Commit c0cf847 has been approved by heycam,bz

@bors-servo
Contributor

⌛️ Testing commit c0cf847 with merge fdddeb1...

@bors-servo bors-servo added a commit that referenced this pull request Jan 7, 2017
@bors-servo bors-servo Auto merge of #14833 - emilio:mq-refactor, r=heycam,bz
style: Isolate the soon-to-be style-backend-specific code from the media_query module.

The idea here is having (for convenience) different `Expression` and `Device` representations for Gecko.

Both will be implemented in rust, but will use `nsMediaFeatures` as a source of media features instead of the Servo bits.

Does it sound good?

r? @heycam or @Manishearth

<!-- 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/14833)
<!-- Reviewable:end -->
fdddeb1
@bors-servo bors-servo merged commit c0cf847 into servo:master Jan 7, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment