[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
bugzilla-daemon at webkit.org
Fri Sep 7 15:32:54 PDT 2012
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.
More information about the webkit-reviews
mailing list