style: Move text-align outside of the mako file. #19576

canova · 2017-12-15T17:42:27Z

I will need this refactoring before my next job. I didn't actually fix the FIXME's along the way. My other PR probably will cover these.

./mach build -d does not report any errors
./mach test-tidy does not report any errors

This change is

highfive · 2017-12-15T17:42:31Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/inherited_text.mako.rs, components/style/values/computed/text.rs, components/style/values/specified/mod.rs, components/style/values/computed/mod.rs, components/style/values/specified/text.rs and 1 more
@emilio: components/style/properties/longhand/inherited_text.mako.rs, components/style/values/computed/text.rs, components/style/values/specified/mod.rs, components/style/values/computed/mod.rs, components/style/values/specified/text.rs and 1 more

highfive · 2017-12-15T17:42:33Z

Warning

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

canova · 2017-12-15T17:43:01Z

r? @emilio or @Manishearth

Manishearth · 2017-12-15T18:20:48Z

components/style/values/specified/text.rs

+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
+pub enum TextAlign {
+    /// Keyword value of text-align property.
+    Keyword(ComputedTextAlign),


Given that ComputedTextAlign is used in the specified value I'd prefer if it were in this file as TextAlignKeyword and computed::TextAlign is an alias

emilio

r=me with @Manishearth's comment addressed and mine.

emilio · 2017-12-15T18:30:47Z

components/style/values/specified/text.rs

+        }
+        #[cfg(feature = "gecko")]
+        {
+            if let Ok(_) = input.try(|i| i.expect_ident_matching("match-parent")) {


This doesn't need to use if let, just:

input.expect_ident_matching("match_parent")?; return Ok(TextAlign::MatchParent);

emilio · 2017-12-15T18:31:30Z

components/style/values/computed/text.rs

+        impl TextAlign {
+            /// Convert computed TextAlign into u32
+            pub fn to_u32(self) -> u32 {
+                match self {


This is super stupid btw (not your fault of course). I should just land the patch to derive(Parse) for keywords and use self as u32.

I'll actually land it, gimme a sec :)


        Auto merge of #19578 - emilio:parse-keyword, r=Manishearth,canaltinova

Allow deriving Parse on keywords. This makes patches like #19576 much easier.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19578)


        style_derive: Make parse derive work with C like enums.

canova · 2017-12-16T21:51:03Z

@emilio Added a small commit to do that Parse derive. Could you look at it?

emilio

Arrg, still unfortunate we need the macros just for from_u32, but it's fine for now. We can maybe auto-generate that in the future.

emilio · 2017-12-16T23:10:16Z

components/style/values/specified/text.rs

+            input.expect_ident_matching("match_parent")?;
+            return Ok(TextAlign::MatchParent);
+        }
+        return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));


Travis has failed because this is unreachable on Gecko.

canova · 2017-12-17T03:58:27Z

Actually let me create a ToKeywordEnum proc macro to implement from_u32. So we can get rid of that macro.

canova · 2017-12-17T04:07:13Z

Can you look at it again to the new commit?

emilio · 2017-12-17T11:32:48Z

components/style_derive/to_keyword_enum.rs

+
+    // TODO(canaltinova): We can create a trait for that.
+    quote! {
+        impl #name {


It'd be nice to make it a real trait if possible. also, it's kind of unfortunate that we fix this to u32 enums, isn't it?

I think the macro is ok for now if there's only one use case.

canova · 2017-12-18T17:54:41Z

Nvm, it doesn't worth the effort. Changed back to macro.
@bors-servo r=Manishearth,emilio

bors-servo · 2017-12-18T17:54:42Z

📌 Commit 8c2c478 has been approved by Manishearth,emilio

emilio · 2017-12-18T17:56:48Z

components/style/values/specified/text.rs

+        }
+        #[cfg(feature = "gecko")]
+        {
+            input.expect_ident_matching("match_parent")?;


match-parent (s/_/-/)

emilio · 2017-12-18T17:57:02Z

@bors-servo r-

emilio · 2017-12-18T17:57:34Z

(Feel free to land with that fixed and the rest of the patch sanity-checked, thanks!)


        style: Move text-align outside of the mako file.

canova · 2017-12-18T18:01:46Z

Oops, thanks!
@bors-servo r=Manishearth,emilio

bors-servo · 2017-12-18T18:01:47Z

📌 Commit 9882d0b has been approved by Manishearth,emilio

bors-servo · 2017-12-18T18:12:53Z

⌛ Testing commit 9882d0b with merge a9c1797...


        Auto merge of #19576 - canaltinova:text-align, r=Manishearth,emilio

style: Move text-align outside of the mako file. I will need this refactoring before my next job. I didn't actually fix the FIXME's along the way. My other PR probably will cover these. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19576)

bors-servo · 2017-12-18T19:16:56Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: Manishearth,emilio
Pushing a9c1797 to master...

highfive assigned jdm Dec 15, 2017

highfive added the S-awaiting-review label Dec 15, 2017

highfive assigned emilio and unassigned jdm Dec 15, 2017

Manishearth approved these changes Dec 15, 2017

View changes

emilio approved these changes Dec 15, 2017

View changes

emilio reviewed Dec 15, 2017

View changes

This was referenced Dec 15, 2017

Allow deriving Parse on keywords. #19578

Merged

Move CSS longhands outside of mako #19015

Closed

style_derive: Make parse derive work with C like enums.

3ad2687

canova force-pushed the canova:text-align branch from 5644054 to e0e798a Dec 16, 2017

emilio approved these changes Dec 16, 2017

View changes

emilio reviewed Dec 16, 2017

View changes

canova force-pushed the canova:text-align branch from e0e798a to a2c24b0 Dec 17, 2017

emilio reviewed Dec 17, 2017

View changes

canova force-pushed the canova:text-align branch from a2c24b0 to 8c2c478 Dec 18, 2017

highfive assigned Manishearth and unassigned emilio Dec 18, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 18, 2017

emilio requested changes Dec 18, 2017

View changes

components/style/values/specified/text.rs Outdated

}

#[cfg(feature = "gecko")]

{

input.expect_ident_matching("match_parent")?;

This comment has been minimized.

Sign in to view

emilio Dec 18, 2017
Member

match-parent (s/_/-/)

style: Move text-align outside of the mako file.

Loading status checks…

9882d0b

canova force-pushed the canova:text-align branch from 8c2c478 to 9882d0b Dec 18, 2017

highfive added S-awaiting-review and removed S-awaiting-merge labels Dec 18, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 18, 2017

bors-servo merged commit 9882d0b into servo:master Dec 18, 2017
3 checks passed

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

highfive removed the S-awaiting-merge label Dec 18, 2017

canova deleted the canova:text-align branch Dec 22, 2017

servo / servo

style: Move text-align outside of the mako file. #19576

style: Move text-align outside of the mako file. #19576

canova commented Dec 15, 2017 •

edited by SimonSapin

highfive commented Dec 15, 2017

highfive commented Dec 15, 2017

canova commented Dec 15, 2017

This comment has been minimized.

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

canova commented Dec 16, 2017

emilio left a comment

This comment has been minimized.

canova commented Dec 17, 2017

canova commented Dec 17, 2017

This comment has been minimized.

canova commented Dec 18, 2017

bors-servo commented Dec 18, 2017

This comment has been minimized.

emilio commented Dec 18, 2017

emilio commented Dec 18, 2017

canova commented Dec 18, 2017

bors-servo commented Dec 18, 2017

bors-servo commented Dec 18, 2017

bors-servo commented Dec 18, 2017

servo / servo

Join GitHub today

style: Move text-align outside of the mako file. #19576

style: Move text-align outside of the mako file. #19576

Conversation

canova commented Dec 15, 2017 • edited by SimonSapin

highfive commented Dec 15, 2017

highfive commented Dec 15, 2017

canova commented Dec 15, 2017

This comment has been minimized.

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

canova commented Dec 16, 2017

emilio left a comment

This comment has been minimized.

canova commented Dec 17, 2017

canova commented Dec 17, 2017

This comment has been minimized.

canova commented Dec 18, 2017

bors-servo commented Dec 18, 2017

This comment has been minimized.

emilio commented Dec 18, 2017

emilio commented Dec 18, 2017

canova commented Dec 18, 2017

bors-servo commented Dec 18, 2017

bors-servo commented Dec 18, 2017

bors-servo commented Dec 18, 2017

canova commented Dec 15, 2017 •

edited by SimonSapin