[CSS Shapes] off-by-one error in Shape::createRasterShape()

• Component: CSS
• Status: RESOLVED FIXED

[Hans Muller]

This bug was reported and fixed in Blink by David Vest: https://codereview.chromium.org/237123002/

The fix needs to be ported to WebKit.

[Hans Muller]

Created attachment 230318 [details]
Patch

This is a port of a patch for a bug that was reported by and fixed in Blink by David Vest: https://codereview.chromium.org/237123002/. Shape::createRasterShape() now consistently reports "end-point exclusive" intervals. Before the patch an entire row of pixels was above the shape-image-threshold, the interval's end index was reported as image.width. Now it's image.width + 1, which is consistent with the way the end index is reported if the last above threshold pixel is within an image row.

Two existing tests were revised to account for this change.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/shapes/RasterShape.cpp:173:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 6 files


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

[Hans Muller]

Created attachment 230319 [details]
Patch

[Hans Muller]

Created attachment 230322 [details]
Patch

Fixed a typo.

[Bem Jones-Bey]

Comment on attachment 230322 [details]
Patch

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

> Source/WebCore/rendering/shapes/RasterShape.cpp:174
> +    // Note: marginIntervals() returns "end-point exclusive" intervals.
> +    // The value of excludedInterval.x1() is the first index of an 
> +    // above-threshold pixel within the y1 - y2 line. The value of x2
> +    // is the 1+ the index of the last above-threshold pixel.

I would remove this comment from here and put something similar to it inside of createRasterShape where the endpoint exclusive interval is actually created.

> Source/WebCore/rendering/shapes/Shape.cpp:205
> +                        int endX = alphaAboveThreshold ? x + 1 : x;

if alphaAboveThreshold, doesn't x == imageRect.width() - 1? It seems to me that it would be clearer to readers of the code if you said:
int endX = alphaAboveThreshold ? imageRect.width() : x;

Regardless, I think that comment about end point exclusive intervals would be better placed here than at the usage point of the function, since it is really explaining why you are doing this contortion with endX instead of just using x directly.

[Bem Jones-Bey]

Comment on attachment 230322 [details]
Patch

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

> Source/WebCore/rendering/shapes/RasterShape.cpp:174
> +    // Note: marginIntervals() returns "end-point exclusive" intervals.
> +    // The value of excludedInterval.x1() is the first index of an 
> +    // above-threshold pixel within the y1 - y2 line. The value of x2
> +    // is the 1+ the index of the last above-threshold pixel.

I would remove this comment from here and put something similar to it inside of createRasterShape where the endpoint exclusive interval is actually created.

> Source/WebCore/rendering/shapes/Shape.cpp:205
> +                        int endX = alphaAboveThreshold ? x + 1 : x;

if alphaAboveThreshold, doesn't x == imageRect.width() - 1? It seems to me that it would be clearer to readers of the code if you said:
int endX = alphaAboveThreshold ? imageRect.width() : x;

Regardless, I think that comment about end point exclusive intervals would be better placed here than at the usage point of the function, since it is really explaining why you are doing this contortion with endX instead of just using x directly.

[Hans Muller]

Created attachment 230379 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 230379 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 230379 [details]
Patch

Clearing flags on attachment: 230379

Committed r167938: <http://trac.webkit.org/changeset/167938>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

<rdar://problem/16772806>