Bug 96156 – [CSS Exclusions] ExclusionShape API should use logical coordinates for input/output

• Component: CSS
• Status: RESOLVED FIXED

[Hans Muller]

The ExclusionShape API should use logical coordinates for the incoming top/bottom line edges and similarly it should report line segments' start/stops with logical X coordinates. The incoming BasicShape, which is just an encoding of the CSS shape, is defined with physical coordinates.  That means that createExclusionShape() method will need an additional parameter, so that it can transform the BasicShape's physical coordinates when the writing-mode is vertical.  It also means that an ExclusionShape will be specific to a writing-mode, so in a shape-outside case like this: https://bug-95625-attachments.webkit.org/attachment.cgi?id=162542, two ExclusionShape objects will be used to compute the layouts. 

Internally, the ExclusionShapes classes (just ExclusionRectangle for the moment) do their computations as if the writing-mode was horizontal.  I don't think this needs to change, although the ExclusionInterval class, which is used to report line segment endpoints, should use logical names for the X coordinates, start and end instead of x1 and x2.  Here's what the ExclusionShape API would become:

  // A representation of a BasicShape that enables layout code to determine how to break a line up into segments
  // that will fit within or around a shape. The line is defined by a pair of logical Y coordinates and the 
  // computed segments are returned as pairs of logical X coordinates (ExclusionIntervals).  

  class ExclusionShape {
  public:
      static PassOwnPtr<ExclusionShape> createExclusionShape(const BasicShape*, float borderBoxLogicalWidth, float borderBoxLogicalHeight, WritingMode);

      virtual ~ExclusionShape() { }

      virtual LayouRect shapeLogicalBoundingBox() const = 0;
      virtual void getInsideIntervals(float logicalTop, float logicalBottom, Vector<ExclusionInterval>&) const = 0;
      virtual void getOutsideIntervals(float logicalTop, float logicalBottom, Vector<ExclusionInterval>&) const = 0;
  };

  } // namespace WebCore


The shapeLogicalBoundingBox() would return the logical bounds of the exclusion shape.  I've changed the return type from FloatRect to LayoutRect.

[Hans Muller]

Created attachment 164180 [details]
Diagram: conversion to ExclusionShape internal coordinates, vertical-lr writing-mode

[Hans Muller]

Created attachment 164182 [details]
Diagram: conversion to ExclusionShape internal coordinates, vertical-rl writing-mode

[Hans Muller]

Internally, ExclusionShapes are oriented so that lines run horizontally.  BasicShapes define shape-inside or shape-outside shapes in physcial coordinates, so no transformation is needed when the corresponding element's writing-mode is horizontal.  For the two vertical writing-modes, the BasicShape and the logical top,bottom line edge parameters must be transformed.  The transformation is trivial and is illustrated in the two attached diagrams.

[Hans Muller]

Created attachment 164185 [details]
Patch

[Bear Travis]

Comment on attachment 164185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164185&action=review

Looking good. Two high-level questions: Is it worth turning on vertical layout in wrap shapes, so we can write some simple test cases? And would it be worth it to convert to using completely logical coordinates internally? Right now, it seems like they are logical but not flipped, in the case of right to left vertical layout.

> Source/WebCore/ChangeLog:12
> +        system when the writing-mode is vertical. Similarly, the getInclude,ExcludedIntervals()

Is it worth describing the internal coordinate system in the changelog? It seems to be a non-flipped logical coordinate system.

