Bug 129404

Created attachment 225321 [details] Initial Patch

Comment on attachment 225321 [details] Initial Patch Attachment 225321 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5820055455531008 New failing tests: svg/masking/mask-negative-scale.svg fast/masking/parsing-clip-path-shape.html

Created attachment 225330 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Comment on attachment 225321 [details] Initial Patch Attachment 225321 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5159757383991296 New failing tests: svg/masking/mask-negative-scale.svg fast/masking/parsing-clip-path-shape.html

Created attachment 225334 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 225321 [details] Initial Patch Attachment 225321 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5822013960617984 New failing tests: fast/masking/parsing-clip-path-shape.html

Created attachment 225337 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 225321 [details] Initial Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225321&action=review The code looks good to me. I think the tests is something astearns should look at. > Source/WebCore/css/CSSBasicShapes.cpp:126 > + } Would it be possible to combine this with the next if/else block? I am not sure if the side == CSSValueCenter needs to do the later if checks or not.

Created attachment 225407 [details] Updated patch Fixing up clip-path test and adding some extra position test cases

Created attachment 225483 [details] Updating Patch

Comment on attachment 225483 [details] Updating Patch View in context: https://bugs.webkit.org/attachment.cgi?id=225483&action=review Sorry for the delay. Have some comments to the test results that I would like to clarify before landing. > LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:76 > +PASS getCSSText("-webkit-shape-inside", "circle(closest-side)") is "circle(closest-side at 50% 50%)" This is not correct IMO. From the spec "omitting components when possible without changing the meaning" There is even an example that is very similar to this one. > LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:98 > +PASS getCSSText("-webkit-shape-inside", "circle(10px at right 0px bottom 0px)") is "circle(10px at 100% 100%)" That looks like the code is more intelligent than it should be. According to the spec this needs calc(right - 0px) and therefore must be written as right 0px. > LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:99 > +PASS getComputedStyleValue("-webkit-shape-inside", "circle(10px at right 0px bottom 0px)") is "circle(10px at 100% 100%)" Ditto. > LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt:92 > +PASS getCSSText("-webkit-shape-outside", "circle(10px at left top 10px)") is "circle(10px at 0% 10px)" This seems more intelligent than it should be as well. I read the spec that this should be serialized to "circle(10px at left 0% top 10px)" > LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt:94 > +PASS getCSSText("-webkit-shape-outside", "circle(10px at top 10px left 10px)") is "circle(10px at 10px 10px)" Ditto. > LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt:98 > +PASS getCSSText("-webkit-shape-outside", "circle(10px at right 0px bottom 0px)") is "circle(10px at 100% 100%)" Ditto. > LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt:99 > +PASS getComputedStyleValue("-webkit-shape-outside", "circle(10px at right 0px bottom 0px)") is "circle(10px at 100% 100%)" Ditto.

(In reply to comment #11) > (From update of attachment 225483 [details]) > > LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:76 > > +PASS getCSSText("-webkit-shape-inside", "circle(closest-side)") is "circle(closest-side at 50% 50%)" > > This is not correct IMO. From the spec "omitting components when possible without changing the meaning" There is even an example that is very similar to this one. The test is correct. <position> values always use the 2- or 4-value form, so you can't omit the default <position> > > LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:98 > > +PASS getCSSText("-webkit-shape-inside", "circle(10px at right 0px bottom 0px)") is "circle(10px at 100% 100%)" > > That looks like the code is more intelligent than it should be. According to the spec this needs calc(right - 0px) and therefore must be written as right 0px. Since right 0px can be written using a left offset without calc, the test is correct that it should serialize as 100%. And the same applies to the rest of your comments. All of the tests you mention are correct.

(In reply to comment #12) > (In reply to comment #11) > > (From update of attachment 225483 [details] [details]) > > > LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:76 > > > +PASS getCSSText("-webkit-shape-inside", "circle(closest-side)") is "circle(closest-side at 50% 50%)" > > > > This is not correct IMO. From the spec "omitting components when possible without changing the meaning" There is even an example that is very similar to this one. > > The test is correct. <position> values always use the 2- or 4-value form, so you can't omit the default <position> Whoops - I was only looking at <position>. You are correct that closest-side should be omitted.

(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > (From update of attachment 225483 [details] [details] [details]) > > > > LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:76 > > > > +PASS getCSSText("-webkit-shape-inside", "circle(closest-side)") is "circle(closest-side at 50% 50%)" > > > > > > This is not correct IMO. From the spec "omitting components when possible without changing the meaning" There is even an example that is very similar to this one. > > > > The test is correct. <position> values always use the 2- or 4-value form, so you can't omit the default <position> > > Whoops - I was only looking at <position>. You are correct that closest-side should be omitted. Yep, the closest-side should be omitted, but that will fall under a subsequent patch. This patch only covers the <position> argument to the circle function.

Comment on attachment 225483 [details] Updating Patch r=me then.

Comment on attachment 225483 [details] Updating Patch Clearing flags on attachment: 225483 Committed r164998: <http://trac.webkit.org/changeset/164998>

All reviewed patches have been landed. Closing bug.