style: Move font-variant-alternates outside of mako #19167

CYBAI · 2017-11-09T17:53:31Z

This is a sub-PR of #19015
r? emilio

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Move font-variant-alternates outside of mako. #19166 (github issue number if applicable).
These changes do not require tests

This change is

highfive · 2017-11-09T17:53:36Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/gecko.mako.rs, components/style/values/specified/font.rs, components/style/values/specified/mod.rs, components/style/values/computed/mod.rs, components/style/values/computed/font.rs and 1 more
@canaltinova: components/style/properties/gecko.mako.rs, components/style/values/specified/font.rs, components/style/values/specified/mod.rs, components/style/values/computed/mod.rs, components/style/values/computed/font.rs and 1 more
@emilio: components/style/properties/gecko.mako.rs, components/style/values/specified/font.rs, components/style/values/specified/mod.rs, components/style/values/computed/mod.rs, components/style/values/computed/font.rs and 1 more

highfive · 2017-11-09T17:53:37Z

Warning

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

CYBAI · 2017-11-09T17:58:19Z

components/style/values/specified/font.rs

+            },
+            VariantAlternates::HistoricalForms => {
+                dest.write_str("historical-forms")
+            },


Every dest.write_str write with different string for each variant so I just type them by hand.
Not sure if there's any better and graceful way to write this pattern matching.
If yes, I'm willing to update it!

cc @jdm @emilio

I think you could derive(ToCss) here using #[css(function)] annotations.

I'll try to implement ToCss for Box<[CustomIdent]> in another PR.
Then, I'll update this.

emilio

Looks great! Just one bug I noticed, and a couple nits. Thanks as always!

emilio · 2017-11-09T17:56:42Z

components/style/values/computed/mod.rs

@@ -36,7 +36,10 @@ pub use self::angle::Angle;
 pub use self::background::{BackgroundSize, BackgroundRepeat};
 pub use self::border::{BorderImageSlice, BorderImageWidth, BorderImageSideWidth};
 pub use self::border::{BorderRadius, BorderCornerRadius, BorderSpacing};
-pub use self::font::{FontSize, FontSizeAdjust, FontWeight, MozScriptLevel, MozScriptMinSize, XTextZoom};
+pub use self::font::{


nit: Use two lines for imports instead of breaking them into multiple lines, see the border imports on the line above.

emilio · 2017-11-09T18:00:10Z

components/style/values/specified/font.rs

+bitflags! {
+    #[cfg_attr(feature = "servo", derive(MallocSizeOf))]
+    /// Flags of variant alternates in bit
+    pub struct ParsingFlags: u8 {


Let's call this something more specific like VariantAlternatesParsingFlags.

Also, no need for pub I think.

Oh, sorry for that I forgot to remove the pub. I'll update it!

emilio · 2017-11-09T18:43:41Z

components/style/values/specified/font.rs

+            // FIXME: remove clone() when lifetimes are non-lexical
+            match input.next()?.clone() {
+                Token::Ident(ref ident) => {
+                    if *ident == "historical-forms" {


This should be ident.eq_ignore_ascii_case("historical-forms"), could you add a test for this?

Something in the lines of:

<!doctype html> <div style="font-variant-alternates: Historical-Forms"></div> <script> getComputedStyle(document.querySelector('div')).fontVariantAlternates == "historical-forms" </script>

Though... I guess it would need to be a test for Firefox, not servo, so let's fix it and I'll land the test in https://bugzilla.mozilla.org/show_bug.cgi?id=1415946

emilio · 2017-11-09T18:45:28Z

components/style/values/specified/font.rs

+                                let idents = i.parse_comma_separated(|i| {
+                                    let location = i.current_source_location();
+                                    CustomIdent::from_ident(location, i.expect_ident()?, &[])
+                                })?;


Hmm, the duplication here is slightly worrying, but it's not too much, so I guess it's fine.

Hmm... yes, I also would like to improve them with checking with multiple patterns of pattern matching; however, there're very small different inside the match body. If there's a good way to solve this problem, I think we can reduce this match.

emilio · 2017-11-09T18:45:48Z

components/style/values/specified/mod.rs

@@ -30,7 +30,10 @@ pub use self::align::{AlignItems, AlignJustifyContent, AlignJustifySelf, Justify
 pub use self::background::{BackgroundRepeat, BackgroundSize};
 pub use self::border::{BorderCornerRadius, BorderImageSlice, BorderImageWidth};
 pub use self::border::{BorderImageSideWidth, BorderRadius, BorderSideWidth, BorderSpacing};
-pub use self::font::{FontSize, FontSizeAdjust, FontWeight, MozScriptLevel, MozScriptMinSize, XTextZoom};
+pub use self::font::{
+    FontSize, FontSizeAdjust, FontWeight, FontVariantAlternates,


Same regarding multiline imports.

bors-servo · 2017-11-10T05:18:06Z

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

emilio · 2017-11-10T23:49:12Z

Looks good! Let's try to derive it in a followup.

@bors-servo r+

bors-servo · 2017-11-10T23:49:12Z

📌 Commit aeae311 has been approved by emilio

bors-servo · 2017-11-10T23:49:17Z

⌛ Testing commit aeae311 with merge 33fa728...

…milio style: Move font-variant-alternates outside of mako This is a sub-PR of #19015 r? emilio --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #19166 (github issue number if applicable). - [x] These changes do not require tests  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19167)

bors-servo · 2017-11-11T00:46:59Z

☀️ 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: emilio
Pushing 33fa728 to master...

highfive assigned emilio Nov 9, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 9, 2017

CYBAI commented Nov 9, 2017

View reviewed changes

emilio reviewed Nov 9, 2017

View reviewed changes

highfive added the S-needs-rebase There are merge conflict errors. label Nov 10, 2017

CYBAI force-pushed the font-variant-alternates-out-of-mako branch from 5d96982 to 773e7fc Compare November 10, 2017 18:15

style: Move font-variant-alternates outside of mako

aeae311

CYBAI force-pushed the font-variant-alternates-out-of-mako branch from 773e7fc to aeae311 Compare November 10, 2017 18:21

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 Nov 10, 2017

bors-servo merged commit aeae311 into servo:master Nov 11, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 11, 2017

CYBAI deleted the font-variant-alternates-out-of-mako branch November 11, 2017 02:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: Move font-variant-alternates outside of mako #19167

style: Move font-variant-alternates outside of mako #19167

CYBAI commented Nov 9, 2017 •

edited by emilio

highfive commented Nov 9, 2017

highfive commented Nov 9, 2017

CYBAI Nov 9, 2017

emilio Nov 9, 2017

CYBAI Nov 10, 2017

emilio left a comment

emilio Nov 9, 2017

emilio Nov 9, 2017

emilio Nov 9, 2017

CYBAI Nov 10, 2017

emilio Nov 9, 2017

emilio Nov 9, 2017

CYBAI Nov 10, 2017

emilio Nov 9, 2017

bors-servo commented Nov 10, 2017

emilio commented Nov 10, 2017

bors-servo commented Nov 10, 2017

bors-servo commented Nov 10, 2017

bors-servo commented Nov 11, 2017

style: Move font-variant-alternates outside of mako #19167

style: Move font-variant-alternates outside of mako #19167

Conversation

CYBAI commented Nov 9, 2017 • edited by emilio

highfive commented Nov 9, 2017

highfive commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Nov 10, 2017

emilio commented Nov 10, 2017

bors-servo commented Nov 10, 2017

bors-servo commented Nov 10, 2017

bors-servo commented Nov 11, 2017

CYBAI commented Nov 9, 2017 •

edited by emilio