[webkit-reviews] review denied: [Bug 56238] Non-premultiplied-alpha canvas attribute is ignore for toDataURL, drawImage, texImage2D : [Attachment 85767] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied John Bauman
<jbauman at chromium.org>'s request for review:
Bug 56238: Non-premultiplied-alpha canvas attribute is ignore for toDataURL,
drawImage, texImage2D
https://bugs.webkit.org/show_bug.cgi?id=56238

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=85767&action=review

The code changes look good but there are a few issues with the test case that
need to be fixed.

> LayoutTests/fast/canvas/webgl/premultiplyalpha-test.html:49
> +	console.log(gl.getContextAttributes());

This should be removed.

> LayoutTests/fast/canvas/webgl/premultiplyalpha-test.html:70
> +	img.src = png;
> +	img.onload = function() {

There are issues like this race condition that I mentioned in the upstream bug
report on Khronos. Please fix those and re-convert this test case into a layout
test.

> LayoutTests/fast/canvas/webgl/resources/webgl-test-utils.js:109
> -  '	  gl_FragColor = texture2D(tex, texCoord);\n' +
> +  '	  gl_FragData[0] = texture2D(tex, texCoord);\n' +

This change is invalid. gl_FragData is unsupported in GLSL ES 2.0.

> LayoutTests/platform/chromium/test_expectations.txt:2360
> +BUGWEBGL : fast/canvas/webgl/premultiplyalpha-test.html = TEXT

Why is this new test not passing? It seems wrong that it doesn't pass even in
Chromium.

> LayoutTests/platform/mac/Skipped:306
> +fast/canvas/webgl/premultipliedalpha-test.html

Since the new test doesn't involve drawing a WebGL canvas to the screen whose
premultipliedAlpha flag is set to false, it seems that we should be able to
make the new test pass on Mac as well. Is that not possible?

> Source/WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:106
> +	       preMultipliedBGRAtoRGBA(pixels,
> +				       imageSize.width(), row.data());

Line break here is unnecessary.