> Source/WebCore/rendering/ExclusionShape.h:41
> +struct LineSegment {

There seems to be a little bit of a naming disconnect between RenderBlock's "LineSegment" and ExclusionShape's "ExclusionInterval". With methods the getIncluded/ExcludedIntervals, it seems like it would make more sense to keep ExclusionInterval here. On the RenderBlock side, LineSegment will eventually have to contain more information than just the start and end coordinates, so it may make sense to keep them separate.

[Bear Travis]

Comment on attachment 164185 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164185&action=review

> Source/WebCore/rendering/WrapShapeInfo.cpp:123
> +

Extra newline

[Hans Muller]

Created attachment 164426 [details]
Patch

Correct how ExclusionShapes deal with logical coordinates and enable shape-inside exclusion layout for vertical writing-modes.

[Hans Muller]

Created attachment 164598 [details]
Patch

[Hans Muller]

(In reply to comment #8)
> Created an attachment (id=164598) [details]
Updated the patch to reflect the fix for https://bugs.webkit.org/show_bug.cgi?id=93547, which just landed this morning.

[Dirk Schulze]

Comment on attachment 164598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164598&action=review

> LayoutTests/fast/exclusions/shape-inside/shape-inside-vertical-text-expected.html:2
> +<!DOCTYPE html>
> +<html>

The ChangeLog for the layout tests is missing.

> Source/WebCore/ChangeLog:809
> [phone]  Hans Muller  <[e-mail]>
> +
> +        ExclusionShape API should use logical coordinates for input/output
> +        https://bugs.webkit.org/show_bug.cgi?id=96156
> +

oops. Merge failure?

> Source/WebCore/ChangeLog:846
> +        (WebCore::LineSegment::LineSegment): relocated (for now) from WrapShapeInfo.h

for now? Where does it belong to?

> Source/WebCore/ChangeLog:2115
> [phone]  Hans Muller  <[e-mail]>
> +
> +        ExclusionShape API should use logical coordinates for input/output
> +        https://bugs.webkit.org/show_bug.cgi?id=96156

Ditto?

> Source/WebCore/rendering/ExclusionRectangle.cpp:43
> +void ExclusionRectangle::getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList& result) const

s/logicalTop/localTop/ As far as I understood your changelog, these values are in relative coordinates. In this case we use local (for local coordinate space).

Ditto for all other names.

> Source/WebCore/rendering/ExclusionRectangle.cpp:46
> +    float y1 = minYForLogicalLine(logicalTop, logicalBottom);
> +    float y2 = maxYForLogicalLine(logicalTop, logicalBottom);

Why is this not needed for x1 and x2?

> Source/WebCore/rendering/ExclusionRectangle.h:54
> +    virtual void getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const OVERRIDE;
> +    virtual void getIncludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const OVERRIDE;

Ditto. s/logical/local/

> Source/WebCore/rendering/ExclusionShape.cpp:62
> +// If the writingMode is vertical, then the BasicShape's (physical) x and y coordinates are swapped, so that

Do you have another name for physical? Maybe absolute? Sounds better. absolute is also used on other places for this context.

> Source/WebCore/rendering/ExclusionShape.cpp:67
> -    if (!wrapShape)
> +    if (!basicShape)

Will this just be used for BasicShape? Will it be used for SVG shapes as well (if we get them in the future)? If so, I would rename them to shape, or exclusionShape.

> Source/WebCore/rendering/ExclusionShape.cpp:73
> +    OwnPtr<ExclusionShape> exclusionShape;

so exclusionShape is already used :)

> Source/WebCore/rendering/ExclusionShape.cpp:82
> +        float x = floatValueForLength(rectangle->x(), boxWidth);
> +        float y = floatValueForLength(rectangle->y(), boxHeight);
> +        float width = floatValueForLength(rectangle->width(), boxWidth);
> +        float height = floatValueForLength(rectangle->height(), boxHeight);

Can you combine this to a FloatRect?

> Source/WebCore/rendering/ExclusionShape.cpp:86
> +        float radiusX = radiusXLength.isUndefined() ? 0 : floatValueForLength(radiusXLength, boxWidth);
> +        float radiusY = radiusYLength.isUndefined() ? 0 : floatValueForLength(radiusYLength, boxHeight);

FloatSize?

> Source/WebCore/rendering/ExclusionShape.cpp:90
> +          ? createExclusionRectangle(x, y, width, height, radiusX, radiusY)
> +          : createExclusionRectangle(y, x, height, width, radiusY, radiusX);

Would be great if it can take a FloatRect(), FloatSize(). This is how we usually handle these cases in WebKit. Only exclusions are JS API's.

> Source/WebCore/rendering/ExclusionShape.cpp:97
> +        float centerX = floatValueForLength(circle->centerX(), boxWidth);
> +        float centerY = floatValueForLength(circle->centerY(), boxHeight);

FloatPoint

> Source/WebCore/rendering/ExclusionShape.cpp:102
> +          ? createExclusionCircle(centerX, centerY, radius)
> +          : createExclusionCircle(centerY, centerX, radius);

createExclusionCircle(FloatPoint, float)?

