#13875 - Implement parsing/serialization for font-size-adjust. #14125

chenpighead · 2016-11-08T08:11:21Z

Implement parsing/serialization for font-size-adjust.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
[X ] These changes fix Implement parsing/serialization for font-size-adjust #13875 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-11-08T08:11:28Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Ms2ger (or someone else) soon.

highfive · 2016-11-08T08:11:28Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/longhand/font.mako.rs
@emilio: components/style/properties/longhand/font.mako.rs

highfive · 2016-11-08T08:11:29Z

Warning

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

wafflespeanut · 2016-11-08T08:12:59Z

components/style/properties/longhand/font.mako.rs

+    }
+
+    pub mod computed_value {
+        use cssparser::ToCss;


As of #14089, we make use of style_traits::ToCss. This was a recent change, and the guide should be updated :)

Filed #14128 for this.

wafflespeanut · 2016-11-08T08:14:55Z

components/style/properties/longhand/font.mako.rs

+
+        match try!(input) {
+            Token::Ident(ref value) if value.eq_ignore_ascii_case("none") =>
+            Ok(SpecifiedValue::None),


Indent here (and below) please... (to distinguish the match pattern from the expression)

Oops. You're right. Should have done this. Thanks.

canova · 2016-11-08T09:21:33Z

components/style/properties/longhand/font.mako.rs

+    #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
+    pub enum SpecifiedValue {
+        None,
+        Number(CSSFloat),


You can use Number struct here (which stores number as CSSFloat but it has useful implementations). And in parsing function, you can try to parse number directly with:
SpecifiedValue::Number(try!(Number::parse_non_negative(input)))

canova · 2016-11-08T09:25:32Z

components/style/properties/longhand/font.mako.rs

+        use cssparser::Token;
+        use std::ascii::AsciiExt;
+
+        match try!(input) {


After converting to Number struct, you can return early like this:

if input.try(|input| input.expect_ident_matching("none")).is_ok() { return Ok(SpecifiedValue::None); }

And you can try to parse number after that. This is a good example for what I say I think

Thank you for these hints. Code looks much better now. :-)

Manishearth · 2016-11-08T14:55:31Z

components/style/properties/longhand/font.mako.rs

+            return Ok(SpecifiedValue::None);
+        }
+
+        Ok(SpecifiedValue::Number(try!(Number::parse_non_negative(input))))


We probably should file a spec bug for making this explicit

Filed w3c/csswg-drafts#702

Manishearth · 2016-11-08T14:56:36Z

@bors-servo r+

bors-servo · 2016-11-08T14:56:38Z

📌 Commit 08c47c4 has been approved by Manishearth

Manishearth · 2016-11-08T14:56:53Z

components/style/properties/longhand/font.mako.rs

@@ -349,6 +349,55 @@ ${helpers.single_keyword("font-variant",
    }
 </%helpers:longhand>

+// https://www.w3.org/TR/css-fonts-3/#font-size-adjust-prop
+// FIXME: This prop should be animatable


If you're going to work on the gecko glue this is easy to fix.

bors-servo · 2016-11-08T16:39:33Z

⌛ Testing commit 08c47c4 with merge 2e2eab0...

#13875 - Implement parsing/serialization for font-size-adjust.  Implement parsing/serialization for font-size-adjust. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X ] These changes fix #13875 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14125)

bors-servo · 2016-11-08T17:31:29Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

Manishearth · 2016-11-08T17:32:53Z

are you planning on working on the gecko stuff next?

chenpighead · 2016-11-09T01:51:09Z

Yes, I'd like to do it. Should I file another issue? Or I could just send a PR and ask for help there?

Manishearth · 2016-11-09T01:57:09Z

Just a PR is fine. Feel free to ask any questions here, in IRC (or in person to Shing if you can find him 😜 )

Stylo - gecko glue code for font-size-adjust.  Implement the gecko-side glue code for font-size-adjust. This is a followup for #14125, which is originally filed in #13875. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14227)

highfive assigned Ms2ger Nov 8, 2016

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

wafflespeanut reviewed Nov 8, 2016

View reviewed changes

chenpighead force-pushed the font-size-adjust#13875 branch from e8bf4e2 to 5c897de Compare November 8, 2016 08:40

canova reviewed Nov 8, 2016

View reviewed changes

servo#13875 - Implement parsing/serialization for font-size-adjust.

08c47c4

chenpighead force-pushed the font-size-adjust#13875 branch from 5c897de to 08c47c4 Compare November 8, 2016 09:59

Manishearth reviewed Nov 8, 2016

View reviewed changes

highfive assigned Manishearth and unassigned Ms2ger Nov 8, 2016

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 Nov 8, 2016

Manishearth reviewed Nov 8, 2016

View reviewed changes

bors-servo merged commit 08c47c4 into servo:master Nov 8, 2016

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

This was referenced Nov 13, 2016

Implement parsing/serialization for font-size-adjust #13875

Closed

Stylo - gecko glue code for font-size-adjust. #14227

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#13875 - Implement parsing/serialization for font-size-adjust. #14125

#13875 - Implement parsing/serialization for font-size-adjust. #14125

chenpighead commented Nov 8, 2016 •

edited by larsbergstrom

highfive commented Nov 8, 2016

highfive commented Nov 8, 2016

highfive commented Nov 8, 2016

wafflespeanut Nov 8, 2016

chenpighead Nov 8, 2016

chenpighead Nov 8, 2016

wafflespeanut Nov 8, 2016

chenpighead Nov 8, 2016

canova Nov 8, 2016

canova Nov 8, 2016

chenpighead Nov 8, 2016

Manishearth Nov 8, 2016

chenpighead Nov 10, 2016

Manishearth commented Nov 8, 2016

bors-servo commented Nov 8, 2016

Manishearth Nov 8, 2016

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

Manishearth commented Nov 8, 2016

chenpighead commented Nov 9, 2016

Manishearth commented Nov 9, 2016

#13875 - Implement parsing/serialization for font-size-adjust. #14125

#13875 - Implement parsing/serialization for font-size-adjust. #14125

Conversation

chenpighead commented Nov 8, 2016 • edited by larsbergstrom

highfive commented Nov 8, 2016

highfive commented Nov 8, 2016

highfive commented Nov 8, 2016

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

Choose a reason for hiding this comment

Manishearth commented Nov 8, 2016

bors-servo commented Nov 8, 2016

Choose a reason for hiding this comment

bors-servo commented Nov 8, 2016

bors-servo commented Nov 8, 2016

Manishearth commented Nov 8, 2016

chenpighead commented Nov 9, 2016

Manishearth commented Nov 9, 2016

chenpighead commented Nov 8, 2016 •

edited by larsbergstrom