[webkit-reviews] review denied: [Bug 55170] ImageBuffer::clip creates an image of the incorrect context in IOSurface case : [Attachment 83723] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Matthew Delaney
<mdelaney at apple.com>'s request for review:
Bug 55170: ImageBuffer::clip creates an image of the incorrect context in
IOSurface case
https://bugs.webkit.org/show_bug.cgi?id=55170

Attachment 83723: Patch
https://bugs.webkit.org/attachment.cgi?id=83723&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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().