> Source/WebCore/rendering/ExclusionShape.cpp:109
> +        float centerX = floatValueForLength(ellipse->centerX(), boxWidth);
> +        float centerY = floatValueForLength(ellipse->centerY(), boxHeight);

FloatPoint

> Source/WebCore/rendering/ExclusionShape.cpp:111
> +        float radiusX = floatValueForLength(ellipse->radiusX(), boxWidth);
> +        float radiusY = floatValueForLength(ellipse->radiusY(), boxHeight);

FloatSize

> Source/WebCore/rendering/ExclusionShape.cpp:115
> +          ? createExclusionEllipse(centerX, centerY, radiusX, radiusY)
> +          : createExclusionEllipse(centerY, centerX, radiusY, radiusX);

createExclusionEllipse(FloatPoint, FloatSize)

> Source/WebCore/rendering/ExclusionShape.cpp:127
> +    exclusionShape->m_logicalBoxWidth = logicalBoxWidth;
> +    exclusionShape->m_logicalBoxHeight = logicalBoxHeight;

exclusionShape->localSize(FloatSize())?

> Source/WebCore/rendering/ExclusionShape.h:49
> +    float logicalLeft;
> +    float logicalRight;
> +
> +    LineSegment(float logicalLeft, float logicalRight)
> +        : logicalLeft(logicalLeft)
> +        , logicalRight(logicalRight)
> +    {
> +    }

Shouldn't a LineSegment have two points? Are these lines just horizontal?

> Source/WebCore/rendering/ExclusionShape.h:62
> +    static PassOwnPtr<ExclusionShape> createExclusionShape(const BasicShape*, float logicalBoxWidth, float logicalBoxHeight, WritingMode);

FloatSize()

> Source/WebCore/rendering/ExclusionShape.h:68
> +    virtual void getIncludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const = 0;
> +    virtual void getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const = 0;

FloatPoint

> Source/WebCore/rendering/ExclusionShape.h:78
> +    float m_logicalBoxWidth;
> +    float m_logicalBoxHeight;

It would look better if you use FloatSize here instead.

[Hans Muller]

(In reply to comment #10)
> (From update of attachment 164598 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164598&action=review
> 
> > LayoutTests/fast/exclusions/shape-inside/shape-inside-vertical-text-expected.html:2
> > +<!DOCTYPE html>
> > +<html>
> 
> The ChangeLog for the layout tests is missing.

Yes it is, I'll fix that.

> > Source/WebCore/ChangeLog:809
> > [phone]  Hans Muller  <[e-mail]>
> > +
> > +        ExclusionShape API should use logical coordinates for input/output
> > +        https://bugs.webkit.org/show_bug.cgi?id=96156
> > +
> 
> oops. Merge failure?

Yes!  Will fix that.

> 
> > Source/WebCore/ChangeLog:846
> > +        (WebCore::LineSegment::LineSegment): relocated (for now) from WrapShapeInfo.h
> 
> for now? Where does it belong to?

I'll leave the parenthetical statement out of the ChangeLog.  In the future, if additional layout-specific data is added to the LineSegment type, then we may relocate it.


> > Source/WebCore/ChangeLog:2115
> > [phone]  Hans Muller  <[e-mail]>
> > +
> > +        ExclusionShape API should use logical coordinates for input/output
> > +        https://bugs.webkit.org/show_bug.cgi?id=96156
> 
> Ditto?

Yes. I'm going to reset the ChangeLog and then re-insert my changes at the top.  Note sure why the usual auto-merge ChangeLog stuff wasn't working for me.

> > Source/WebCore/rendering/ExclusionRectangle.cpp:43
> > +void ExclusionRectangle::getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList& result) const
> 
> s/logicalTop/localTop/ As far as I understood your changelog, these values are in relative coordinates. In this case we use local (for local coordinate space).
> ... 
> Ditto for all other names.

The patch uses the term "logical coordinates" in the same way that the layout code does: for values that correspond to physical X or Y coordinates, depending on the writing-mode.  The origin also moves, depending on the writing-mode.  When the writing-mode is "vertical-rl" the origin moves to  the top right of the content box's physical location.  You can see the implications of this in the two diagrams I attached to the bug.  They show how logical coordinates are converted to the ExclusionShape "internal" coordinates.  These "internal" coordinates simplify the implementations of the ExclusionShape getIncludedIntervals() and getExcludedIntervals() methods (particularly for the polygon case, which hasn't been included yet) as well the algorithm described here for finding the 'first fit' line segment location - http://hansmuller-webkit.blogspot.com/2012/08/revised-algorithm-for-finding-first.html.  Lines are always horizontal in internal coordinates: they're defined by two increasing Y values.  Similarly internal line segments are always defined by a pair of increasing X values.

