[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 #162408|review?                     |review-
               Flag|                            |




--- Comment #16 from Dave Hyatt <hyatt at apple.com>  2012-09-07 15:33:09 PST ---
(From update of attachment 162408)
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.

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