[webkit-reviews] review denied: [Bug 96156] [CSS Exclusions] ExclusionShape API should use logical coordinates for input/output : [Attachment 164598] Patch

Fri Sep 21 01:21:57 PDT 2012

Dirk Schulze <krit at webkit.org> has denied Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 96156: [CSS Exclusions] ExclusionShape API should use logical coordinates
for input/output
https://bugs.webkit.org/show_bug.cgi?id=96156

Attachment 164598: Patch
https://bugs.webkit.org/attachment.cgi?id=164598&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=164598&action=review

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

The ChangeLog for the layout tests is missing.

> Source/WebCore/ChangeLog:809
> +2012-09-14  Hans Muller  <hmuller at adobe.com>
> +
> +	   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
> +2012-09-14  Hans Muller  <hmuller at adobe.com>
> +
> +	   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.