The term "physical coordinates" is the one used by the layout system to refer to the usual graphics coordinate system.  BasicShapes are defined in physical coordinates.

> > Source/WebCore/rendering/ExclusionRectangle.cpp:46
> > +    float y1 = minYForLogicalLine(logicalTop, logicalBottom);
> > +    float y2 = maxYForLogicalLine(logicalTop, logicalBottom);
> 
> Why is this not needed for x1 and x2?

Because none of the writing-modes affect our internal X axis.

> > Source/WebCore/rendering/ExclusionRectangle.h:54
> > +    virtual void getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const OVERRIDE;
> > +    virtual void getIncludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const OVERRIDE;
> 
> Ditto. s/logical/local/

See above.

> > Source/WebCore/rendering/ExclusionShape.cpp:62
> > +// If the writingMode is vertical, then the BasicShape's (physical) x and y coordinates are swapped, so that
> 
> Do you have another name for physical? Maybe absolute? Sounds better. absolute is also used on other places for this context.

The phsycial/logical distinction is mentioned here: http://trac.webkit.org/wiki/April%202012%20Write%20Your%20Own%20Render%20Object 

It was the principal reason for creating this patch, see https://bugs.webkit.org/show_bug.cgi?id=95625#c5.

> > Source/WebCore/rendering/ExclusionShape.cpp:67
> > -    if (!wrapShape)
> > +    if (!basicShape)
> 
> Will this just be used for BasicShape? Will it be used for SVG shapes as well (if we get them in the future)? If so, I would rename them to shape, or exclusionShape.

All of the exclusion code is likely to evolve a great deal over the next couple of months.  I'd be happy to substitute a better name for basicShape when the scope of the implementation warrants it.

> > Source/WebCore/rendering/ExclusionShape.cpp:82
> > +        float x = floatValueForLength(rectangle->x(), boxWidth);
> > +        float y = floatValueForLength(rectangle->y(), boxHeight);
> > +        float width = floatValueForLength(rectangle->width(), boxWidth);
> > +        float height = floatValueForLength(rectangle->height(), boxHeight);
> 
> Can you combine this to a FloatRect?

> > Source/WebCore/rendering/ExclusionShape.cpp:86
> > +        float radiusX = radiusXLength.isUndefined() ? 0 : floatValueForLength(radiusXLength, boxWidth);
> > +        float radiusY = radiusYLength.isUndefined() ? 0 : floatValueForLength(radiusYLength, boxHeight);
> 
> FloatSize?

> > Source/WebCore/rendering/ExclusionShape.cpp:90
> > +          ? createExclusionRectangle(x, y, width, height, radiusX, radiusY)
> > +          : createExclusionRectangle(y, x, height, width, radiusY, radiusX);
> 
> Would be great if it can take a FloatRect(), FloatSize(). This is how we usually handle these cases in WebKit. Only exclusions are JS API's.

I don't see the advantage of packing and unpacking these values for a purely internal function call.  Why is this:

...
{ 
    float x = ... ;
    float y = ... ;
    float width = ... ;
    float height = ... ;
    localFunction(FloatRect(x, y, width height))
}

localFunction(FloatRect rect) {
    anotherFunction(rect.x, rect.y, rect.width, rect.height);
}


better than this:

...
{ 

    float x = ... ;
    float y = ... ;
    float width = ... ;
    float height = ... ;
    localFunction(x, y, width, height);
}

localFunction(float x, float y, float width, float height)
    anotherFunction(x, y, width, height);
}

> > Source/WebCore/rendering/ExclusionShape.h:49
> > +    float logicalLeft;
> > +    float logicalRight;
> > +
> > +    LineSegment(float logicalLeft, float logicalRight)
> > +        : logicalLeft(logicalLeft)
> > +        , logicalRight(logicalRight)
> > +    {
> > +    }
> 
> Shouldn't a LineSegment have two points? Are these lines just horizontal?

