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

Improve sequence values in style #17530

Merged
merged 6 commits into from Jun 27, 2017

Conversation

@nox
Copy link
Member

commented Jun 27, 2017

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jun 27, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/gecko.mako.rs, components/style/values/mod.rs, components/style/parser.rs, components/style/counter_style/mod.rs, components/style/values/specified/mod.rs and 11 more
  • @emilio: components/layout/display_list_builder.rs, components/style/properties/gecko.mako.rs, components/style/values/mod.rs, components/style/parser.rs, components/style/counter_style/mod.rs and 13 more
@highfive

This comment has been minimized.

Copy link

commented Jun 27, 2017

warning Warning warning

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

@highfive highfive assigned emilio and unassigned mbrubeck Jun 27, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

⌛️ Trying commit c9f8acb with merge f122bd9...

bors-servo added a commit that referenced this pull request Jun 27, 2017
Auto merge of #17530 - servo:derive-all-the-things, r=<try>
Improve sequence values in style
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

@nox nox force-pushed the derive-all-the-things branch 2 times, most recently from 6be377e to 393e215 Jun 27, 2017

@nox

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2017

Had not made the generated ToCss impl use Separator::separator().

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecb9d5e1dacfcdac521d53842b7a50226df96449

@emilio
emilio approved these changes Jun 27, 2017
}
debug_assert!(v.0.len() == self.gecko.mFilters.len());
debug_assert!(v.len() == self.gecko.mFilters.len());

This comment has been minimized.

Copy link
@emilio

emilio Jun 27, 2017

Member

while you're here, you can change this to debug_assert_eq! if you want.

}

fn add(&self, other: &Self) -> Result<Self, ()> {
Ok(self.0.iter().chain(other.0.iter()).cloned().collect::<Vec<_>>().into())
Ok(AnimatedFilterList(
self.0.iter().chain(other.0.iter()).cloned().collect::<Vec<_>>(),

This comment has been minimized.

Copy link
@emilio

emilio Jun 27, 2017

Member

Do you really need the type annotation?

/// type to indicate that a serialized list of elements of this type is
/// separated by commas, but spaces without commas are also allowed when
/// parsing.
pub struct CommaWithSpace;

This comment has been minimized.

Copy link
@emilio

emilio Jun 27, 2017

Member

CommaOrWhitespace, maybe? (no big deal anyway)

@@ -240,14 +246,41 @@ impl Separator for Space {
where
F: for<'tt> FnMut(&mut Parser<'i, 'tt>) -> Result<T, ParseError<'i, E>>
{
let mut results = vec![];
let mut results = vec![parse_one(input)?];

This comment has been minimized.

Copy link
@emilio

emilio Jun 27, 2017

Member

This should be in the previous commit, right?

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

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

@nox nox force-pushed the derive-all-the-things branch from 393e215 to 9fbb4f4 Jun 27, 2017

@nox nox force-pushed the derive-all-the-things branch from 9fbb4f4 to 395f6be Jun 27, 2017

@nox

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2017

@bors-servo r=emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

📌 Commit 395f6be has been approved by emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

⌛️ Testing commit 395f6be with merge de0ee6c...

bors-servo added a commit that referenced this pull request Jun 27, 2017
Auto merge of #17530 - servo:derive-all-the-things, r=emilio
Improve sequence values in style

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

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2017

@bors-servo bors-servo merged commit 395f6be into master Jun 27, 2017

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.