[webkit-reviews] review denied: [Bug 55411] [chromium] premultipliedAlpha WebGL context attribute is ignored. : [Attachment 84143] 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 55411: [chromium] premultipliedAlpha WebGL context attribute is ignored.
https://bugs.webkit.org/show_bug.cgi?id=55411

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

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

The code changes look good. Some more work is needed on the test case.

> LayoutTests/ChangeLog:10
> +	   * compositing/webgl/webgl-nonpremultiplied-blend.html: Added.

You'll need to incorporate expected results for this new test. Chromium's
results for the existing compositing/webgl/ tests seem to be under LayoutTests
in platform/chromium-gpu-{win,mac,linux}/compositing/webgl/ . Despite the fact
that the existing tests in compositing/webgl/ have text on the web page, that
makes the pixel results non-portable, so I would recommend putting the text in
an HTML comment instead. You may then be able to share the pixel results across
platforms and put them under platform/chromium-gpu/ .

Because you aren't fixing the Mac code path in this patch, you'll need to add
your new test to the Skipped file in LayoutTests/platform/mac/ . I don't think
the other ports are running WebGL layout tests by default right now. You'll
also need to add it to that in LayoutTests/platform/mac-wk2/ to keep the bots
green at http://build.webkit.org/console .

See http://www.chromium.org/developers/testing/webkit-layout-tests for
instructions on building DumpRenderTree out of your Chromium tree and running
layout tests. You will likely need to pass the arguments "--debug --platform
chromium-gpu" to run_webkit_tests.sh.

> LayoutTests/compositing/webgl/webgl-nonpremultiplied-blend.html:20
> +	 .compare {
> +	   margin: 20px;
> +	   width: 200px;
> +	   height: 200px;
> +	   background-color: rgba(0, 0, 128, 0.5);
> +	 }

It looks like the "compare" class is left over and should be taken out.

> LayoutTests/compositing/webgl/webgl-nonpremultiplied-blend.html:62
> +    <div class="compare"></div>

This should be taken out.