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

[bugzilla-daemon at webkit.org]

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 165225: Patch
https://bugs.webkit.org/attachment.cgi?id=165225&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
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.