Bug 124620

(In reply to comment #0) > Support parsing the new ellipse shape syntax Hi Bem, I am interested in taking this bug, I hope you don't mind? My patch is far along.

Created attachment 218124 [details] Patch

Comment on attachment 218124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218124&action=review Overall great patch. Still some requests. r- > Source/WebCore/css/BasicShapeFunctions.cpp:114 > + return 0; Is this even necessary? The compiler should be smart enough to detect that the switch needs to run though all types, no? > Source/WebCore/css/BasicShapeFunctions.cpp:250 > + if (radius) { could you make an early return instead? if (!radius) return BasicShapeRadius(BasicShapeRadius::ClosestSide); > Source/WebCore/css/BasicShapeFunctions.cpp:259 > + break; This would be a return as well, but still more readable IMO. > Source/WebCore/css/CSSBasicShapes.cpp:191 > + result.reserveCapacity((sizeof(opening) - 1) + (4 * (sizeof(separator) - 1)) + 1 + radiusX.length() + radiusY.length() + sizeof(at) + centerX.length() + centerY.length()); I don't think that this is particular useful. I had this discussion with Bear before IIRC. StringBuilder should already be quite fast. This complicates the code more. > Source/WebCore/css/CSSParser.cpp:5574 > + for (CSSParserValue* argument = args->current(); argument; argument = args->next()) { Didn't we use while loops in the past? I don't particularly care but while(argument) { argument = args->next(); } seems more readable. Guess it is a matter of taste. You can keep it. > Source/WebCore/css/CSSParser.cpp:5593 > + if (argument->id == CSSValueAt) { This could be an early return and a comment that we assume an at value now. // At this point we expect an 'at' if (argument->id != CSSValueAt) return 0; > Source/WebCore/css/CSSParser.cpp:5598 > + if (centerX && centerY) { This could be an early return: if (!centerX || !centerY) return 0; > Source/WebCore/rendering/shapes/Shape.cpp:173 > + // FIXME implement layout. bug 124619 Change // FIXME implement layout. bug 124619 to // FIXME: Layout implementation needed. See bug https://bugs.webkit.org/show_bug.cgi?id=124619 > Source/WebCore/rendering/shapes/Shape.cpp:174 > + shape = createRectangleShape(FloatRect(0, 0, boxWidth, boxHeight), FloatSize(0, 0)); s/FloatSize(0, 0)/FloatSize()/ (IIRC FloatSize defaults to 0,0) > Source/WebCore/rendering/style/BasicShapes.cpp:186 > + // FIXME Complete implementation of path. Bug 124619. // FIXME Complete implementation of path. Bug 124619. // FIXME: The implementation of path is incomplete. See https://bugs.webkit.org/show_bug.cgi?id=124619 Though I am not sure what that actually means. We do not support creating clip paths with this patch? So -webkit-clip-path: would support the new syntax, but the path is not created? If so, can you state that and create a new bug for that please? 124619 is not about paths. > Source/WebCore/rendering/style/BasicShapes.cpp:204 > + radiusY * 2 > + )); braces in the same line. > LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:-33 > -PASS getCSSText("-webkit-shape-inside", "circle(at 10px)") is "circle(at 10px 50%)" > -PASS getComputedStyleValue("-webkit-shape-inside", "circle(at 10px)") is "circle(closest-side at 10px 50%)" Why removing these passes? > LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt:-133 > -PASS getCSSText("-webkit-shape-inside", "ellipse()") is "" > -PASS getComputedStyleValue("-webkit-shape-inside", "ellipse()") is "auto" > -PASS getCSSText("-webkit-shape-inside", "ellipse(10px)") is "" > -PASS getComputedStyleValue("-webkit-shape-inside", "ellipse(10px)") is "auto" Ditto. > LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt:-33 > -PASS getCSSText("-webkit-shape-outside", "circle(at 10px)") is "circle(at 10px 50%)" > -PASS getComputedStyleValue("-webkit-shape-outside", "circle(at 10px)") is "circle(closest-side at 10px 50%)" Ditto. > LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt:-131 > -PASS getCSSText("-webkit-shape-outside", "ellipse()") is "" > -PASS getComputedStyleValue("-webkit-shape-outside", "ellipse()") is "auto" > -PASS getCSSText("-webkit-shape-outside", "ellipse(10px)") is "" > -PASS getComputedStyleValue("-webkit-shape-outside", "ellipse(10px)") is "auto" Ditto. > LayoutTests/fast/shapes/parsing/parsing-test-utils.js:-24 > - ["circle(at 10px)", "circle(at 10px 50%)", "circle(closest-side at 10px 50%)"], Ah, duplication. I take the question back. > LayoutTests/fast/shapes/parsing/parsing-test-utils.js:31 > + "ellipse(10px, 20px, 30px, 40px)", // FIXME remove with deprecated ellipse Sentences please. // FIXME: Remove this test once we do not support the deprecated CSS Shapes syntax anymore.

Comment on attachment 218124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218124&action=review >> Source/WebCore/css/BasicShapeFunctions.cpp:114 >> + return 0; > > Is this even necessary? The compiler should be smart enough to detect that the switch needs to run though all types, no? Here I would just like to do the same thing as before my patch, i.e. not change existing behavior for circle. >> Source/WebCore/css/BasicShapeFunctions.cpp:250 >> + if (radius) { > > could you make an early return instead? > > if (!radius) > return BasicShapeRadius(BasicShapeRadius::ClosestSide); Fixed. >> Source/WebCore/css/BasicShapeFunctions.cpp:259 >> + break; > > This would be a return as well, but still more readable IMO. Fixed. >> Source/WebCore/css/CSSBasicShapes.cpp:191 >> + result.reserveCapacity((sizeof(opening) - 1) + (4 * (sizeof(separator) - 1)) + 1 + radiusX.length() + radiusY.length() + sizeof(at) + centerX.length() + centerY.length()); > > I don't think that this is particular useful. I had this discussion with Bear before IIRC. StringBuilder should already be quite fast. This complicates the code more. Probably correct, removed. >> Source/WebCore/css/CSSParser.cpp:5574 >> + for (CSSParserValue* argument = args->current(); argument; argument = args->next()) { > > Didn't we use while loops in the past? I don't particularly care but > > while(argument) { > argument = args->next(); > } > seems more readable. Guess it is a matter of taste. You can keep it. Kept as is. >> Source/WebCore/css/CSSParser.cpp:5593 >> + if (argument->id == CSSValueAt) { > > This could be an early return and a comment that we assume an at value now. > > // At this point we expect an 'at' > if (argument->id != CSSValueAt) > return 0; Fixed. >> Source/WebCore/css/CSSParser.cpp:5598 >> + if (centerX && centerY) { > > This could be an early return: > > if (!centerX || !centerY) > return 0; Fixed. >> Source/WebCore/rendering/shapes/Shape.cpp:173 >> + // FIXME implement layout. bug 124619 > > Change > > // FIXME implement layout. bug 124619 > > to > > // FIXME: Layout implementation needed. See bug https://bugs.webkit.org/show_bug.cgi?id=124619 Fixed. >> Source/WebCore/rendering/shapes/Shape.cpp:174 >> + shape = createRectangleShape(FloatRect(0, 0, boxWidth, boxHeight), FloatSize(0, 0)); > > s/FloatSize(0, 0)/FloatSize()/ (IIRC FloatSize defaults to 0,0) Fixed. >> Source/WebCore/rendering/style/BasicShapes.cpp:186 >> + // FIXME Complete implementation of path. Bug 124619. > > // FIXME Complete implementation of path. Bug 124619. > > // FIXME: The implementation of path is incomplete. See https://bugs.webkit.org/show_bug.cgi?id=124619 > > Though I am not sure what that actually means. We do not support creating clip paths with this patch? So -webkit-clip-path: would support the new syntax, but the path is not created? If so, can you state that and create a new bug for that please? 124619 is not about paths. You are right, I made a new bug for this. >> Source/WebCore/rendering/style/BasicShapes.cpp:204 >> + )); > > braces in the same line. Fixed. >> LayoutTests/fast/shapes/parsing/parsing-test-utils.js:-24 >> - ["circle(at 10px)", "circle(at 10px 50%)", "circle(closest-side at 10px 50%)"], > > Ah, duplication. I take the question back. Ok :) >> LayoutTests/fast/shapes/parsing/parsing-test-utils.js:31 >> + "ellipse(10px, 20px, 30px, 40px)", // FIXME remove with deprecated ellipse > > Sentences please. > // FIXME: Remove this test once we do not support the deprecated CSS Shapes syntax anymore. Fixed, and also the other occurrences.

Created attachment 218185 [details] Patch

Comment on attachment 218185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218185&action=review Hey Rob, I just got back from vacation. Thanks for putting together this patch. I have a few comments myself: > Source/WebCore/css/BasicShapeFunctions.cpp:103 > +static PassRefPtr<CSSPrimitiveValue> BasicShapeRadiusToCSSValue(const RenderStyle* style, CSSValuePool& pool, const BasicShapeRadius& radius) This should start with a lower case letter: basicShapeRadiusToCSSValue > Source/WebCore/css/BasicShapeFunctions.cpp:248 > +static BasicShapeRadius CSSValueToBasicShapeRadius(const RenderStyle* style, const RenderStyle* rootStyle, PassRefPtr<CSSPrimitiveValue> radius) This should start with a lower case letter: cssValueToBasicShapeRadius.

(In reply to comment #4) > (From update of attachment 218124 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=218124&action=review > > >> Source/WebCore/css/BasicShapeFunctions.cpp:114 > >> + return 0; > > > > Is this even necessary? The compiler should be smart enough to detect that the switch needs to run though all types, no? > > Here I would just like to do the same thing as before my patch, i.e. not change existing behavior for circle. In this case, I'd agree with Dirk and argue for removing that return; I personally think it's better to have the compiler complain if the return is missing rather than have the code quietly do something that isn't correct. (If you do need to add the return 0 to silence the compilet, I'd put an ASSERT_UNREACHED()) before it.

Comment on attachment 218185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218185&action=review Missed a couple comments on the first look. > Source/WebCore/css/CSSBasicShapes.cpp:190 > + result.append(opening); You should use appendLiteral here. > Source/WebCore/css/CSSBasicShapes.cpp:198 > + result.append(separator); Ditto. > Source/WebCore/css/CSSBasicShapes.cpp:205 > + result.append(separator); Ditto. > Source/WebCore/css/CSSBasicShapes.cpp:206 > + result.append(at); Ditto. > Source/WebCore/css/CSSBasicShapes.cpp:207 > + result.append(separator); Ditto. > Source/WebCore/css/CSSBasicShapes.cpp:209 > + result.append(separator); Ditto. > Source/WebCore/css/CSSBasicShapes.cpp:212 > + result.append(")"); Ditto. > Source/WebCore/css/CSSBasicShapes.cpp:214 > + result.append(separator); Ditto.

Comment on attachment 218185 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218185&action=review Missed a couple comments on the first look. > Source/WebCore/css/CSSBasicShapes.cpp:190 > + result.append(opening); You should use appendLiteral here. > Source/WebCore/css/CSSBasicShapes.cpp:198 > + result.append(separator); Ditto. > Source/WebCore/css/CSSBasicShapes.cpp:205 > + result.append(separator); Ditto. > Source/WebCore/css/CSSBasicShapes.cpp:206 > + result.append(at); Ditto. > Source/WebCore/css/CSSBasicShapes.cpp:207 > + result.append(separator); Ditto. > Source/WebCore/css/CSSBasicShapes.cpp:209 > + result.append(separator); Ditto. > Source/WebCore/css/CSSBasicShapes.cpp:212 > + result.append(")"); Ditto. > Source/WebCore/css/CSSBasicShapes.cpp:214 > + result.append(separator); Ditto.

Created attachment 218195 [details] Patch

Comment on attachment 218195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218195&action=review lgtm, just a nit on a comment. > Source/WebCore/css/CSSParser.cpp:5569 > + // ellipse(radius at <position> Strictly speaking, this should be: // ellipse(radiusX radiusY) // ellipse(radiusX radiusY at <position>)

Comment on attachment 218195 [details] Patch Wit the blessing of Bem: r=me :) But please fix the nits that Bem pointed out.

(In reply to comment #12) > (From update of attachment 218195 [details]) > Wit the blessing of Bem: r=me :) But please fix the nits that Bem pointed out. I told him that I didn't mind if he fixed them in a future patch, so I'm happy if he lands this one as is.

Created attachment 218203 [details] Patch

Created attachment 218205 [details] Patch

Comment on attachment 218205 [details] Patch Clearing flags on attachment: 218205 Committed r159954: <http://trac.webkit.org/changeset/159954>

All reviewed patches have been landed. Closing bug.