Yes, LineSegments are horizontal lines.

> > Source/WebCore/rendering/ExclusionShape.h:68
> > +    virtual void getIncludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const = 0;
> > +    virtual void getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const = 0;
> 
> FloatPoint

The names of these arguments reflect the structure of the caller, which also keeps the parameters separate, as does the line layout code.  The names were specified in the review for https://bugs.webkit.org/show_bug.cgi?id=95625.

[Dirk Schulze]

(In reply to comment #11)
> > s/logicalTop/localTop/ As far as I understood your changelog, these values are in relative coordinates. In this case we use local (for local coordinate space).
> > ... 
> > Ditto for all other names.
> 
> The patch uses the term "logical coordinates" in the same way that the layout code does: for values that correspond to physical X or Y coordinates, depending on the writing-mode.  The origin also moves, depending on the writing-mode.  When the writing-mode is "vertical-rl" the origin moves to  the top right of the content box's physical location.  You can see the implications of this in the two diagrams I attached to the bug.  They show how logical coordinates are converted to the ExclusionShape "internal" coordinates.  These "internal" coordinates simplify the implementations of the ExclusionShape getIncludedIntervals() and getExcludedIntervals() methods (particularly for the polygon case, which hasn't been included yet) as well the algorithm described here for finding the 'first fit' line segment location - http://hansmuller-webkit.blogspot.com/2012/08/revised-algorithm-for-finding-first.html.  Lines are always horizontal in internal coordinates: they're defined by two increasing Y values.  Similarly internal line segments are always defined by a pair of increasing X values.
> 
> The term "physical coordinates" is the one used by the layout system to refer to the usual graphics coordinate system.  BasicShapes are defined in physical coordinates.
...
> The phsycial/logical distinction is mentioned here: http://trac.webkit.org/wiki/April%202012%20Write%20Your%20Own%20Render%20Object 

Sounds reasonable. Didn't know of the usage in layout code.

> 
> > > Source/WebCore/rendering/ExclusionRectangle.cpp:46
> > > +    float y1 = minYForLogicalLine(logicalTop, logicalBottom);
> > > +    float y2 = maxYForLogicalLine(logicalTop, logicalBottom);
> > 
> > Why is this not needed for x1 and x2?
> 
> Because none of the writing-modes affect our internal X axis.
Ok.

> > > Source/WebCore/rendering/ExclusionShape.cpp:67
> > > -    if (!wrapShape)
> > > +    if (!basicShape)
> > 
> > Will this just be used for BasicShape? Will it be used for SVG shapes as well (if we get them in the future)? If so, I would rename them to shape, or exclusionShape.
K.

> 
> All of the exclusion code is likely to evolve a great deal over the next couple of months.  I'd be happy to substitute a better name for basicShape when the scope of the implementation warrants it.
> 
> > > Source/WebCore/rendering/ExclusionShape.cpp:82
> > > +        float x = floatValueForLength(rectangle->x(), boxWidth);
> > > +        float y = floatValueForLength(rectangle->y(), boxHeight);
> > > +        float width = floatValueForLength(rectangle->width(), boxWidth);
> > > +        float height = floatValueForLength(rectangle->height(), boxHeight);
> > 
> > Can you combine this to a FloatRect?
> 
> > > Source/WebCore/rendering/ExclusionShape.cpp:86
> > > +        float radiusX = radiusXLength.isUndefined() ? 0 : floatValueForLength(radiusXLength, boxWidth);
> > > +        float radiusY = radiusYLength.isUndefined() ? 0 : floatValueForLength(radiusYLength, boxHeight);
> > 
> > FloatSize?
> 
> > > Source/WebCore/rendering/ExclusionShape.cpp:90
> > > +          ? createExclusionRectangle(x, y, width, height, radiusX, radiusY)
> > > +          : createExclusionRectangle(y, x, height, width, radiusY, radiusX);
> > 
> > Would be great if it can take a FloatRect(), FloatSize(). This is how we usually handle these cases in WebKit. Only exclusions are JS API's.
> 
> I don't see the advantage of packing and unpacking these values for a purely internal function call.  Why is this:
> 
> ...
> { 
>     float x = ... ;
>     float y = ... ;
>     float width = ... ;
>     float height = ... ;
>     localFunction(FloatRect(x, y, width height))
> }
> 
> localFunction(FloatRect rect) {
>     anotherFunction(rect.x, rect.y, rect.width, rect.height);
> }

