Bug 124621 - [css shapes] layout for new ellipse syntax
Summary: [css shapes] layout for new ellipse syntax
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords:
: 125079 (view as bug list)
Depends on:
Blocks: 124173
  Show dependency treegraph
 
Reported: 2013-11-19 17:03 PST by Bem Jones-Bey
Modified: 2013-12-03 10:18 PST (History)
8 users (show)

See Also:


Attachments
Patch (27.73 KB, patch)
2013-12-03 07:56 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (25.95 KB, patch)
2013-12-03 08:56 PST, Rob Buis
krit: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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!).