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

Jump to bottom

implement the all shorthand #16413

Merged
merged 3 commits into from Apr 14, 2017
Merged

implement the all shorthand #16413

merged 3 commits into from Apr 14, 2017

Conversation

heycam
Copy link
Contributor

@heycam heycam commented Apr 13, 2017 •
edited by larsbergstrom

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/gecko.mako.rs, components/style/build_gecko.rs, components/style/properties/helpers.mako.rs, components/style/properties/properties.mako.rs
  • @emilio: components/style/properties/gecko.mako.rs, components/style/build_gecko.rs, components/style/properties/helpers.mako.rs, components/style/properties/properties.mako.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 13, 2017
@highfive
Copy link

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!

@heycam
Copy link
Contributor Author

heycam commented Apr 13, 2017

@bors-servo r=emilio

(Reviewed in the Gecko bug.)

@bors-servo
Copy link
Contributor

📌 Commit e983eab has been approved by emilio

@highfive highfive assigned emilio and unassigned nox Apr 13, 2017
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 13, 2017
@heycam
Copy link
Contributor Author

heycam commented Apr 13, 2017

@bors-servo p=1

(Bumping this so I'll be around to land the corresponding Gecko bug.)

@bors-servo
Copy link
Contributor

⌛ Testing commit e983eab with merge e2023a4...

bors-servo pushed a commit that referenced this pull request Apr 13, 2017
Auto merge of #16413 - heycam:all, r=emilio
e2023a4 
implement the all shorthand

From https://bugzilla.mozilla.org/show_bug.cgi?id=1356125.

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

💔 Test failed - windows-msvc-dev

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 13, 2017
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 13, 2017
@heycam
Copy link
Contributor Author

heycam commented Apr 13, 2017

@bors-servo r=emilio

@bors-servo
Copy link
Contributor

📌 Commit 56ab106 has been approved by emilio

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 13, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 56ab106 with merge 24af7a4...

bors-servo pushed a commit that referenced this pull request Apr 13, 2017
Auto merge of #16413 - heycam:all, r=emilio
24af7a4 
implement the all shorthand

From https://bugzilla.mozilla.org/show_bug.cgi?id=1356125.

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

💔 Test failed - windows-msvc-dev

@highfive highfive added S-tests-failed The changes caused existing tests to fail. S-awaiting-review There is new code that needs to be reviewed. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. S-tests-failed The changes caused existing tests to fail. labels Apr 13, 2017
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 14, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit 081da2a with merge 2afd5a58c703a9544127d98a8dd69001533a794d...

@bors-servo
Copy link
Contributor

💔 Test failed - windows-msvc-dev

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Apr 14, 2017
@bors-servo
Copy link
Contributor

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Apr 14, 2017
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 14, 2017
@heycam
Copy link
Contributor Author

heycam commented Apr 14, 2017

r? @emilio

(The bindings update has a conflict but I'll update that later.)

emilio
emilio approved these changes Apr 14, 2017
View reviewed changes
Copy link
Member

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with that, thanks!

components/style/properties/properties.mako.rs Outdated
@@ -868,8 +916,10 @@ impl PropertyId {
/// Includes shorthands before expansion
pub enum ParsedDeclaration {
% for shorthand in data.shorthands:
% if shorthand.name != "all":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a comment here to say why this is done?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, use shorthands_except_all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the other one can't use shorthands_except_all since it only surrounds part of the for loop body.

components/style/properties/properties.mako.rs
@@ -910,6 +960,7 @@ impl ParsedDeclaration {
overwrite_more_important: bool) -> bool {
match self {
% for shorthand in data.shorthands:
% if shorthand.name != "all":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shorthands_except_all?

components/style/properties/properties.mako.rs
if input.seen_var_functions() {
input.reset(start);
let (first_token_type, css) = try!(
::custom_properties::parse_non_custom_with_var(input));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to share this, somehow... But I guess it's not a huge deal. Please add a comment in the relevant code pointing to this one so we can remember if we need to update it.

@heycam
Copy link
Contributor Author

heycam commented Apr 14, 2017

@bors-servo r=emilio

@bors-servo
Copy link
Contributor

📌 Commit c488e71 has been approved by emilio

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Apr 14, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit c488e71 with merge 1bf10a9...

bors-servo pushed a commit that referenced this pull request Apr 14, 2017
Auto merge of #16413 - heycam:all, r=emilio
1bf10a9 
implement the all shorthand

From https://bugzilla.mozilla.org/show_bug.cgi?id=1356125.

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

☀️ 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 1bf10a9 to master...

@bors-servo bors-servo merged commit c488e71 into servo:master Apr 14, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 14, 2017
@bors-servo bors-servo mentioned this pull request Apr 14, 2017
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio emilio approved these changes

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@heycam @highfive @bors-servo @emilio @nox