10933/shorthands #12572

craftytrickster · 2016-07-23T22:34:11Z

This implements the serialization of nearly all of the css shorthand properties with the following exceptions:

font - this may be implemented correctly, but I am not 100% sure
border-radius - I do not know how to implement this, since I am not familiar with how the property works
background - this is implemented, but I think that the implementation might be a tiny bit off.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #10933 (github issue number if applicable). Also fixes issue #11448

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

This change is

highfive · 2016-07-23T22:34:14Z

Heads up! This PR modifies the following files:

@bholley: components/style/properties/properties.mako.rs

highfive · 2016-07-23T22:34:16Z

Warning

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

emilio · 2016-07-25T00:20:48Z

Awesome work!

I think it still needs some refactoring to keep things clean, but overall the approach looks good to me. What do you think about the comments suggested below?

Thanks again for doing this! :)

-S-awaiting-review +S-needs-code-changes

Reviewed 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

components/style/properties/properties.mako.rs, line 462 [r11] (raw file):

                            where W: fmt::Write, I: Iterator<Item=&'a PropertyDeclaration> {
    match shorthand {
        Shorthand::Margin => try!(append_margin_shorthand(dest, declarations)),

This should probably be refactored to use a Mako loop.

components/style/properties/properties.mako.rs, line 481 [r11] (raw file):

        Shorthand::BorderTop => try!(append_border_top_shorthand(dest, declarations)),
        Shorthand::BorderRight => try!(append_border_right_shorthand(dest, declarations)),
        Shorthand::BorderBottom => try!(append_border_bottom_shorthand(dest, declarations)),

What about hoisting this to the shorthand module? That would keep things more self-contained, and we could do something like:

% for shorthand in data.shorthands:
    Shorthand::${shorthand.camel_case} => shorthands::{$shorthand.ident}::write(dest, declarations),
% endfor

Note that the write name is arbitrary, probably serialize is a better choice.

components/style/properties/properties.mako.rs, line 534 [r11] (raw file):

    for decl in declarations {
        match decl {

nit: I usually prefer matching the dereferenced value, looks cleaner:

match *decl {
    PropertyDeclaration::MarginTop(ref value) => // ...
    // ...
}

components/style/properties/properties.mako.rs, line 592 [r11] (raw file):

                   try!(single_value.to_css(dest));
               }
               else {

nit: keep the else if the same line of the rest of the closing brace: } else {.

Comments from Reviewable

craftytrickster · 2016-07-26T03:51:33Z

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

components/style/properties/properties.mako.rs, line 462 [r11] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

This should probably be refactored to use a Mako loop.

Done. It would appear that at some point I have to rebase from master, because two new shorthands have appeared in this mako loop (according to the travis build failure), that aren't on my current branch.

components/style/properties/properties.mako.rs, line 481 [r11] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

What about hoisting this to the shorthand module? That would keep things more self-contained, and we could do something like:
% for shorthand in data.shorthands:
    Shorthand::${shorthand.camel_case} => shorthands::{$shorthand.ident}::write(dest, declarations),
% endfor
Note that the write name is arbitrary, probably serialize is a better choice.

Done.

components/style/properties/properties.mako.rs, line 534 [r11] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

nit: I usually prefer matching the dereferenced value, looks cleaner:
match *decl {
    PropertyDeclaration::MarginTop(ref value) => // ...
    // ...
}

Done.

components/style/properties/properties.mako.rs, line 592 [r11] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

nit: keep the else if the same line of the rest of the closing brace: } else {.

Although I personally strongly prefer the other way, I have swallowed my pride and made the change.

Comments from Reviewable

emilio · 2016-07-26T04:02:20Z

I'd strongly prefer to move these functions to its own files into their own submodules, though if for some reason you don't have enough time or something, just let me know and we could fill a followup :)

Reviewed 1 of 1 files at r12.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

components/style/properties/properties.mako.rs, line 481 [r11] (raw file):

Previously, craftytrickster (David Raifaizen) wrote…

Done.

I meant hoisting the function to its own module (that is, in the `shorthand/xxx.mako.rs` file). I guess we could file a followup for this, but given you have to rebase already...

components/style/properties/properties.mako.rs, line 592 [r11] (raw file):

Previously, craftytrickster (David Raifaizen) wrote…

Although I personally strongly prefer the other way, I have swallowed my pride and made the change.

Not a big deal personally, but we try to adhere our code to the general rust convention. Actually I think test-tidy should have failed with a line starting by an `else {`.

Comments from Reviewable

craftytrickster · 2016-07-26T12:02:44Z

I was just joking about the pride thing, it's not a big deal to me. I misunderstood what you meant about moving to a module, I thought you meant move it to the shorthands mod, not another file, so I will make that change as well, in addition to looking into those new shorthands that were added.

craftytrickster · 2016-07-28T00:11:39Z

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

components/style/properties/properties.mako.rs, line 481 [r11] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

I meant hoisting the function to its own module (that is, in the shorthand/xxx.mako.rs file). I guess we could file a followup for this, but given you have to rebase already...

Done.

components/style/properties/properties.mako.rs, line 592 [r11] (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

Not a big deal personally, but we try to adhere our code to the general rust convention. Actually I think test-tidy should have failed with a line starting by an else {.

Done.

Comments from Reviewable

craftytrickster · 2016-07-28T00:55:18Z

The Travis build is failing due to the the below "missing" shorthand, yet I just rebased the latest from master, and I was able to build without any issue, I am not sure why this shorthand only appears on the Travis build.

Should I add a _ => catchall generic serialization in the append_shorthand_value function to get around these kinds of issues? The only problem I see with doing that is that when new shorthands are added, the person adding them might not realize they have to add a custom serialization function, since the catchall will allow them to ignore it.

/home/travis/build/servo/servo/target/geckolib/debug/build/style-b398ea74b908472c/out/properties.rs:31583:26: 31583:65 error: unresolved name `serialize__moz_outline_radius_shorthand` [E0425]

/home/travis/build/servo/servo/target/geckolib/debug/build/style-b398ea74b908472c/out/properties.rs:31583                     try!(serialize__moz_outline_radius_shorthand(dest, declarations)),

emilio · 2016-07-28T00:56:10Z

I'd love to see the comment below addressed, that being said, it's not a huge deal. The problem with it as written is that it separates the code needed to add a longhand into two different places.

Also, you need to take into account geckolib (the build for it is failing now because of a gecko-only shorthand). You can conditionally add that serialization with the if product == "gecko": check, but moving everything to its own module will solve this problem for you :)

To check the geckolib build, you can use ./mach build-geckolib.

Thanks for everything, this looks great! :)

-S-awaiting-review +S-needs-code-changes

Reviewed 2 of 2 files at r24.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

components/style/properties/properties.mako.rs, line 481 [r11] (raw file):

Previously, craftytrickster (David Raifaizen) wrote…

Done.

Heh, I meant to hoist that to the corresponding shorthand module, but I see it's engineered right now to declare a single function.

What would need to be done is something like changing the helpers.mako.rs file from:

pub fn parse_value(context: &ParserContext, input: &mut Parser) -> Result<Longhands, ()> {
    ${caller.body()}
}

to just:

${caller.body()}

and then change the uses of the helper to define both parse_value(..), with the current signature and contents, and serialize(..).

I'd love to see that done in this PR, but it's a bit tedious change though, so we might file a followup for it if you prefer.

Comments from Reviewable

emilio · 2016-07-28T00:57:48Z

@craftytrickster, sorry, just replied while you commented.

No, the build that is failing isn't servo's main build, it's the gecko port for servo's style system. Above is the command to build it. Let me know if you have any issues and I'll try to help :)

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

craftytrickster · 2016-07-28T01:12:24Z

I am not sure if it is a lot of work for you, but, would it be possible for you to provide me with one concrete example of changing one of the serializations to using hoisting and the helpers.mako file? Once I see exactly what you mean, I can port the rest of the changes to follow that convention.

If it is not easy for you to do quickly, I will try to take more time to analyze what is going on in the helpers.mako file and see if I can figure it out.

Thank you once again for the help.

emilio · 2016-07-28T01:33:57Z

Oh, sure! So, given we have the change I said in the previous review comment, we should port code like:

<%helpers:shorthand name="overflow" sub_properties="overflow-x overflow-y">
    use properties::longhands::{overflow_x, overflow_y};

    let overflow = try!(overflow_x::parse(context, input));
    Ok(Longhands {
        overflow_x: Some(overflow),
        overflow_y: Some(overflow_y::SpecifiedValue(overflow)),
    })
</%helpers:shorthand>

to something like:

<%helpers:shorthand name="overflow" sub_properties="overflow-x overflow-y">
    use properties::longhands::{overflow_x, overflow_y};
    // Probably a few includes missing here.

    pub fn parse_value(context: &ParserContext, input: &mut Parser) -> Result<Longhands, ()> {
        let overflow = try!(overflow_x::parse(context, input));
        Ok(Longhands {
            overflow_x: Some(overflow),
            overflow_y: Some(overflow_y::SpecifiedValue(overflow)),
        })
    }

    pub fn serialize<'a, W, I>(dest: &mut W,
                               appendable_value: AppendableValue<'a, I>,
                               is_first_serialization: &mut bool)
                               -> fmt::Result
        where W: fmt::Write, I: Iterator<Item=&'a PropertyDeclaration>
    {
        let declarations = match appendable_value {
            AppendableValue::DeclarationsForShorthand(_, declarations) => declarations,
            _ => return Err(fmt::Error)
        };

        // snip snip...

        write!(dest, ";")
    }
</%helpers:shorthand>

Does that make sense? :)

