[Webkit-unassigned] [Bug 95625] [CSS Exclusions] add support for a subset of the basic shapes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=95625


Dave Hyatt <hyatt at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #161830|review?                     |review-
               Flag|                            |




--- Comment #5 from Dave Hyatt <hyatt at apple.com>  2012-09-05 12:22:21 PST ---
(From update of attachment 161830)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.