No, you wouldn't use the variables first. you would do something like

    localFunction(FloatRect(..., ..., ..., ...))

> 
> 
> better than this:
> 
> ...
> { 
> 
>     float x = ... ;
>     float y = ... ;
>     float width = ... ;
>     float height = ... ;
>     localFunction(x, y, width, height);
> }
> 
> localFunction(float x, float y, float width, float height)
>     anotherFunction(x, y, width, height);
> }
localFunction(const FloatRect& rect) {
    anotherFunction(rect);
}

You just need to pass the reference, not 4 floats. Since .x(), .y(), .width(), .height() are inline, you don't have performance lost.


> 
> > > Source/WebCore/rendering/ExclusionShape.h:49
> > > +    float logicalLeft;
> > > +    float logicalRight;
> > > +
> > > +    LineSegment(float logicalLeft, float logicalRight)
> > > +        : logicalLeft(logicalLeft)
> > > +        , logicalRight(logicalRight)
> > > +    {
> > > +    }
> > 
> > Shouldn't a LineSegment have two points? Are these lines just horizontal?
> 
> Yes, LineSegments are horizontal lines.
Can you find a better name? Maybe HorizontalSegment? However, don't have strong feelings about that.

> 
> > > Source/WebCore/rendering/ExclusionShape.h:68
> > > +    virtual void getIncludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const = 0;
> > > +    virtual void getExcludedIntervals(float logicalTop, float logicalBottom, SegmentList&) const = 0;
> > 
> > FloatPoint
> 
> The names of these arguments reflect the structure of the caller, which also keeps the parameters separate, as does the line layout code.  The names were specified in the review for https://bugs.webkit.org/show_bug.cgi?id=95625.

For top, bottom you wouldn't create a FloatPoint. Just seems to be logical incorrect, sorry. But for top,right or width, height or cx,cy or rx,ry please use FloatPoint/FloatSize.

[Hans Muller]

Created attachment 165172 [details]
Patch

[Hans Muller]

Created attachment 165225 [details]
Patch

This version of the patch eschews individual ExclusionShape.cpp parameters for FloatRects, FloatSizes, and FloatPoints.

[Dirk Schulze]

Comment on attachment 165225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165225&action=review

Looks good in general, just some snippets.

> LayoutTests/ChangeLog:3522
> -
> + 

merge error again :)

> Source/WebCore/rendering/ExclusionRectangle.h:49
> +        , m_x(bounds.x())
> +        , m_y(bounds.y())
> +        , m_width(bounds.width())
> +        , m_height(bounds.height())
> +        , m_rx(radii.width())
> +        , m_ry(radii.height())

Do the m_x, m_y... need to be floats? Can't you take FloatRect m_boundingBox, FloatSize m_radius?

> Source/WebCore/rendering/ExclusionShape.cpp:54
> +    return adoptPtr(new ExclusionRectangle(FloatRect(center.x() - radius, center.y() - radius, center.x() + radius, center.y() + radius), FloatSize(radius, radius)));

There is and offset function in FloatSize IIRC. Can you check please?

> Source/WebCore/rendering/ExclusionShape.cpp:83
> +        float x = floatValueForLength(rectangle->x(), boxWidth);
> +        float y = floatValueForLength(rectangle->y(), boxHeight);
> +        float width = floatValueForLength(rectangle->width(), boxWidth);
> +        float height = floatValueForLength(rectangle->height(), boxHeight);

Why is that not a FloatRect?

> Source/WebCore/rendering/ExclusionShape.cpp:85
> +        Length radiusXLength = rectangle->cornerRadiusX();
> +        Length radiusYLength = rectangle->cornerRadiusY();

...and FloatSize?

> Source/WebCore/rendering/ExclusionShape.cpp:87
> +        float radiusX = radiusXLength.isUndefined() ? 0 : floatValueForLength(radiusXLength, boxWidth);
> +        float radiusY = radiusYLength.isUndefined() ? 0 : floatValueForLength(radiusYLength, boxHeight);

Ditto.

> Source/WebCore/rendering/ExclusionShape.cpp:91
> +          ? createExclusionRectangle(FloatRect(x, y, width, height), FloatSize(radiusX, radiusY))
> +          : createExclusionRectangle(FloatRect(y, x, height, width), FloatSize(radiusY, radiusX));

