[webkit-reviews] review denied: [Bug 95625] [CSS Exclusions] add support for a subset of the basic shapes : [Attachment 162408] 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 162408: Patch
https://bugs.webkit.org/attachment.cgi?id=162408&action=review

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


Looks pretty good. Just minor comments.

> Source/WebCore/ChangeLog:499
> -	   * platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.cpp:
> +	   * rendering/harfbuzz/FontPlatformDataHarfBuzz.cpp:

Not sure what this is about? I don't see this code in the patch.

> Source/WebCore/ChangeLog:504
> -	   * platform/graphics/harfbuzz/FontPlatformDataHarfBuzz.h:
> +	   * rendering/harfbuzz/FontPlatformDataHarfBuzz.h:

Ditto.

> Source/WebCore/rendering/ExclusionShape.h:43
> +    static PassOwnPtr<ExclusionShape> createExclusionShape(const
BasicShape*, float borderBoxWidth, float borderBoxHeight);

Rename borderBoxWidth and borderBoxHeight to borderBoxLogicalWidth and
borderBoxLogicalHeight, since right now at least, your shapes are clearly
logical.

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

I'd just drop the x here. At this point, inside this class, it's pretty obvious
what we're dealing with, so I would just call this m_shape.

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

Rename this to shapeLogicalTop(), since right now at least, it's clearly
logical.

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

Let's call the local shapeLogicalBoundingBox to help clarify it is logical in
nature.