[webkit-reviews] review denied: [Bug 42214] chromium/skia: Fix canvas.toDataURL in the presence of transparency : [Attachment 61446] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Nico Weber <thakis at chromium.org>'s
request for review:
Bug 42214: chromium/skia: Fix canvas.toDataURL in the presence of transparency
https://bugs.webkit.org/show_bug.cgi?id=42214

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
Code changes look fine. Just a few test comments.

> diff --git a/LayoutTests/fast/canvas/toDataURL-alpha.html
b/LayoutTests/fast/canvas/toDataURL-alpha.html
> +	 var d = document;
> +	 var c = d.getElementById("c").getContext("2d");
> +	 c.fillStyle = "rgba(200, 255, 200, 0.7)";
> +	 c.fillRect(0, 0, c.canvas.width, c.canvas.height);
> +	 d.getElementById("d").style.backgroundImage = ["url(",
c.canvas.toDataURL("image/png"), ")"].join("");

Short variable names are generally frowned upon in WebKit code. I know this is
just a test, but it makes this hard to read. Can you s/c/context and either not
alias document or s/d/doc?

> diff --git
a/LayoutTests/platform/mac-leopard/fast/canvas/toDataURL-alpha-expected.checksu
m
b/LayoutTests/platform/mac-leopard/fast/canvas/toDataURL-alpha-expected.checksu
m
> diff --git
a/LayoutTests/platform/mac-leopard/fast/canvas/toDataURL-alpha-expected.png
b/LayoutTests/platform/mac-leopard/fast/canvas/toDataURL-alpha-expected.png

These should be in platform/mac or next to the test. Snow Leopard, for example,
won't know to find the expected results in this directory.