Creating these objects here would be unnecessary then.

> Source/WebCore/rendering/ExclusionShape.cpp:98
> +        float centerX = floatValueForLength(circle->centerX(), boxWidth);
> +        float centerY = floatValueForLength(circle->centerY(), boxHeight);

Ditto. Just waste of memory allocation.

> Source/WebCore/rendering/ExclusionShape.cpp:112
> +        float centerX = floatValueForLength(ellipse->centerX(), boxWidth);
> +        float centerY = floatValueForLength(ellipse->centerY(), boxHeight);
> +        float radiusX = floatValueForLength(ellipse->radiusX(), boxWidth);
> +        float radiusY = floatValueForLength(ellipse->radiusY(), boxHeight);

Ditto.

[Dirk Schulze]

(In reply to comment #15)
> (From update of attachment 165225 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165225&action=review
> 
> Looks good in general, just some snippets.
> 
> > LayoutTests/ChangeLog:3522
> > -
> > + 
> 
> merge error again :)
> 
> > Source/WebCore/rendering/ExclusionRectangle.h:49
> > +        , m_x(bounds.x())
> > +        , m_y(bounds.y())
> > +        , m_width(bounds.width())
> > +        , m_height(bounds.height())
> > +        , m_rx(radii.width())
> > +        , m_ry(radii.height())
> 
> Do the m_x, m_y... need to be floats? Can't you take FloatRect m_boundingBox, FloatSize m_radius?
> 
> > Source/WebCore/rendering/ExclusionShape.cpp:54
> > +    return adoptPtr(new ExclusionRectangle(FloatRect(center.x() - radius, center.y() - radius, center.x() + radius, center.y() + radius), FloatSize(radius, radius)));
> 
> There is and offset function in FloatSize IIRC. Can you check please?
> 
> > Source/WebCore/rendering/ExclusionShape.cpp:83
> > +        float x = floatValueForLength(rectangle->x(), boxWidth);
> > +        float y = floatValueForLength(rectangle->y(), boxHeight);
> > +        float width = floatValueForLength(rectangle->width(), boxWidth);
> > +        float height = floatValueForLength(rectangle->height(), boxHeight);
> 
> Why is that not a FloatRect?
> 
> > Source/WebCore/rendering/ExclusionShape.cpp:85
> > +        Length radiusXLength = rectangle->cornerRadiusX();
> > +        Length radiusYLength = rectangle->cornerRadiusY();
> 
> ...and FloatSize?

I am mistaken on the Length values, these can not be FloatSize. Need to check why they are Length at all.(commuting right now)

> 
> > Source/WebCore/rendering/ExclusionShape.cpp:87
> > +        float radiusX = radiusXLength.isUndefined() ? 0 : floatValueForLength(radiusXLength, boxWidth);
> > +        float radiusY = radiusYLength.isUndefined() ? 0 : floatValueForLength(radiusYLength, boxHeight);
> 
> Ditto.
> 
> > Source/WebCore/rendering/ExclusionShape.cpp:91
> > +          ? createExclusionRectangle(FloatRect(x, y, width, height), FloatSize(radiusX, radiusY))
> > +          : createExclusionRectangle(FloatRect(y, x, height, width), FloatSize(radiusY, radiusX));
> 
> Creating these objects here would be unnecessary then.
> 
> > Source/WebCore/rendering/ExclusionShape.cpp:98
> > +        float centerX = floatValueForLength(circle->centerX(), boxWidth);
> > +        float centerY = floatValueForLength(circle->centerY(), boxHeight);
> 
> Ditto. Just waste of memory allocation.
> 
> > Source/WebCore/rendering/ExclusionShape.cpp:112
> > +        float centerX = floatValueForLength(ellipse->centerX(), boxWidth);
> > +        float centerY = floatValueForLength(ellipse->centerY(), boxHeight);
> > +        float radiusX = floatValueForLength(ellipse->radiusX(), boxWidth);
> > +        float radiusY = floatValueForLength(ellipse->radiusY(), boxHeight);
> 
> Ditto.

[Hans Muller]

(In reply to comment #15)
> (From update of attachment 165225 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165225&action=review
> > Source/WebCore/rendering/ExclusionRectangle.h:49
> > +        , m_x(bounds.x())
> > +        , m_y(bounds.y())
> > +        , m_width(bounds.width())
> > +        , m_height(bounds.height())
> > +        , m_rx(radii.width())
> > +        , m_ry(radii.height())
> 
> Do the m_x, m_y... need to be floats? Can't you take FloatRect m_boundingBox, FloatSize m_radius?

Doing so would make the ExclusionRectangle implementation more difficult to read and I don't think there's any performance speed/size advantage to using struct valued members here.

> > Source/WebCore/rendering/ExclusionShape.cpp:54
> > +    return adoptPtr(new ExclusionRectangle(FloatRect(center.x() - radius, center.y() - radius, center.x() + radius, center.y() + radius), FloatSize(radius, radius)));
> 
> There is and offset function in FloatSize IIRC. Can you check please?

Yes there are FloatSize mutators however using them would either require making the FloatSize center parameter non-const.  That would be a little misleading, since we're only doing so to make it possible to hack the parameter (usually not a good idea).  And are expresions like center.expand(-radius) really more readable than what's here?

Sadly, what's really wrong here are the width and height ExclusionRectangle parameters for both circles and ellipses.  I've fixed that.


> > Source/WebCore/rendering/ExclusionShape.cpp:83
> > +        float x = floatValueForLength(rectangle->x(), boxWidth);
> > +        float y = floatValueForLength(rectangle->y(), boxHeight);
> > +        float width = floatValueForLength(rectangle->width(), boxWidth);
> > +        float height = floatValueForLength(rectangle->height(), boxHeight);
> 
> Why is that not a FloatRect?
> 
> > Source/WebCore/rendering/ExclusionShape.cpp:85
> > +        Length radiusXLength = rectangle->cornerRadiusX();
> > +        Length radiusYLength = rectangle->cornerRadiusY();
> 
> ...and FloatSize?

I used coordinate variables here, and elsewhere, because I think what I've got is easier to read.  When the x's and y's are implicit, because they're positional FloatRect, FloatSize, it's harder to tell what's going on.  For example in this version, it's harder to see that what's happening is that I'm simply swapping x's and y's:

        exclusionShape = horizontalWritingMode
          ? createExclusionRectangle(
                FloatRect(
                    floatValueForLength(rectangle->x(), boxWidth),
                    floatValueForLength(rectangle->y(), boxHeight),
                    floatValueForLength(rectangle->width(), boxWidth),
                    floatValueForLength(rectangle->height(), boxHeight))
                FloatSize(radiusX, radiusY));
          : createExclusionRectangle(
                FloatRect(
                    floatValueForLength(rectangle->y(), boxHeight),
                    floatValueForLength(rectangle->x(), boxWidth),
                    floatValueForLength(rectangle->height(), boxHeight))
                    floatValueForLength(rectangle->width(), boxWidth),
                FloatSize(radiusY, radiusX));

Here's the original version:

        exclusionShape = horizontalWritingMode
          ? createExclusionRectangle(FloatRect(x, y, width, height), FloatSize(radiusX, radiusY))
          : createExclusionRectangle(FloatRect(y, x, height, width), FloatSize(radiusY, radiusX));
 

If the cost of stacking the additional variables is measurable and significant in this case I'd be happy to use the first version.  If not, I think the added readability of the current version is worthwhile.

> Source/WebCore/rendering/ExclusionShape.cpp:98
> +        float centerX = floatValueForLength(circle->centerX(), boxWidth);
> +        float centerY = floatValueForLength(circle->centerY(), boxHeight);

[Hans Muller]

Created attachment 165413 [details]
Patch

[Dirk Schulze]

Comment on attachment 165413 [details]
Patch

To be honest, I am not very happy that you don't store the FloatRect and FloatSize values in the Exclusion* classes. That would look a lot cleaner IMO. However, at this point it might be over engineering to prepare the code for that.

[WebKit Review Bot]

Comment on attachment 165413 [details]
Patch

Clearing flags on attachment: 165413

Committed r129411: <http://trac.webkit.org/changeset/129411>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Bear Travis]

Reopening to attach new patch.

[Bear Travis]

Created attachment 165450 [details]
Attached to incorrect bug

Merging with master branch

[Bear Travis]

Closing. Accidentally uploaded patch for bug 96157 here.