Bug 124621 – [css shapes] layout for new ellipse syntax

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Bem Jones-Bey]

Support layout for new ellipse syntax

[Rob Buis]

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

[Rob Buis]

Created attachment 218295 [details]
Patch

[Dirk Schulze]

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 :)

[Rob Buis]

Created attachment 218298 [details]
Patch

[Rob Buis]

(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 :)

[Dirk Schulze]

Comment on attachment 218298 [details]
Patch

r=me

[Build Bot]

Comment on attachment 218298 [details]
Patch

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

[Build Bot]

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

[Bem Jones-Bey]

(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. ;-)

[Rob Buis]

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