Also, don't thank me, thank you for all the work you're doing, it's awesome! :)

craftytrickster · 2016-07-28T12:40:08Z

@emilio thanks for the pointers above. When I have some free time over the next several days I will make the changes.

craftytrickster · 2016-08-01T12:57:18Z

Looks like it failed because I did not rebase master, the Url::parse method looks to have been changed. Should be an easy fix.

emilio · 2016-08-01T17:58:33Z

The approach looks so much cleaner now! Thanks!

I still need to do a bit more thorough review going through the spec for each property, but this looks good to me. Can you rebase so we can make a try run and see which tests need to change?

-S-awaiting-review +S-needs-rebase

Review status: 0 of 18 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

bors-servo · 2016-08-17T03:08:30Z

⌛ Testing commit 3085433 with merge d1ebcd5...


        Auto merge of #12572 - craftytrickster:10933/shorthands, r=emilio

10933/shorthands  This implements the serialization of nearly all of the css shorthand properties with the following exceptions: 1. font - this may be implemented correctly, but I am not 100% sure 2. border-radius - I do not know how to implement this, since I am not familiar with how the property works 3. background - this is implemented, but I think that the implementation might be a tiny bit off. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #10933 (github issue number if applicable). Also fixes issue #11448  - [X] 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/12572)

