[webkit-reviews] review denied: [Bug 95625] [CSS Exclusions] add support for a subset of the basic shapes : [Attachment 161830] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Hans Muller <giles_joplin at yahoo.com>'s
request for review:
Bug 95625: [CSS Exclusions] add support for a subset of the basic shapes
https://bugs.webkit.org/show_bug.cgi?id=95625

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=161830&action=review


Overall I'm left scratching my head here regarding the mixture of physical and
logical terminology. Your platform/graphics additions seem to be using largely
physical terminology, but then everything you're doing with them over in the
rendering code is in terms of logical coordinates (i.e., line top and line
bottom are x values in vertical writing-modes, but you're happily passing those
values in to your XShape code, which then calls them "y").

I think you need to have a clear design intent here. Either the shapes are
physical, in which case you need to be doing a translation at the WrapShapeInfo
level, or the shapes themselves are logical, in which case I don't think you
should be using x and y terminology in their implementations.

I'm also wondering what the reasoning is behind putting all of the classes into
platform/graphics. Especially if they end up being logical, I'm not sure there
is any reason to have them over there instead of just putting them in the
rendering directory.

> Source/WebCore/ChangeLog:8
> +	   Initial commit of a subset of the Xhapes classes.

Typo, should be XShapes.

> Source/WebCore/ChangeLog:10
> +	   This set of classes enables the exclusions layout code to determine
how to break up a line into segements

Typo, segements should be segments.

> Source/WebCore/ChangeLog:11
> +	   that will fit within or around a shape, given the Y coordinates of
the line's top edge and bottom edges.

Do you mean logical top rather than Y?

> Source/WebCore/platform/graphics/XSRectangle.h:39
> +class XSRectangle : public XShape {

I think a comment above this class declaration describing what it represents
might be helpful here.

> Source/WebCore/platform/graphics/XSRectangle.h:54
> +    virtual FloatRect shapeBoundingBox() const { return FloatRect(m_x, m_y,
m_width, m_height); }
> +    virtual void getOutsideIntervals(float y1, float y2,
Vector<XSInterval>&) const;
> +    virtual void getInsideIntervals(float y1, float y2, Vector<XSInterval>&)
const;

You're missing OVERRIDEs on these three functions

> Source/WebCore/platform/graphics/XSRectangle.h:62
> +    float m_rx;
> +    float m_ry;

I would add a comment here explaining what these are. A person who isn't
familiar with this code will see x,y,width,height and understand those, but may
be left scratching their head wondering what m_rx and m_ry are here.

> Source/WebCore/platform/graphics/XShape.h:43
> +    static PassOwnPtr<XShape> createXShape(const BasicShape*, float
borderBoxWidth, float borderBoxHeight);

borderBoxWidth and borderBoxHeight are physical terminology. Is this physical
or logical? I'm confused about this.

> Source/WebCore/rendering/WrapShapeInfo.cpp:110
> +    m_xshape = XShape::createXShape(shape, logicalWidth, logicalHeight);

Here you're passing in logical values to a function whose (see my comment
above)	arguments had physical names. Which is correct?

> Source/WebCore/rendering/WrapShapeInfo.h:70
> +    LayoutUnit shapeTop() const 

Is this logical or physical? If it's supposed to be a value that is compared
with a line top or bottom, then this should be renamed to shapeLogicalTop().

> Source/WebCore/rendering/WrapShapeInfo.h:103
> +    FloatRect shapeBounds = m_xshape->shapeBoundingBox();

This appears to be logical. Is that the intent?