Bug 124621

*** Bug 125079 has been marked as a duplicate of this bug. ***

Created attachment 218295 [details] Patch

Comment on attachment 218295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=218295&action=review Some snippets. > Source/WebCore/ChangeLog:8 > + Implement support for doing layout with the new circle shape syntax, This patch is doing ellipse, not circle. > Source/WebCore/ChangeLog:18 > + (WebCore::BasicShapeEllipse::floatValueForRadiusInBox): helper function to calculate s/helper/Helper/ :) > Source/WebCore/rendering/style/BasicShapes.cpp:64 > + if (type() == BasicShape::BasicShapeEllipseType) { > + const BasicShapeEllipse* thisEllipse = static_cast<const BasicShapeEllipse*>(this); early return here: if (type() != BasicShape::BasicShapeEllipseType) return true; > Source/WebCore/rendering/style/BasicShapes.cpp:213 > + // If radius.type() == BasicShapeRadius::FarthestSide. Can you assert that? ASSERT(radius.type() == BasicShapeRadius::FarthestSide) instead of the comment > LayoutTests/ChangeLog:25 > + * fast/shapes/shape-outside-floats/shape-outside-floats-ellipse-000-expected.txt: Added. > + * fast/shapes/shape-outside-floats/shape-outside-floats-ellipse-000.html: Added. I mean it shows something. But could you still try to follow the other tests and make it a ref test please? > LayoutTests/animations/resources/animation-test-helpers.js:222 > - matches = s.match("ellipse\\((.*)\\s*,\\s*(.*)\\s*,\\s*(.*)\\,\\s*(.*)\\)"); > + matches = s.match("ellipse\\((.*)\\s+(.*)\\s+at\\s+(.*)\\s+(.*)\\)"); Something that bugs me on this and Bems patch (but didn't realize it until now): We do not test the old syntax anymore and don't know if this is still working. We should discuss if we want to add a deprecated mode to testing as well. > LayoutTests/css3/masking/clip-path-ellipse.html:16 > \ No newline at end of file argh. Not this line again :)

Created attachment 218298 [details] Patch

(In reply to comment #3) > (From update of attachment 218295 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=218295&action=review > > Some snippets. > > > Source/WebCore/ChangeLog:8 > > + Implement support for doing layout with the new circle shape syntax, > > This patch is doing ellipse, not circle. Oops! Fixed :) > > Source/WebCore/ChangeLog:18 > > + (WebCore::BasicShapeEllipse::floatValueForRadiusInBox): helper function to calculate > > s/helper/Helper/ :) Sure, fixed. > > Source/WebCore/rendering/style/BasicShapes.cpp:64 > > + if (type() == BasicShape::BasicShapeEllipseType) { > > + const BasicShapeEllipse* thisEllipse = static_cast<const BasicShapeEllipse*>(this); > > early return here: > > if (type() != BasicShape::BasicShapeEllipseType) > return true; Done. > > Source/WebCore/rendering/style/BasicShapes.cpp:213 > > + // If radius.type() == BasicShapeRadius::FarthestSide. > > Can you assert that? > ASSERT(radius.type() == BasicShapeRadius::FarthestSide) > > instead of the comment Fixed. > > LayoutTests/ChangeLog:25 > > + * fast/shapes/shape-outside-floats/shape-outside-floats-ellipse-000-expected.txt: Added. > > + * fast/shapes/shape-outside-floats/shape-outside-floats-ellipse-000.html: Added. > > I mean it shows something. But could you still try to follow the other tests and make it a ref test please? I didn't see they were converted, done. > > LayoutTests/animations/resources/animation-test-helpers.js:222 > > - matches = s.match("ellipse\\((.*)\\s*,\\s*(.*)\\s*,\\s*(.*)\\,\\s*(.*)\\)"); > > + matches = s.match("ellipse\\((.*)\\s+(.*)\\s+at\\s+(.*)\\s+(.*)\\)"); > > Something that bugs me on this and Bems patch (but didn't realize it until now): We do not test the old syntax anymore and don't know if this is still working. We should discuss if we want to add a deprecated mode to testing as well. I am not sure if it matters since it will be dropped (soon)? > > LayoutTests/css3/masking/clip-path-ellipse.html:16 > > \ No newline at end of file > > argh. Not this line again :) No idea, a vim thing, I guess :)

Comment on attachment 218298 [details] Patch r=me

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

Comment on attachment 218298 [details] Patch Attachment 218298 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/39458247

(In reply to comment #5) > (In reply to comment #3) > > (From update of attachment 218295 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=218295&action=review > > > LayoutTests/animations/resources/animation-test-helpers.js:222 > > > - matches = s.match("ellipse\\((.*)\\s*,\\s*(.*)\\s*,\\s*(.*)\\,\\s*(.*)\\)"); > > > + matches = s.match("ellipse\\((.*)\\s+(.*)\\s+at\\s+(.*)\\s+(.*)\\)"); > > > > Something that bugs me on this and Bems patch (but didn't realize it until now): We do not test the old syntax anymore and don't know if this is still working. We should discuss if we want to add a deprecated mode to testing as well. > > I am not sure if it matters since it will be dropped (soon)? Yeah, the amount of work needed to make it work for both didn't seem worth it to me, as we really hope to not support both for very long at all. The idea is to cut over as fast as we can and drop the old syntax. > > > LayoutTests/css3/masking/clip-path-ellipse.html:16 > > > \ No newline at end of file > > > > argh. Not this line again :) > > No idea, a vim thing, I guess :) It's a general vi thing. Vi requires that a file ends with a new line. Emacs doesn't. This is the subject of a great many flamewars. That's all I'm going to say on that. ;-)

Landed in r160007 (and then I needed a build fix r160009, sorry!).