Implement ToCss serialization for CSSRules #14238

canova · 2016-11-15T21:16:53Z

Implementation of ToCss serialization for CSSRules. It requires #14190 to merge first to uncomment CssRule::Style branch in CssRule's ToCss implementation.

r? @Manishearth

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14195 (github issue number if applicable).
These changes do not require tests because it's serialization changes and I'm not sure there is a test for that.

This change is

highfive · 2016-11-15T21:16:57Z

Heads up! This PR modifies the following files:

@bholley: components/style/stylesheets.rs, components/style/keyframes.rs, components/style/font_face.rs, components/style/viewport.rs
@emilio: components/style/stylesheets.rs, components/style/keyframes.rs, components/style/font_face.rs, components/style/viewport.rs

highfive · 2016-11-15T21:16:58Z

Warning

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

canova · 2016-11-15T21:19:57Z

components/style/viewport.rs

@@ -92,6 +93,50 @@ declare_viewport_descriptor! {
    Orientation(Orientation),
 }

+impl ToCss for ViewportDescriptor {


ViewportDescriptor is being created with a macro but I couldn't find a way to generalize this code within macro. So I moved outside of it. Should I move it inside of macro?

Yes, please extend the macro. It will need to take in a string as well, so the invocation will be something like

declare_viewport_descriptor! { "min-width": MinWidth(ViewportLength), "max-width": MaxWidth(ViewportLength), ...

You could also do

declare_viewport_descriptor! { min-width: MinWidth(ViewportLength), max-width: MaxWidth(ViewportLength), ...

and use stringify!() on the identifiers. It may be easy to make the macro follow sets work that way. If you're having trouble, I can fix this up for you, so don't worry too much about it.

Manishearth · 2016-11-15T21:35:45Z

This needs spec links for the bits that are specced. For the bits that aren't, leave a comment saying so (and try to make a PR to add some spec text for this)

Manishearth

Overall I suggest you compare with the MDN pages and/or spec closely to ensure that the output will look the same as (any one valid way of providing) the input.

[By "any one valid way" I mean that if different CSS text can produce the same parsed rule, pick the simplest one for the ToCss.]

Manishearth · 2016-11-15T21:56:54Z

components/style/font_face.rs

+        try!(dest.write_str("; src: "));
+
+        let mut iter = self.sources.iter();
+        try!(iter.next().unwrap().to_css(dest));


It's valid for font face rules to not have sources, in which case there should be no src block.

Manishearth · 2016-11-15T21:56:54Z

components/style/keyframes.rs

@@ -82,6 +84,18 @@ pub struct Keyframe {
    pub block: Arc<RwLock<PropertyDeclarationBlock>>,
 }

+impl ToCss for Keyframe {


This doesn't write out the property declaration block.

https://developer.mozilla.org/en-US/docs/Web/CSS/@keyframes

A single keyframe rule looks like 68%, 72% { left: 50px; top: 50px; }. So you first print out a comma-separated list of percentages, and then serialize the block. I think you'll need to add the braces yourself.

Manishearth · 2016-11-15T21:56:54Z

components/style/stylesheets.rs

+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        match *self {
+            CssRule::Namespace(ref lock) => try!(lock.read().to_css(dest)),
+            // TODO: Uncomment here when Manishearth's CSSOM PR(#14190) lands


Just base it on top of the CSSOM PR for now, and update the css_text getters to use this (there already is commented code to use this)

Manishearth · 2016-11-15T21:56:54Z

components/style/stylesheets.rs

+            try!(dest.write_str(&*prefix));
+            try!(dest.write_str(" "));
+        }
+        try!(dest.write_str(&*self.url));


We must wrap this in url("..."). A plain quoted string will do too, but Firefox picks the quoted URL.

https://developer.mozilla.org/en-US/docs/Web/CSS/@namespace

Manishearth · 2016-11-15T21:56:54Z

components/style/stylesheets.rs

+    // https://drafts.csswg.org/cssom/#serialize-a-css-rule CSSMediaRule
+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        try!(dest.write_str("@media "));
+        try!(self.media_queries.read().to_css(dest));


It should be @media (queries...) {rules...}. You're missing the parentheses.

https://developer.mozilla.org/en-US/docs/Web/CSS/@media

Manishearth · 2016-11-15T21:56:54Z

components/style/viewport.rs

@@ -92,6 +93,50 @@ declare_viewport_descriptor! {
    Orientation(Orientation),
 }

+impl ToCss for ViewportDescriptor {


Yes, please extend the macro. It will need to take in a string as well, so the invocation will be something like

declare_viewport_descriptor! { "min-width": MinWidth(ViewportLength), "max-width": MaxWidth(ViewportLength), ...

You could also do

declare_viewport_descriptor! { min-width: MinWidth(ViewportLength), max-width: MaxWidth(ViewportLength), ...

and use stringify!() on the identifiers. It may be easy to make the macro follow sets work that way. If you're having trouble, I can fix this up for you, so don't worry too much about it.

canova · 2016-11-16T11:31:43Z

Oops sorry for the mistakes. Pushed the updated codes.

Also updated the macro rule, but I'm not so good with macros so maybe it can be improved :) I tried to make it like min-width: MinWidth(ViewportLength), but ident doesn't allow dash. And I couldn't use : in the middle because it's not allowed after expr type. I made it like this:
"min-width" => MinWidth(ViewportLength),

Manishearth · 2016-11-16T18:54:43Z

String is fine, actually even better. The reason I was suggesting idents was because the follow rules for idents are more lenient (so you could use a colon or whatever), but I forgot about the dash.

Manishearth · 2016-11-17T07:00:51Z

btw, my PR landed, you should rebase over master

Manishearth · 2016-11-17T07:09:50Z

components/style/font_face.rs

 #[derive(Debug, PartialEq, Eq)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub struct FontFaceRule {
    pub family: FontFamily,
    pub sources: Vec<Source>,
 }

+impl ToCss for FontFaceRule {
+    // Serialization of FontFaceRule is not specced.


I wanted to check if the whitespace matched browsers, and Chrome and Firefox don't match (this is unspecced and probably unimportant so this is ok). Chrome produces @font-face { font-family: foo; src: url("bar"); }, with the same whitespace as the (specced) CSSStyleRule serialization. Firefox produces the nice-looking

@font-face { font-family: "foo"; src: url("bar"); }

with newlines.

Since the rules with specced serialization just have spaces around each brace and after each semicolon, I think we should just stick with that (And if you're interested in speccing the rest, do the same then). This also seems to be what you've done here anyway, yay 😄

Firefox's way looks better but it's not specified this way in specced rules as you said. I think it's better to continue with the existing rule style too :)

canova · 2016-11-17T09:47:37Z

Rebased it over master.

Manishearth · 2016-11-17T18:23:28Z

@bors-servo r+

thanks!

bors-servo · 2016-11-17T18:23:29Z

📌 Commit b71e122 has been approved by Manishearth

Manishearth · 2016-11-17T22:20:07Z

@bors-servo p=22

bors-servo · 2016-11-17T23:46:51Z

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


        Implement ToCss serialization for CSSRules

canova · 2016-11-18T09:19:49Z

Added as_str function to SpecifiedUrl. It should work now.

Manishearth · 2016-11-18T16:12:07Z

@bors-servo r+

(you can r=manishearth this yourself fwiw)

bors-servo · 2016-11-18T16:12:08Z

📌 Commit fdbadcd has been approved by Manishearth

Manishearth · 2016-11-18T16:12:48Z

oh, you can't, I delegated the other PR but not this

@bors-servo delegate+

bors-servo · 2016-11-18T16:12:49Z

✌️ @canaltinova can now approve this pull request

bors-servo · 2016-11-18T16:15:53Z

⌛ Testing commit fdbadcd with merge 731264f...


        Auto merge of #14238 - canaltinova:cssom-tocss, r=Manishearth

@Manishearth

Implement ToCss serialization for CSSRules  Implementation of ToCss serialization for CSSRules. It requires #14190 to merge first to uncomment `CssRule::Style` branch in CssRule's ToCss implementation. r? @Manishearth ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14195 (github issue number if applicable). - [X] These changes do not require tests because it's serialization changes and I'm not sure there is a test for that.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14238)

bors-servo · 2016-11-18T17:15:04Z

💔 Test failed - linux-rel-wpt

canova · 2016-11-18T17:50:02Z

@bors-servo retry

#13776

bors-servo · 2016-11-18T19:20:22Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-wpt, mac-rel-wpt1...

bors-servo · 2016-11-18T20:13:57Z

💔 Test failed - linux-rel-wpt

highfive · 2016-11-18T20:14:00Z

  ▶ FAIL [expected PASS] /_mozilla/css/incremental_trailing_whitespace_a.html
  └   → /_mozilla/css/incremental_trailing_whitespace_a.html da54041086f2975f8e3c776d2283ad6609e6862a
/_mozilla/css/incremental_trailing_whitespace_ref.html f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b
Testing da54041086f2975f8e3c776d2283ad6609e6862a == f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b

canova · 2016-11-18T20:15:34Z

@bors-servo retry

#10473

bors-servo · 2016-11-18T20:54:36Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-wpt, mac-rel-wpt1...

bors-servo · 2016-11-18T21:39:42Z

☀️ 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

canova · 2016-11-18T21:49:59Z

🎉 🎉 🎉

mark-buer · 2016-11-18T21:59:54Z

components/style/font_face.rs

+    fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
+        match *self {
+            Source::Url(ref url) => {
+                try!(dest.write_str("local(\""));


Too late now... but are these ("local(" and "url(") around the wrong way?

Oh god... I must have mixed it during rebase or something 😞

Fix included in #14241. Good catch!


        Fixup error from servo#14238


        Fixup error from servo#14238

highfive assigned Manishearth Nov 15, 2016

highfive added the S-awaiting-review label Nov 15, 2016

canova reviewed Nov 15, 2016

View changes

Manishearth requested changes Nov 15, 2016

View changes

canova force-pushed the canova:cssom-tocss branch from 5c00e72 to 10bbf95 Nov 16, 2016

canova changed the title ~~[WIP] Implement ToCss serialization for CSSRules~~ Implement ToCss serialization for CSSRules Nov 16, 2016

canova force-pushed the canova:cssom-tocss branch 3 times, most recently from d4fba90 to 0254452 Nov 16, 2016

Manishearth reviewed Nov 17, 2016

View changes

canova force-pushed the canova:cssom-tocss branch from 0254452 to d4989f4 Nov 17, 2016

canova force-pushed the canova:cssom-tocss branch from d4989f4 to b71e122 Nov 17, 2016

canova mentioned this pull request Nov 17, 2016

[cssom] Specify serialization of CSSMediaRule and CSSFontFaceRule w3c/csswg-drafts#728

Merged

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 17, 2016

highfive added S-needs-rebase and removed S-awaiting-merge labels Nov 17, 2016

Manishearth force-pushed the canova:cssom-tocss branch from b71e122 to 5bb7175 Nov 17, 2016

highfive added the S-awaiting-review label Nov 17, 2016

Implement ToCss serialization for CSSRules

Loading status checks…

fdbadcd

canova force-pushed the canova:cssom-tocss branch from 5bb7175 to fdbadcd Nov 18, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Nov 18, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 18, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 18, 2016

highfive added the S-tests-failed label Nov 18, 2016

bors-servo merged commit fdbadcd into servo:master Nov 18, 2016
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

canova deleted the canova:cssom-tocss branch Nov 18, 2016

mark-buer reviewed Nov 18, 2016

View changes

Manishearth added a commit to Manishearth/servo that referenced this pull request Nov 18, 2016

Fixup error from servo#14238

a7b7459

Manishearth added a commit to Manishearth/servo that referenced this pull request Nov 23, 2016

Fixup error from servo#14238

0e19f45

servo / servo

Join GitHub today

Implement ToCss serialization for CSSRules #14238

Implement ToCss serialization for CSSRules #14238

Conversation

canova commented Nov 15, 2016 • edited by larsbergstrom

highfive commented Nov 15, 2016

highfive commented Nov 15, 2016

This comment has been minimized.

canova Nov 15, 2016 • edited

This comment has been minimized.

Manishearth commented Nov 15, 2016

Manishearth left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

canova commented Nov 16, 2016

Manishearth commented Nov 16, 2016

Manishearth commented Nov 17, 2016

This comment has been minimized.

This comment has been minimized.

canova commented Nov 17, 2016

Manishearth commented Nov 17, 2016

bors-servo commented Nov 17, 2016

Manishearth commented Nov 17, 2016

bors-servo commented Nov 17, 2016

canova commented Nov 18, 2016

Manishearth commented Nov 18, 2016

bors-servo commented Nov 18, 2016

Manishearth commented Nov 18, 2016

bors-servo commented Nov 18, 2016

bors-servo commented Nov 18, 2016

bors-servo commented Nov 18, 2016

canova commented Nov 18, 2016

bors-servo commented Nov 18, 2016

bors-servo commented Nov 18, 2016

highfive commented Nov 18, 2016

canova commented Nov 18, 2016

bors-servo commented Nov 18, 2016

bors-servo commented Nov 18, 2016

canova commented Nov 18, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

canova commented Nov 15, 2016 •

edited by larsbergstrom

canova Nov 15, 2016 •

edited