[webkit-reviews] review granted: [Bug 116643] [CSS Exclusions] Minimal support for using an image to define a shape : [Attachment 208573] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Aug 14 09:58:48 PDT 2013
Alexandru Chiculita <achicu at adobe.com> has granted Hans Muller
<giles_joplin at yahoo.com>'s request for review:
Bug 116643: [CSS Exclusions] Minimal support for using an image to define a
shape
https://bugs.webkit.org/show_bug.cgi?id=116643
Attachment 208573: Patch
https://bugs.webkit.org/attachment.cgi?id=208573&action=review
------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=208573&action=review
> Source/WebCore/ChangeLog:11
> + https://bugs.webkit.org/show_bug.cgi?id=116348 for the current list.
nit: You should note that there's no cross-domain check yet. Might be worth
having a bug for it already.
> Source/WebCore/rendering/RenderBlock.cpp:1449
> + ShapeInsideInfo* shapeInsideInfo = ensureShapeInsideInfo();
> + shapeInsideInfo->dirtyShapeSize();
nit: there's a lot of "shapeInsideInfo" on these two lines :)
> Source/WebCore/rendering/shapes/RasterShape.cpp:113
> + // FIXME: this method is only a stub, see
https://code.google.com/p/chromium/issues/detail?id=252737.
nit: you may need to update this to a webkit bug instead.
> Source/WebCore/rendering/shapes/Shape.cpp:228
> + for (int x = 0; x < imageSize.width() && pixelArrayOffset <
pixelArrayLength; ++x, pixelArrayOffset += 4) {
nit: You don't need to check pixelArrayOffset < pixelArrayLength. You already
have an assert for it.
> Source/WebCore/rendering/shapes/Shape.cpp:230
> + if ((startX == -1) && alpha > alphaPixelThreshold) {
nit: I would extract alpha > alphaPixelThreshold as a local variable. You have
the same expression in both branches.
nit: There's no need for the first set of parentheses around startX. One line
if block-statements do not need the brackets around them.
> Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp:49
> + return false;
Is there a bug added for this case? We should have a fixme in here.
More information about the webkit-reviews
mailing list