Bug 42214 – chromium/skia: Fix canvas.toDataURL in the presence of transparency

• Component: New Bugs
• Status: RESOLVED FIXED

[Nico Weber]

chromium/skia: Fix canvas.toDataURL in the presence of transparency

[Nico Weber]

Created attachment 61436 [details]
Patch

[Nico Weber]

The patch still needs a test case (despite claim in the changelog), but the code change is complete. Comments?

[Nico Weber]

Oh, this is http://crbug.com/26456 , and I need to credit deanm in the changelog as well.

[Nico Weber]

Created attachment 61446 [details]
Patch

[Nico Weber]

Now with tests and attribution.

[Adam Langley]

LGTM

[Ojan Vafai]

Comment on attachment 61446 [details]
Patch

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.checksum b/LayoutTests/platform/mac-leopard/fast/canvas/toDataURL-alpha-expected.checksum
> 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.

[Nico Weber]

Created attachment 61690 [details]
Patch

[Nico Weber]

> 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?

Don.

> > 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.

jamesr told me to put them there because text rendering is different on Snow Leopard, which would cause this to turn the 10.6 bot red.

[Ojan Vafai]

(In reply to comment #9)
> > 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.
> 
> jamesr told me to put them there because text rendering is different on Snow Leopard, which would cause this to turn the 10.6 bot red.

If this were a test where pixel results were different on Snow Leopard, then it would make sense to do this, then grab the pixel results for Snow Leopard off the bot (assuming we had webkit pixel bots anymore) and put them in platform/mac.

But, not all text rendering is different. I think that most of the time Leopard matches Snow Leopard. I'd be surprised to find that this test rendered differently on Snow Leopard than Leopard. Maybe I'm missing something though.

[James Robinson]

Text _always_ renders differently on Leopard vs Snow Leopard due to subpixel AA differences.  Thus pixel results for tests with text on mac are always Leopard or Snow Leopard specific.  In this case, as the test exists now someone running with -p on Snow Leopard would not see any expected results.  If the results were in platform/mac then someone running with -p on Snow Leopard would see small pixel diffs (if they had fuzzy matching enabled) or get a pixel failure (if they didn't have fuzzy matching enabled).

Currently nobody runs pixel tests in an automated fashion except for Chromium and Chromium only runs them on Leopard, so this won't break anyone either way.  However since we know that these results are going to only be valid on Leopard I'd much rather put them in mac-leopard now instead of having to move them manually later.

[James Robinson]

Naturally adding Snow Leopard specific results to platform/mac in addition to the Leopard specific results in platform/mac-leopard is the best option of them all :)

[WebKit Commit Bot]

Comment on attachment 61690 [details]
Patch

Clearing flags on attachment: 61690

Committed r63494: <http://trac.webkit.org/changeset/63494>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/63494 might have broken GTK Linux 32-bit Release and Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/63494
http://trac.webkit.org/changeset/63495

[Nico Weber]

gtk/qt guys: That was probably me. Looks like minor, non-important differences. Could that be caused by fonts? How is this usually handled? Do you just add platform-specific expectations?

[Ojan Vafai]

(In reply to comment #16)
> gtk/qt guys: That was probably me. Looks like minor, non-important differences. Could that be caused by fonts? How is this usually handled? Do you just add platform-specific expectations?

The chromium-win/chromium-linux bots are not finding the expected png. Looking at the code, they don't check mac-leopard. They only fallback to mac.

[Nico Weber]

ojan: jamesr told me the usual procedure is to check in an expected failure for chromium (which I think I did, see my change to LayoutTests/platform/chromium/test_expectations.txt), then grab baselines from the bots later and check them in together with a change that removes the expected failure.

This is my first time doing this, so chances are I screwed something up, but that was my plan :-)

[Ojan Vafai]

(In reply to comment #18)
> ojan: jamesr told me the usual procedure is to check in an expected failure for chromium (which I think I did, see my change to LayoutTests/platform/chromium/test_expectations.txt), then grab baselines from the bots later and check them in together with a change that removes the expected failure.

Ah right. That is the right thing to do. The only catch is that the expectations file differentiates between a pixel failure and a missing result. So, instead of IMAGE here, you should have put MISSING...I think. :) This stuff is unfortunately very complicated.

[Ojan Vafai]

I went ahead and marked it as MISSING.

[Adam Barth]

This patch is wrong.

LayoutTests/fast/canvas/toDataURL-alpha-expected.txt

is a render tree dump.  It should be in the platform/mac directory.  In this patch, it's in the place where a text result should be, which is confusing all the tools.

Will fix.

[Adam Barth]

Result moved in http://trac.webkit.org/changeset/63512

[Adam Barth]

Gtk and Qt baselines in http://trac.webkit.org/changeset/63513

The lesson here is the render tree dumps are very platform specific because of font metrics.  Expected render tree dumps go in the platform folder for the appropriate port.  Text expected results are much more likely to be cross-port, so they go in the main LayoutTest tree.

When you land a render tree dump in the cross-port folder, every port thinks that it should work for it, but it doesn't.  If you landed the results in the platform/mac folder, Qt and Gtk would know that the expected results were missing for them and the builders wouldn't get red and unhappy.

[Nico Weber]

Thanks, Adam. Sorry for the nuisance :-/

[Adam Barth]

(In reply to comment #24)
> Thanks, Adam. Sorry for the nuisance :-/

No problem.  Sorry I was distracted in the channel and couldn't answer you questions in real time.

[Nico Weber]

Baselines for win and linux for this patch are at https://bugs.webkit.org/show_bug.cgi?id=42488