Bug 55170 - ImageBuffer::clip creates an image of the incorrect context in IOSurface case
Summary: ImageBuffer::clip creates an image of the incorrect context in IOSurface case
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Matthew Delaney
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-24 12:02 PST by Matthew Delaney
Modified: 2011-02-24 18:08 PST (History)
1 user (show)

See Also:


Attachments
Patch (1.85 KB, patch)
2011-02-24 12:21 PST, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (5.25 KB, patch)
2011-02-24 14:35 PST, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (5.66 KB, patch)
2011-02-24 15:15 PST, Matthew Delaney
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Delaney 2011-02-24 12:02:07 PST
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.
Comment 1 Matthew Delaney 2011-02-24 12:21:45 PST
Created attachment 83697 [details]
Patch
Comment 2 Simon Fraser (smfr) 2011-02-24 13:54:04 PST
Comment on attachment 83697 [details]
Patch

Would be nice to make a test case for this.
Comment 3 Matthew Delaney 2011-02-24 14:35:52 PST
Created attachment 83723 [details]
Patch
Comment 4 Simon Fraser (smfr) 2011-02-24 14:45:17 PST
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().
Comment 5 Matthew Delaney 2011-02-24 15:15:44 PST
Created attachment 83727 [details]
Patch
Comment 6 Matthew Delaney 2011-02-24 18:08:17 PST
Committed r79651: <http://trac.webkit.org/changeset/79651>