[webkit-reviews] review denied: [Bug 116643] [CSS Exclusions] Minimal support for using an image to define a shape : [Attachment 204336] Patch

Tue Jun 11 14:00:42 PDT 2013

Alexandru Chiculita <achicu at adobe.com> has denied 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 204336: Patch
https://bugs.webkit.org/attachment.cgi?id=204336&action=review

------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=204336&action=review

Looks good, just a couple of comments. I would like to see bug numbers next to
each Fixme, so that we are tracking that correctly.

I think that we also need to start the discussions about CORS and getting
access to the pixels. One could use shapes to get access to the alpha channel
of a cross-domain image and that is usually a red flag for security. Moreover,
an image could be analyzed by just loading an SVG that loads the image inside
and applies a color matrix filter on it to move color channels to the alpha
channel.

> LayoutTests/ChangeLog:6
> +	   One test to verify that the initial implementation of shape valued
images is working

You have a couple of edge cases that I'm not sure you check in here:
1. What happens if the last pixel on the line is below the threshold?
2. What happens if the last pixel on the line is above the threshold?
3. What happens if there's no alpha information in the image?

>
LayoutTests/fast/exclusions/shape-inside/shape-inside-image-001-expected.html:7

> +<script>
> +    if (window.internals)
> +	   window.internals.settings.setCSSShapesEnabled(true);
> +</script>

You don't need to enable shapes in the ref test.

> LayoutTests/fast/exclusions/shape-inside/shape-inside-image-001.html:15
> +	   -webkit-shape-inside:
url("data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAASklE
QVRYCe3XIQ4AMAhD0XXh/lfuMkeCJjUfhSt5YJDtk6ybDP/ZDFBjBdLuUdjqmawAAQQQQAABBBBAAAE
E4gLiN+xfSqKP38ADuQMJO6LNbr4AAAAASUVORK5CYII=");

Add a comment that the threshold is supposed to be zero, so that when we
implement that property, we know what the test is expecting. Maybe link to the
bug number for that property too.

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:-1
> -ï»¿<?xml version="1.0" encoding="utf-8"?>

nit: You might need to revert this change manually.

> Source/WebCore/rendering/RenderBlock.cpp:1442
> +    if (!parent())

When can this happen?

> Source/WebCore/rendering/RenderBlock.cpp:1446
> +    if (shapeValue  && shapeValue->image() && shapeValue->image()->data() ==
image) {

nit: extra space between "shapeValue" and "&&"

> Source/WebCore/rendering/RenderObject.cpp:1849
> +    updateShapeImage(oldStyle ? oldStyle->shapeOutside() : 0, m_style ?
m_style->shapeOutside() : 0);

In the changelog you said you are only doing the shapeInside.

> Source/WebCore/rendering/RenderObject.cpp:2628
> +static inline void removeShapeImageClient(RenderObject *thisRenderObject,
ShapeValue* shapeValue)

This looks like a good candidate for a RenderObject method. Function will end
up unused if ENABLE(CSS_SHAPES) is false.

> Source/WebCore/rendering/RenderObject.cpp:2657
> +	   removeShapeImageClient(this, m_style->shapeOutside());

ditto, the changelog only talks about shape inside.

> Source/WebCore/rendering/shapes/RasterShape.cpp:2
> + * Copyright (C) 2012 Adobe Systems Incorporated. All rights reserved.

nit: 2013 :)

> Source/WebCore/rendering/shapes/RasterShape.cpp:65
> +    // FIXME: if lineRegion.isRect() .. we're done

nit: Comments should be phrases. Also add bugs that will track these issues.

> Source/WebCore/rendering/shapes/RasterShape.cpp:67
> +    Vector<IntRect> lineRects = lineRegion.rects(); // FIXME: add a
non-copying Region::rects() method

nit: the fixmes are usually on the previous line.

> Source/WebCore/rendering/shapes/RasterShape.cpp:87
> +    Region segmentsRegion(lineRect);
> +    Region intervalsRegion;
> +
> +    int lineY = lineRects[0].y();
> +    for (unsigned i = 0; i < lineRects.size(); i++) {
> +	   if (lineRects[i].y() != lineY) {
> +	       segmentsRegion.intersect(intervalsRegion);
> +	       intervalsRegion = Region(); // FIXME: add a region clear()
method
> +	   }
> +	   intervalsRegion.unite(Region(alignedRect(lineRects[i], y1, y2)));
> +	   lineY = lineRects[i].y();
> +    }
> +    if (!intervalsRegion.isEmpty())
> +	   segmentsRegion.intersect(intervalsRegion);
> +
> +    Vector<IntRect> segmentRects = segmentsRegion.rects();
> +    for (unsigned i = 0; i < segmentRects.size(); i++)
> +	   result.append(LineSegment(segmentRects[i].x(),
segmentRects[i].maxX()));

If I understand correctly, all you need to do here is to find the spaces that
are not intersecting any of the lineRects. Looks like that should be doable in
one pass without using regions.

> Source/WebCore/rendering/shapes/RasterShape.cpp:92
> +    ASSERT(shapeMargin() >= 0);

What code is making sure that the shapeMargin is always bigger or equal to 0?
Looks like the input is always converted from Length. I think the assert should
be in the constructor. This object is immutable, so it's better to catch the
problem as soon as it happens.

> Source/WebCore/rendering/shapes/RasterShape.cpp:102
> +    ASSERT(shapePadding() >= 0);

ditto

> Source/WebCore/rendering/shapes/RasterShape.h:2
> + * Copyright (C) 2012 Adobe Systems Incorporated. All rights reserved.

nit: 2012 - > 2013

> Source/WebCore/rendering/shapes/Shape.cpp:204
> +    Image* image = styleImage->cachedImage()->image();

WebGL would check access with CORS before accessing the pixels of an image. I
suppose we will need to do the same. Add a fixme for that too.

> Source/WebCore/rendering/shapes/Shape.cpp:205
> +    GraphicsContext3D::ImageExtractor imageExtractor(image,
GraphicsContext3D::HtmlDomNone, false, true);

Let's add a FIXME to move ImageExtractor out of the GC3D. Add a bug for it too.

Is there any WebGL compile-time flag that would make the ImageExtractor
unavailable when Shapes are enabled?

> Source/WebCore/rendering/shapes/Shape.cpp:234
> +		   } else if (startX != -1 && (alpha <= alphaPixelThreshold ||
x == imageWidth - 1)) {

Not sure how the region would work in this case, but you actually have two
cases in here:
alpha <= alphaPixelThreshold => the pixel at "x" should NOT BE included in the
interval.
x == imageWidth - 1 => the pixel at "x" should BE included in the interval.

> Source/WebCore/rendering/shapes/Shape.cpp:245
> +    rasterShape->m_writingMode = writingMode;
> +    rasterShape->m_margin = floatValueForLength(margin, 0);
> +    rasterShape->m_padding = floatValueForLength(padding, 0);

Why not put these in the constructor?

> Source/WebCore/rendering/style/ShapeValue.h:69
>      StyleImage* image() const { return m_image.get(); }
> +    bool isImageValid() const { return image() && image()->cachedImage() &&
image()->cachedImage()->hasImage(); }
>      void setImage(PassRefPtr<StyleImage> image)

nit: these functions should have an ASSERT for the type.