bors-servo · 2016-08-17T03:50:33Z

💔 Test failed - mac-rel-css

highfive · 2016-08-17T03:50:35Z

  ▶ FAIL [expected PASS] /css21_dev/html4/run-in-listitem-between-001.htm
  └   → /css21_dev/html4/run-in-listitem-between-001.htm af34c20eed63b2636b8a0a97c5c77991e2039bc1
/css21_dev/html4/reference/run-in-text-ref.htm 638e3ddce4ea49b428602b944937bb27f420bd63
Testing af34c20eed63b2636b8a0a97c5c77991e2039bc1 == 638e3ddce4ea49b428602b944937bb27f420bd63

  ▶ FAIL [expected PASS] /css21_dev/html4/run-in-listitem-between-002.htm
  └   → /css21_dev/html4/run-in-listitem-between-002.htm af34c20eed63b2636b8a0a97c5c77991e2039bc1
/css21_dev/html4/reference/run-in-text-ref.htm 638e3ddce4ea49b428602b944937bb27f420bd63
Testing af34c</span><span class="stdout">20eed63b2636b8a0a97c5c77991e2039bc1 == 638e3ddce4ea49b428602b944937bb27f420bd63

emilio · 2016-08-17T04:10:00Z

These seem intermittents, I filled #12901.

