Bug 124621

Summary: [css shapes] layout for new ellipse syntax
Product: WebKit Reporter: Bem Jones-Bey <bjonesbe>
Component: Layout and RenderingAssignee: Rob Buis <rwlbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, esprehn+autocc, glenn, kondapallykalyan, rniwa, rwlbuis, syoichi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 124173    
Attachments:
Description Flags
Patch
none
Patch krit: review+, buildbot: commit-queue-

Description Bem Jones-Bey 2013-11-19 17:03:10 PST
Support layout for new ellipse syntax
Comment 1 Rob Buis 2013-12-03 07:37:22 PST
*** Bug 125079 has been marked as a duplicate of this bug. ***
Comment 2 Rob Buis 2013-12-03 07:56:03 PST
Created attachment 218295 [details]
Patch
Comment 3 Dirk Schulze 2013-12-03 08:16:35 PST
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 :)
Comment 4 Rob Buis 2013-12-03 08:56:08 PST
Created attachment 218298 [details]
Patch
Comment 5 Rob Buis 2013-12-03 08:57:48 PST
(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 6 Dirk Schulze 2013-12-03 09:06:25 PST
Comment on attachment 218298 [details]
Patch

r=me
Comment 7 Build Bot 2013-12-03 09:15:15 PST
Comment on attachment 218298 [details]
Patch

Attachment 218298 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/42818082
Comment 8 Build Bot 2013-12-03 09:25:23 PST
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
Comment 9 Bem Jones-Bey 2013-12-03 09:41:09 PST
(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. ;-)
Comment 10 Rob Buis 2013-12-03 10:18:56 PST
Landed in r160007 (and then I needed a build fix r160009, sorry!).