Bug 116643 – [CSS Exclusions] Minimal support for using an image to define a shape

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Hans Muller]

An initial cut at supporting shapes defined by an image at a threshold: correctly compute the included/excluded line segment intervals.

- No support for shape-image-threshold, will use 0 (applied to the image's alpha channel) for now.
- No support for computing ExclusionShape::firstIncludedIntervalLogicalTop().  Will assume that the first interval fits at shapeLogicalTop.
- No support for shape-margin or shape-padding.
- No native image processing.

[Hans Muller]

Created attachment 204227 [details]
Patch

ChangeLogs are incomplete.  Just giving the bots a shot at this.

[Early Warning System Bot]

Comment on attachment 204227 [details]
Patch

Attachment 204227 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/816144

[EFL EWS Bot]

Comment on attachment 204227 [details]
Patch

Attachment 204227 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/828011

[Build Bot]

Comment on attachment 204227 [details]
Patch

Attachment 204227 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/816147

[EFL EWS Bot]

Comment on attachment 204227 [details]
Patch

Attachment 204227 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/766186

[Build Bot]

Comment on attachment 204227 [details]
Patch

Attachment 204227 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/778246

[Early Warning System Bot]

Comment on attachment 204227 [details]
Patch

Attachment 204227 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/807257

[Hans Muller]

Created attachment 204336 [details]
Patch

Use GraphicsContext3D::packImageData() return value outside of an ASSERT.

[Alexandru Chiculita]

Comment on attachment 204336 [details]
Patch

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,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAASklEQVRYCe3XIQ4AMAhD0XXh/lfuMkeCJjUfhSt5YJDtk6ybDP/ZDFBjBdLuUdjqmawAAQQQQAABBBBAAAEE4gLiN+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.

[Hans Muller]

(In reply to comment #9)
> (From update of attachment 204336 [details])
> 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'd prefer to list all of the related work in the "master" bug, https://bugs.webkit.org/show_bug.cgi?id=116348.  Would that be OK?

> 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.

Definitely.  

> > 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?

The current test case covers #1, I'll add tests for the other two.

> > Source/WebCore/rendering/RenderBlock.cpp:1442
> > +    if (!parent())
> 
> When can this happen?

I included this check because RenderBox::imageChanged() does, and I assume that it exists to guard against handling a notification that occurs after the node has been removed from its parent.

> > 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.

I've only completed the initial support for shape-inside.  I did do some of the work for shape-outside, just where there was an obvious inside/outside symmetry in the code.  I'll be finishing the shape-outside support next.

> > 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.

My mistake: the function should have been #ifdef'd.

> > 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.

Yes, this could be done more efficiently.  If it's OK, I'd like to make performance improvements in subsequent patches.

> > 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

The Shape invariants like this are guaranteed by the createShape() functions, not by the constructors.  In this case I could make the constructor check the invariants, but I wanted to be consistent with the existing code.

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

As far as I can tell, ImageExtractor et al are included in the build unconditionally. We can insulate ourselves from WebGL by separating the ImageExtractor code as noted earlier.

> > 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.

If we're accumulating an interval then startX != -1.  If we're accumulating an interval and the current pixel is below threshold OR we reach the end of the row, then the interval ends.

> > 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?

I just structured the code this way to be consistent with the other createShape() method. In that case it was simpler to just initialize these fields at the end. In this case it's useful to easily be able to see the similarity between the ends of the two methods.

[Hans Muller]

Created attachment 204371 [details]
Patch

Made all of the corrections per Alex's review that I didn't ask to defer.

[Hans Muller]

Created attachment 204372 [details]
Patch

Fixed some whitespace-Os.

[Hans Muller]

Created attachment 204446 [details]
Patch

Removed many noisy FIXMEs and consolidated the pending work list in https://bugs.webkit.org/show_bug.cgi?id=116348#c1

[Hans Muller]

Created attachment 204447 [details]
Patch

Included ChangeLog updates.

[Hans Muller]

Created attachment 204462 [details]
Patch

Added a comment about the Region algorithm used to compute shape intervals.

[Hans Muller]

Created attachment 204463 [details]
Patch

Added a comment about the shape interval algorithm's implementation.

[Hans Muller]

Created attachment 204536 [details]
Patch

Removed incomplete shape-outside support. Conditionally included code that depends on 3DGraphics.  Made removeShapeImageClient() a RenderObject method.

[Hans Muller]

Created attachment 205132 [details]
Patch

Renamed RasterShapeIntervals::getIntervals() to getIncludedIntervals().

[Build Bot]

Comment on attachment 205132 [details]
Patch

Attachment 205132 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/906782

[Hans Muller]

Created attachment 205300 [details]
Patch

Resync'd.

[Dirk Schulze]

Comment on attachment 205300 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205300&action=review

I just looked over the patch and did not read previous comments. Sorry if I missed something. Would be great if you can answer some questions.

> Source/WebCore/rendering/shapes/RasterShape.cpp:42
> +        m_bounds = adoptPtr(new IntRect(m_region.bounds()));
> +    return *m_bounds;

Why is m_bounds a pointer? Seems strange that you don't return a copy of the IntRect. Where is this used?

> Source/WebCore/rendering/shapes/RasterShape.cpp:74
> +

Remove the empty line.

> Source/WebCore/rendering/shapes/RasterShape.cpp:76
> +    for (unsigned i = 0; i < lineRects.size(); i++) {

++i

> Source/WebCore/rendering/shapes/RasterShape.cpp:88
> +    for (unsigned i = 0; i < segmentRects.size(); i++)

++i

> Source/WebCore/rendering/shapes/RasterShape.cpp:98
> +    // FIXME: add support for non-zero margin

Real sentences and, if possible, a link to the bug.

> Source/WebCore/rendering/shapes/RasterShape.cpp:108
> +    // FIXME: add support for non-zero padding

Ditto.

> Source/WebCore/rendering/shapes/RasterShape.cpp:114
> +void RasterShape::getExcludedIntervals(LayoutUnit logicalTop, LayoutUnit logicalHeight, SegmentList& result) const
> +{
> +    UNUSED_PARAM(result);

Just don't define result. The call of UNUSED_PARAM is not needed.

> Source/WebCore/rendering/shapes/RasterShape.cpp:121
> +    float y1 = logicalTop;
> +    float y2 = logicalTop + logicalHeight;

Seems to be unnecessary if it is just used once.

> Source/WebCore/rendering/shapes/RasterShape.cpp:135
> +

no empty line here.

> Source/WebCore/rendering/shapes/RasterShape.cpp:158
> +    // FIXME: complete this method

See comment above.

> Source/WebCore/rendering/shapes/RasterShape.h:52
> +    mutable OwnPtr<IntRect> m_bounds; // Cached value of m_region.bounds().

Why does it need to be a pointer? Do you create it on the fly and fear that it takes to much memory?

> Source/WebCore/rendering/shapes/Shape.cpp:36
> +#include "GraphicsContext3D.h"

Why do you need GC3D for reading an image?

> Source/WebCore/rendering/shapes/Shape.cpp:210
> +    // FIXME: Should clip shape to logical box.
> +    UNUSED_PARAM(logicalBoxSize);

don't define logicalBoxSize

> Source/WebCore/rendering/shapes/Shape.cpp:215
> +    Image* image = styleImage->cachedImage()->image();
> +    GraphicsContext3D::ImageExtractor imageExtractor(image, GraphicsContext3D::HtmlDomNone, false, true);

What does it do? Can you add a comment?

> Source/WebCore/rendering/shapes/Shape.cpp:236
> +        for (int y = 0; y < imageHeight; y++) {

++y

> Source/WebCore/rendering/shapes/Shape.cpp:238
> +            for (int x = 0; x < imageWidth; x++) {

++x

> Source/WebCore/rendering/shapes/Shape.cpp:263
> +    UNUSED_PARAM(styleImage);
> +    UNUSED_PARAM(threshold);
> +    UNUSED_PARAM(logicalBoxSize);
> +

Don't define the arguments.

[Hans Muller]

(In reply to comment #21)
> (From update of attachment 205300 [details])
> > Source/WebCore/rendering/shapes/RasterShape.cpp:42
> > +        m_bounds = adoptPtr(new IntRect(m_region.bounds()));
> > +    return *m_bounds;
> 
> Why is m_bounds a pointer? Seems strange that you don't return a copy of the IntRect. Where is this used?

It's computed lazily and cached.  I'm using !m_bounds to indicate that the value hasn't been computed yet, instead of a separate flag.

The value is used by the RasterShape::getIncludedIntervals() and RasterShapeIntervals::getExcludedIntervals() methods. It's also
used by RasterShape::shapeMarginLogicalBoundingBox() and RasterShape::shapePaddingLogicalBoundingBox().

> > Source/WebCore/rendering/shapes/RasterShape.cpp:135
> > +
> 
> no empty line here.
> 
> > Source/WebCore/rendering/shapes/RasterShape.h:52
> > +    mutable OwnPtr<IntRect> m_bounds; // Cached value of m_region.bounds().
> 
> Why does it need to be a pointer? Do you create it on the fly and fear that it takes to much memory?

No. As with RasterShapeIntervals::m_bounds, I'm using a pointer because the value is lazily computed and cached.

> > Source/WebCore/rendering/shapes/Shape.cpp:36
> > +#include "GraphicsContext3D.h"
> 
> Why do you need GC3D for reading an image?

I'm using GraphicsContext3D::ImageExtractor().

[Hans Muller]

Created attachment 205499 [details]
Patch

Made the suggested changes.

[Dirk Schulze]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 205300 [details] [details])
> > > Source/WebCore/rendering/shapes/RasterShape.cpp:42
> > > +        m_bounds = adoptPtr(new IntRect(m_region.bounds()));
> > > +    return *m_bounds;
> > 
> > Why is m_bounds a pointer? Seems strange that you don't return a copy of the IntRect. Where is this used?
> 
> It's computed lazily and cached.  I'm using !m_bounds to indicate that the value hasn't been computed yet, instead of a separate flag.
> 
> The value is used by the RasterShape::getIncludedIntervals() and RasterShapeIntervals::getExcludedIntervals() methods. It's also
> used by RasterShape::shapeMarginLogicalBoundingBox() and RasterShape::shapePaddingLogicalBoundingBox().


The question is more: How often do you expect that the class gets initiated? If it is 100s or thousands of times, I can understand it. It would be a waste of memory to have 4 floats instead of one pointer. If it is just a couple of times, it is over-engineered. And I would be in favor for replacing it with a "physical" IntRect.

> 
> > > Source/WebCore/rendering/shapes/RasterShape.cpp:135
> > > +
> > 
> > no empty line here.
> > 
> > > Source/WebCore/rendering/shapes/RasterShape.h:52
> > > +    mutable OwnPtr<IntRect> m_bounds; // Cached value of m_region.bounds().
> > 
> > Why does it need to be a pointer? Do you create it on the fly and fear that it takes to much memory?
> 
> No. As with RasterShapeIntervals::m_bounds, I'm using a pointer because the value is lazily computed and cached.

See above.

> 
> > > Source/WebCore/rendering/shapes/Shape.cpp:36
> > > +#include "GraphicsContext3D.h"
> > 
> > Why do you need GC3D for reading an image?
> 
> I'm using GraphicsContext3D::ImageExtractor().

Why this? What does it do? Why not ImageBuffer::imageData? What is the difference?

[Hans Muller]

(In reply to comment #24)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (From update of attachment 205300 [details] [details] [details])
> > > > Source/WebCore/rendering/shapes/RasterShape.cpp:42
> > > > +        m_bounds = adoptPtr(new IntRect(m_region.bounds()));
> > > > +    return *m_bounds;
> > > 
> > > Why is m_bounds a pointer? Seems strange that you don't return a copy of the IntRect. Where is this used?
> > 
> > It's computed lazily and cached.  I'm using !m_bounds to indicate that the value hasn't been computed yet, instead of a separate flag.
> > 
> > The value is used by the RasterShape::getIncludedIntervals() and RasterShapeIntervals::getExcludedIntervals() methods. It's also
> > used by RasterShape::shapeMarginLogicalBoundingBox() and RasterShape::shapePaddingLogicalBoundingBox().
> 
> 
> The question is more: How often do you expect that the class gets initiated? If it is 100s or thousands of times, I can understand it. It would be a waste of memory to have 4 floats instead of one pointer. If it is just a couple of times, it is over-engineered. And I would be in favor for replacing it with a "physical" IntRect.

It's not a question of saving memory, I'm avoiding the cost of calling m_region.bounds() until the value is needed.

> > 
> > > > Source/WebCore/rendering/shapes/Shape.cpp:36
> > > > +#include "GraphicsContext3D.h"
> > > 
> > > Why do you need GC3D for reading an image?
> > 
> > I'm using GraphicsContext3D::ImageExtractor().
> 
> Why this? What does it do? Why not ImageBuffer::imageData? What is the difference?

I've got a CachedImage and I want to extract its alpha channel. ImageExtractor and packImageData() provide away to do this.  To insert an ImageBuffer into the process, I'd have to render the image first?

[Hans Muller]

Created attachment 208286 [details]
Patch

[Dirk Schulze]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #22)
> > > (In reply to comment #21)
> > > > (From update of attachment 205300 [details] [details] [details] [details])
> > > > > Source/WebCore/rendering/shapes/RasterShape.cpp:42
> > > > > +        m_bounds = adoptPtr(new IntRect(m_region.bounds()));
> > > > > +    return *m_bounds;
> > > > 
> > > > Why is m_bounds a pointer? Seems strange that you don't return a copy of the IntRect. Where is this used?
> > > 
> > > It's computed lazily and cached.  I'm using !m_bounds to indicate that the value hasn't been computed yet, instead of a separate flag.
> > > 
> > > The value is used by the RasterShape::getIncludedIntervals() and RasterShapeIntervals::getExcludedIntervals() methods. It's also
> > > used by RasterShape::shapeMarginLogicalBoundingBox() and RasterShape::shapePaddingLogicalBoundingBox().
> > 
> > 
> > The question is more: How often do you expect that the class gets initiated? If it is 100s or thousands of times, I can understand it. It would be a waste of memory to have 4 floats instead of one pointer. If it is just a couple of times, it is over-engineered. And I would be in favor for replacing it with a "physical" IntRect.
> 
> It's not a question of saving memory, I'm avoiding the cost of calling m_region.bounds() until the value is needed.

And it is a questions of balance between maintenance and performance if we add things like that. I leave it up to Alex to do the final review and decision.

> 
> > > 
> > > > > Source/WebCore/rendering/shapes/Shape.cpp:36
> > > > > +#include "GraphicsContext3D.h"
> > > > 
> > > > Why do you need GC3D for reading an image?
> > > 
> > > I'm using GraphicsContext3D::ImageExtractor().
> > 
> > Why this? What does it do? Why not ImageBuffer::imageData? What is the difference?
> 
> I've got a CachedImage and I want to extract its alpha channel. ImageExtractor and packImageData() provide away to do this.  To insert an ImageBuffer into the process, I'd have to render the image first?

I would still prefer that you refactor this code first so that we do not need to reference 3D classes.

[Hans Muller]

Created attachment 208573 [details]
Patch

I've replaced the GraphicsContext3D dependent Shape::createShape() function with one that renders the StyleImage to an ImageBuffer and uses getUnmultipliedImageData() to access the ImageBuffer's raw pixels.

[Alexandru Chiculita]

Comment on attachment 208573 [details]
Patch

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.

[Hans Muller]

Created attachment 208755 [details]
Patch

Made the suggested changes.

[WebKit Commit Bot]

Attachment 208755 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/shapes/shape-inside/shape-inside-image-001-expected.html', u'LayoutTests/fast/shapes/shape-inside/shape-inside-image-001.html', u'LayoutTests/fast/shapes/shape-inside/shape-inside-image-002-expected.html', u'LayoutTests/fast/shapes/shape-inside/shape-inside-image-002.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderBlock.h', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObject.h', u'Source/WebCore/rendering/shapes/RasterShape.cpp', u'Source/WebCore/rendering/shapes/RasterShape.h', u'Source/WebCore/rendering/shapes/Shape.cpp', u'Source/WebCore/rendering/shapes/Shape.h', u'Source/WebCore/rendering/shapes/ShapeInfo.cpp', u'Source/WebCore/rendering/shapes/ShapeInsideInfo.cpp', u'Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp', u'Source/WebCore/rendering/style/ShapeValue.h']" exit_code: 1
Source/WebCore/rendering/shapes/Shape.cpp:224:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hans Muller]

Created attachment 208757 [details]
Patch

Fixed a style-o and added the ChangeLog "reviewed by" entries.

[WebKit Commit Bot]

Comment on attachment 208757 [details]
Patch

Clearing flags on attachment: 208757

Committed r154081: <http://trac.webkit.org/changeset/154081>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 208757 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208757&action=review

> Source/WebCore/rendering/shapes/RasterShape.cpp:51
> +static inline IntRect alignedRect(IntRect r, int y1, int y2)

Should be const IntRect&

> Source/WebCore/rendering/shapes/Shape.cpp:236
> +                for (int x = 0; x < imageSize.width(); ++x, pixelArrayOffset += 4) {
> +                    uint8_t alpha = pixelArray->item(pixelArrayOffset);
> +                    bool alphaAboveThreshold = alpha > alphaPixelThreshold;
> +                    if (startX == -1 && alphaAboveThreshold) {
> +                        startX = x;
> +                    } else if (startX != -1 && (!alphaAboveThreshold || x == imageSize.width() - 1)) {
> +                        intervals->addInterval(y, startX, x);
> +                        startX = -1;
> +                    }
> +                }

This seems pretty expensive; it runs for every pixel. Wouldn't it better to work in for each side until you see a non-alpha pixel?