Bug 55170 – ImageBuffer::clip creates an image of the incorrect context in IOSurface case

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Matthew Delaney]

Currently, when using IOSURFACE_CANVAS_BACKING_STORE, we're creating an image of the passed in context instead of the context of the imageBuffer (the correct one). This patch will serve to fix that.

The code fix is tiny, but I'm searching for clients of ImageBuffer::clip that would have been affected by this. Specifically, these clients must be creating ImageBuffers that use IOSurfaces as the backing store. At the moment, this is only CanvasRenderingContext2d. So, I suspect the reason I didn't notice this sooner is because this code path isn't being used. It does appear that CanvasRenderingContext2D::drawTextInternal uses it though. Will investigate that now.

[Matthew Delaney]

Created attachment 83697 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 83697 [details]
Patch

Would be nice to make a test case for this.

[Matthew Delaney]

Created attachment 83723 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 83723 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:249
> +void ImageBuffer::clip(GraphicsContext* clippingContext, const FloatRect& rect) const
>  {
> -    CGContextRef platformContext = context->platformContext();
> +    CGContextRef platformContext = clippingContext->platformContext();

I don't find that clippingContext clarifies much. Is the one doing clipping, or being clipped? Also, if you use this naming, platformContext should be platformClippingContext.

> LayoutTests/fast/canvas/2d.fillText.gradient.html:2
> +<title>2d.fillText.gradient.html</title>

The title doesn't help anything; the browser will show the filename in the titlebar if there is no <title>.

> LayoutTests/fast/canvas/2d.fillText.gradient.html:6
> +<p>On success, the square should appear entirely green. On failure, the red back should show through.</p>
> +<canvas id="c" class="output" width="100" height="100"><p class="fallback">FAIL (fallback content)</p></canvas>
> +<div id="console"></div>

Is this wording correct? I thought the failure mode was to paint too much.

> LayoutTests/fast/canvas/2d.fillText.gradient.html:20
> +  ctx.font = "1000px Times";

You could use the Ahem font to get a rectangle.

> LayoutTests/fast/canvas/2d.fillText.gradient.html:35
> +var renderedCorrectly = true;
> +var imageData = ctx.getImageData(0,0,50,50);
> +if (imageData.data[0] != 0) renderedCorrectly = false;
> +if (imageData.data[1] != 255) renderedCorrectly = false;
> +if (imageData.data[2] != 0) renderedCorrectly = false;
> +if (imageData.data[3] != 255) renderedCorrectly = false;

You should also test that pixels outside of the glyph are not affected by the gradient.

> LayoutTests/fast/canvas/2d.fillText.gradient.html:45
> +</html>
>  \ No newline at end of file

Please fix that.

> LayoutTests/platform/mac/fast/canvas/2d.fillText.gradient-expected.txt:15
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x180
> +  RenderBlock {HTML} at (0,0) size 800x180
> +    RenderBody {BODY} at (8,16) size 784x156
> +      RenderBlock {P} at (0,0) size 784x18
> +        RenderText {#text} at (0,0) size 623x18
> +          text run at (0,0) width 623: "On success, the square should appear entirely green. On failure, the red back should show through."
> +      RenderBlock (anonymous) at (0,34) size 784x104
> +        RenderText {#text} at (0,0) size 0x0
> +      RenderBlock {DIV} at (0,138) size 784x18
> +        RenderText {#text} at (0,0) size 124x18
> +          text run at (0,0) width 124: "Rendered correctly."
> +layer at (8,50) size 100x100
> +  RenderHTMLCanvas {CANVAS} at (0,0) size 100x100

This should be a dumpAsText().

[Matthew Delaney]

Created attachment 83727 [details]
Patch

[Matthew Delaney]

Committed r79651: <http://trac.webkit.org/changeset/79651>