[webkit-reviews] review granted: [Bug 67988] Large canvas fills should not crash or create unnecessarily large image buffers : [Attachment 107448] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ben Wells <benwells at chromium.org>'s
request for review:
Bug 67988: Large canvas fills should not crash or create unnecessarily large
image buffers
https://bugs.webkit.org/show_bug.cgi?id=67988

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=107448&action=review


I’m going to say review+, but I think it would be much better to post the early
return version.

> LayoutTests/fast/canvas/canvas-large-fills-expected.txt:7
> +source-over
> +PASS Actual: 0 Expected: 0
> +PASS Actual: 0 Expected: 0
> +PASS Actual: 255 Expected: 255

This test output is pretty good. I think it would be even better to check these
as a single value; less verbose and easier to understand. The test can convert
them to hex form or some other easy to understand form. More like:

    PASS: source-over: #0000FF

That would cut the number of lines of output by 4 and make the test clearer.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1518
> +    if (!bufferRect.isEmpty()) {

In the WebKit project our usual style is to use “early return”. If we do that
here, a side benefit is that the patch will be smaller. Like this:

    if (bufferRect.isEmpty()) {
	clearCanvas();
	return;
    }

This makes it so we don’t have the whole function indented.