@bors-servo: retry

bors-servo · 2016-08-17T04:47:38Z

⌛ Testing commit 3085433 with merge 9d26d26...


        Auto merge of #12572 - craftytrickster:10933/shorthands, r=emilio

10933/shorthands  This implements the serialization of nearly all of the css shorthand properties with the following exceptions: 1. font - this may be implemented correctly, but I am not 100% sure 2. border-radius - I do not know how to implement this, since I am not familiar with how the property works 3. background - this is implemented, but I think that the implementation might be a tiny bit off. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #10933 (github issue number if applicable). Also fixes issue #11448  - [X] 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/12572)

bors-servo · 2016-08-17T05:35:15Z

💔 Test failed - linux-rel

highfive · 2016-08-17T05:35:17Z

  ▶ FAIL [expected PASS] /css-flexbox-1_dev/html/flex-flexitem-percentage-prescation.htm
  └   → /css-flexbox-1_dev/html/flex-flexitem-percentage-prescation.htm 549dc2995e7de786feb6065fc8fc76b53746d920
/css-flexbox-1_dev/html/reference/flex-flexitem-percentage-prescation-ref.htm 516cbcf7b447cf7d5a568a4f8b57f030555e6525
Testing 549dc2995e7de786feb6065fc8fc76b53746d920 == 516cbcf7b447cf7d5a568a4f8b57f030555e6525

  ▶ FAIL [expected PASS] /css21_dev/html4/run-in-listitem-between-001.htm
  └   → /css21_dev/html4/run-in-listitem-between-001.htm ef608dda75ea29fbec3a0a32f64cc80292e62d82
/css21_dev/html4/reference/run-in-text-ref.htm d068883ccfce1b3fdf24f9ea6b401c9a047b593c
Testing ef608dda75ea29fbec3a0a32f64cc80292e62d82 == d068883ccfce1b3fdf24f9ea6b401c9a047b593c

  ▶ FAIL [expected PASS] /css21_dev/html4/run-in-listitem-between-002.htm
  └   → /css21_dev/html4/run-in-listitem-between-002.htm ef608dda75ea29fbec3a0a32f64cc80292e62d82
/css21_dev/html4/reference/run-in-text-ref.htm d068883ccfce1b3fdf24f9ea6b401c9a047b593c
Testing ef608dda75ea29fbec3a0a32f64cc80292e62d82 == d068883ccfce1b3fdf24f9ea6b401c9a047b593c


        Restoring expected failures for several .ini files

craftytrickster · 2016-08-17T11:39:04Z

For some reason my local only passes when I remove the 3 .ini files corresponding to those failures. However, since Travis CI seems to disagree, I have added them back. Can someone please test the merge commit, hopefully it should go through now.

emilio · 2016-08-17T17:17:46Z

Oh, yeah, @shinglyu had the same problem, let's see.

@bors-servo: r+

bors-servo · 2016-08-17T17:17:47Z

📌 Commit d8c042a has been approved by emilio

bors-servo · 2016-08-17T17:17:49Z

⌛ Testing commit d8c042a with merge b61c456...


        Auto merge of #12572 - craftytrickster:10933/shorthands, r=emilio

