box-shadow is now serialized properly #15703

niki-04 · 2017-02-23T06:08:00Z

Box shadow in now serialized properly. The changes are in the file components/style/properties/longhand/effects.mako.rs. Tests for the serialization of box-shadow are in tests/unit/style/properties/serialization.rs. ./mach test-unit -p style passes all tests including the newly added test case.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
[X ] These changes fix Value of box-shadow is serialized in a wrong order #15203 (github issue number if applicable).

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

This change is

highfive · 2017-02-23T06:08:06Z

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 · 2017-02-23T06:08:07Z

Heads up! This PR modifies the following files:

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

emilio

Thanks for this PR! The serialization changes look great to me, though I believe the test can be shorter.

Also, could you squash your commits? We don't use to land commits like "Fixes tidy" or so.

With that test change, this can land with r=emilio.

Thanks again for this patch! :)

emilio · 2017-02-23T16:03:21Z

tests/unit/style/properties/serialization.rs

+        #[test]
+        fn box_shadow_should_serialize_correctly() {
+            let mut properties = Vec::new();
+            let color = Some(CSSColor {


Let's move the property_declaration_block function to the parent module, and write this test as follows, which will be easier to maintain:

let shadow_css = "box-shadow: 1px 2px 3px 4px"; let shadow = property_declaration_block(shadow_css); assert_eq!(shadow.to_css_string(), shadow_css);

Thanks for the review. So how to use the property_declaration_block function how do I define it in the parent module?

You can just move it to the top of the file making it pub (above the first mod), and use it from the others.

emilio · 2017-02-26T12:41:45Z

tests/unit/style/properties/serialization.rs

@@ -1231,4 +1231,26 @@ mod shorthand_serialization {
            assert_eq!(serialization, block_text);
        }
    }
+      mod effects {


Also, one nit, alignment seems to be off-by-one here.

I did make the changes. Also committed and squashed the commits.

I think something went wrong and it created merge master branch. I don't know how to undo that but last commit to be considered is in box-shadow is now serialized properly. Let me know if I need to do anything to fix it. Sorry if this messes something!!

bors-servo · 2017-03-08T11:52:59Z

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

KiChjang · 2017-03-08T18:16:45Z

Please remove the merge commit and rebase against master instead.

niki-04 · 2017-03-09T21:38:42Z

I removed the merge commit. How do I rebase against the master instead.

KiChjang · 2017-03-09T21:47:57Z

First, git fetch upstream master, and then git rebase upstream/master. You will encounter merge conflicts along the way, I suggest using a mergetool like meld, and you can configure your git client to open it up to solve merge conflicts through git mergetool.

wafflespeanut · 2017-03-21T10:17:17Z

@niki-04 Are you planning to finish this? Need help?

niki-04 · 2017-03-21T14:14:44Z

@wafflespeanut sorry about the delay. I will finish it by today. Will let you know if i need help

niki-04 · 2017-03-21T22:53:32Z

I was going through the changes I see property_declaration_block to parse_declaration_block. Should I make the relevant changes in my code. I was having trouble merging the code for the same

niki-04 · 2017-03-22T17:53:35Z

I resolved the conflicts and committed the code again. Let me know if I need to squash my commits. I did all the checks again everything passes :)

wafflespeanut · 2017-03-22T18:00:30Z

No, you haven't? It looks like you haven't rebased it properly.

niki-04 · 2017-03-22T18:09:14Z

When I rebased it showed me the conflicts and I resolved those and committed again. What should I do now to resolve the issue?

wafflespeanut · 2017-03-23T05:11:59Z

Export the diff from your current branch. Since you need only your last 4 commits squashed, you can do something like git diff HEAD~4.. > patch.diff
git checkout master, sync with upstream and delete your old branch with git branch -D box-shadow-serialization
git checkout -b box-shadow-serialization and git apply patch.diff (resolve conflicts, if any, and also remove patch.diff)
Finally, git add -A && git commit -m "<commit-message>" and push it.

niki-04 · 2017-03-23T13:59:53Z

@wafflespeanut thanks for the information. I am trying that but when I apply my patch it fails with the error patch does not apply. I tried looking for the same error but couldn't find any relevant information.

wafflespeanut · 2017-03-23T14:06:41Z

It may probably have conflicts. Check whether the files have conflicts. If they don't have any, then what does it say? Can you paste the log output?

niki-04 · 2017-03-23T14:45:35Z

No conflicts. The output is

Checking patch components/style/properties/longhand/effects.mako.rs...
error: while searching for:
            if self.inset {
                try!(dest.write_str("inset "));
            }
            try!(self.blur_radius.to_css(dest));
            try!(dest.write_str(" "));
            try!(self.spread_radius.to_css(dest));
            try!(dest.write_str(" "));
            try!(self.offset_x.to_css(dest));
            try!(dest.write_str(" "));
            try!(self.offset_y.to_css(dest));

            if let Some(ref color) = self.color {
                try!(dest.write_str(" "));

error: patch failed: components/style/properties/longhand/effects.mako.rs:29
error: components/style/properties/longhand/effects.mako.rs: patch does not apply
Checking patch tests/unit/style/properties/serialization.rs...
error: while searching for:
            assert_eq!(serialization, block_text);
        }
    }

    mod keywords {
        pub use super::*;
        #[test]

error: patch failed: tests/unit/style/properties/serialization.rs:1130
error: tests/unit/style/properties/serialization.rs: patch does not apply

wafflespeanut · 2017-03-23T15:27:58Z

Oh, okay. What if you try git am -3 patch.diff? From what I see in the manual, git should now try to merge the changes, and will result in the usual file conflicts.

niki-04 · 2017-03-23T15:48:03Z

I get a Patch format detection failed.

wafflespeanut · 2017-03-23T16:00:57Z

Oh wait, it should be done with apply (not am, which is only for exported patches using format-patch). Sorry about that. Try git apply -3 --whitespace=fix?

niki-04 · 2017-03-23T16:20:22Z

git apply -3 --whitespace=fix doesn't work. It just hangs up and does nothing

wafflespeanut · 2017-03-23T16:22:46Z

Oh, you need to point it to the patch like you did previously. git apply -3 --whitespace=fix patch.diff

niki-04 · 2017-03-23T16:49:39Z

Oh my bad. It still doesn't work
error: components/style/properties/longhand/effects.mako.rs: does not match index
error: tests/unit/style/properties/serialization.rs: does not match index

wafflespeanut · 2017-03-23T17:20:20Z

Strange. I just pulled from your branch, exported the patch and applied it, and it seems to work. I'm unable to reproduce the error. Can you check the "Allow edits from maintainers" check box to the right? (so that I'll be able to push to your branch?)

niki-04 · 2017-03-23T17:36:44Z

I did the check allow edits from maintainers. Thanks a lot for helping out. It is strange you don't get the error.

bors-servo · 2017-03-23T17:38:48Z

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

wafflespeanut · 2017-03-23T17:49:25Z

@niki-04 Apologies. I'd accidentally pushed "nothingness" into your PR, and it's now closed. Anyway, I've opened this in #16104. Thanks for working on this :)

…lespeanut Properly serialize <box-shadow> Rebase of #15703, fixes #15203  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16104)

niki-04 · 2017-03-23T19:02:30Z

No worries. I enjoyed working on this!!

highfive assigned Ms2ger Feb 23, 2017

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

niki-04 mentioned this pull request Feb 23, 2017

Fixes box-shadow serialization order #15664

Closed

2 tasks

emilio reviewed Feb 23, 2017

View reviewed changes

jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 23, 2017

jdm assigned emilio and unassigned Ms2ger Feb 23, 2017

emilio reviewed Feb 26, 2017

View reviewed changes

niki-04 force-pushed the box-shadow-serialization branch from ce78317 to 8801300 Compare March 5, 2017 19:58

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 5, 2017

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

niki-04 force-pushed the box-shadow-serialization branch from 48fa42e to 8801300 Compare March 9, 2017 21:36

wafflespeanut closed this Mar 23, 2017

wafflespeanut force-pushed the box-shadow-serialization branch from a3b7bf7 to d4d8293 Compare March 23, 2017 17:38

wafflespeanut mentioned this pull request Mar 23, 2017

Properly serialize <box-shadow> #16104

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

box-shadow is now serialized properly #15703

box-shadow is now serialized properly #15703

niki-04 commented Feb 23, 2017 •

edited by jdm

highfive commented Feb 23, 2017

highfive commented Feb 23, 2017

emilio left a comment

emilio Feb 23, 2017

niki-04 Feb 26, 2017

emilio Feb 26, 2017

emilio Feb 26, 2017

niki-04 Mar 5, 2017

niki-04 Mar 5, 2017

bors-servo commented Mar 8, 2017

KiChjang commented Mar 8, 2017

niki-04 commented Mar 9, 2017

KiChjang commented Mar 9, 2017

wafflespeanut commented Mar 21, 2017 •

edited

niki-04 commented Mar 21, 2017

niki-04 commented Mar 21, 2017 •

edited

niki-04 commented Mar 22, 2017

wafflespeanut commented Mar 22, 2017

niki-04 commented Mar 22, 2017 •

edited

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017 •

edited by jdm

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017

bors-servo commented Mar 23, 2017

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017

box-shadow is now serialized properly #15703

box-shadow is now serialized properly #15703

Conversation

niki-04 commented Feb 23, 2017 • edited by jdm

highfive commented Feb 23, 2017

highfive commented Feb 23, 2017

emilio left a comment

Choose a reason for hiding this comment

emilio Feb 23, 2017

Choose a reason for hiding this comment

niki-04 Feb 26, 2017

Choose a reason for hiding this comment

emilio Feb 26, 2017

Choose a reason for hiding this comment

emilio Feb 26, 2017

Choose a reason for hiding this comment

niki-04 Mar 5, 2017

Choose a reason for hiding this comment

niki-04 Mar 5, 2017

Choose a reason for hiding this comment

bors-servo commented Mar 8, 2017

KiChjang commented Mar 8, 2017

niki-04 commented Mar 9, 2017

KiChjang commented Mar 9, 2017

wafflespeanut commented Mar 21, 2017 • edited

niki-04 commented Mar 21, 2017

niki-04 commented Mar 21, 2017 • edited

niki-04 commented Mar 22, 2017

wafflespeanut commented Mar 22, 2017

niki-04 commented Mar 22, 2017 • edited

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017 • edited by jdm

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017

bors-servo commented Mar 23, 2017

wafflespeanut commented Mar 23, 2017

niki-04 commented Mar 23, 2017

niki-04 commented Feb 23, 2017 •

edited by jdm

wafflespeanut commented Mar 21, 2017 •

edited

niki-04 commented Mar 21, 2017 •

edited

niki-04 commented Mar 22, 2017 •

edited

niki-04 commented Mar 23, 2017 •

edited by jdm