Bug 124619

Created attachment 218213 [details] Patch

Comment on attachment 218213 [details] Patch Attachment 218213 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/42298013

Comment on attachment 218213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218213&action=review r=me but still with some nits. > Source/WebCore/css/CSSParser.cpp:5460 > + if (value->id == CSSValueClosestSide || value->id == CSSValueFarthestSide) Is there a test for the change? I wonder why previous tests didn't detect this. > Source/WebCore/rendering/style/BasicShapes.cpp:135 > + // if radius.type() == BasicShapeRadius::FarthestSide. s/if/If/ :) > Source/WebCore/rendering/style/BasicShapes.h:146 > + return m_keyword == None && other.keyword() == None; It would be great if we can investigate if it is possible to interpolate between some keywords. Can you add an FIXME with a link to a bug report please? > Source/WebCore/rendering/style/BasicShapes.h:179 > + return m_type == Value && other.type() == Value; Ditto. Can be the same bug. > LayoutTests/css3/masking/clip-path-circle-overflow-hidden.html:25 > \ No newline at end of file Hm, do we really want that in this file? > LayoutTests/css3/masking/clip-path-circle-overflow.html:17 > \ No newline at end of file Ditto. > LayoutTests/css3/masking/clip-path-circle-relative-overflow.html:17 > \ No newline at end of file Ditto. > LayoutTests/css3/masking/clip-path-circle.html:16 > \ No newline at end of file Ditto. > LayoutTests/css3/masking/clip-path-restore.html:32 > \ No newline at end of file Ditto.

Comment on attachment 218213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218213&action=review >> Source/WebCore/css/CSSParser.cpp:5460 >> + if (value->id == CSSValueClosestSide || value->id == CSSValueFarthestSide) > > Is there a test for the change? I wonder why previous tests didn't detect this. Because I failed to add a test for this when I wrote the parsing patch. I added a test to the parsing tests for this in this patch. >> Source/WebCore/rendering/style/BasicShapes.cpp:135 >> + // if radius.type() == BasicShapeRadius::FarthestSide. > > s/if/If/ :) Heh. Applied. >> Source/WebCore/rendering/style/BasicShapes.h:146 >> + return m_keyword == None && other.keyword() == None; > > It would be great if we can investigate if it is possible to interpolate between some keywords. Can you add an FIXME with a link to a bug report please? Ok, will do. >> LayoutTests/css3/masking/clip-path-circle-overflow-hidden.html:25 >> \ No newline at end of file > > Hm, do we really want that in this file? That's not in the file, it was just mentioning that a newline was added at the end of the file when I made my change, because I edited it using Vim , and if there isn't a newline at the end of a file it edits, it adds one. (Or are you saying that it's bad that a newline got added?)

Created attachment 218220 [details] Updated Patch

Comment on attachment 218220 [details] Updated Patch Clearing flags on attachment: 218220 Committed r159979: <http://trac.webkit.org/changeset/159979>

All reviewed patches have been landed. Closing bug.