10933/shorthands  This implements the serialization of nearly all of the css shorthand properties with the following exceptions: 1. font - this may be implemented correctly, but I am not 100% sure 2. border-radius - I do not know how to implement this, since I am not familiar with how the property works 3. background - this is implemented, but I think that the implementation might be a tiny bit off. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #10933 (github issue number if applicable). Also fixes issue #11448  - [X] 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/12572)

bors-servo · 2016-08-17T18:22:50Z

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

craftytrickster · 2016-08-17T18:23:59Z

Finally!

highfive assigned asajeffrey Jul 23, 2016

highfive added the S-awaiting-review label Jul 23, 2016

emilio added S-needs-code-changes and removed S-awaiting-review labels Jul 25, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 26, 2016

craftytrickster force-pushed the craftytrickster:10933/shorthands branch from 87c9ffd to cd5ce86 Jul 28, 2016

emilio added S-needs-code-changes and removed S-awaiting-review labels Jul 28, 2016

emilio assigned emilio and unassigned asajeffrey Jul 28, 2016

craftytrickster force-pushed the craftytrickster:10933/shorthands branch from cd5ce86 to ff03405 Aug 1, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 1, 2016

emilio removed the S-awaiting-review label Aug 1, 2016

highfive removed the S-awaiting-merge label Aug 17, 2016

highfive added the S-tests-failed label Aug 17, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 17, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 17, 2016

Restoring expected failures for several .ini files

Loading status checks…

d8c042a

highfive added S-awaiting-review and removed S-tests-failed labels Aug 17, 2016

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

bors-servo merged commit d8c042a into servo:master Aug 17, 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

highfive removed the S-awaiting-merge label Aug 17, 2016

upsuper mentioned this pull request Feb 25, 2017

Background serialization of multiple values. #15733

Merged

3 of 4 tasks complete

servo / servo

10933/shorthands #12572

10933/shorthands #12572

craftytrickster commented Jul 23, 2016 •

edited

highfive commented Jul 23, 2016

highfive commented Jul 23, 2016

emilio commented Jul 25, 2016

craftytrickster commented Jul 26, 2016

emilio commented Jul 26, 2016

craftytrickster commented Jul 26, 2016

craftytrickster commented Jul 28, 2016

craftytrickster commented Jul 28, 2016

emilio commented Jul 28, 2016

emilio commented Jul 28, 2016

craftytrickster commented Jul 28, 2016

emilio commented Jul 28, 2016 •

edited

craftytrickster commented Jul 28, 2016

craftytrickster commented Aug 1, 2016

emilio commented Aug 1, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 17, 2016

highfive commented Aug 17, 2016

emilio commented Aug 17, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 17, 2016

highfive commented Aug 17, 2016

craftytrickster commented Aug 17, 2016

emilio commented Aug 17, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 17, 2016

craftytrickster commented Aug 17, 2016

servo / servo

Join GitHub today

10933/shorthands #12572

10933/shorthands #12572

Conversation

craftytrickster commented Jul 23, 2016 • edited

highfive commented Jul 23, 2016

highfive commented Jul 23, 2016

emilio commented Jul 25, 2016

craftytrickster commented Jul 26, 2016

emilio commented Jul 26, 2016

craftytrickster commented Jul 26, 2016

craftytrickster commented Jul 28, 2016

craftytrickster commented Jul 28, 2016

emilio commented Jul 28, 2016

emilio commented Jul 28, 2016

craftytrickster commented Jul 28, 2016

emilio commented Jul 28, 2016 • edited

craftytrickster commented Jul 28, 2016

craftytrickster commented Aug 1, 2016

emilio commented Aug 1, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 17, 2016

highfive commented Aug 17, 2016

emilio commented Aug 17, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 17, 2016

highfive commented Aug 17, 2016

craftytrickster commented Aug 17, 2016

emilio commented Aug 17, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 17, 2016

bors-servo commented Aug 17, 2016

craftytrickster commented Aug 17, 2016

craftytrickster commented Jul 23, 2016 •

edited

emilio commented Jul 28, 2016 •

edited