[webkit-reviews] review granted: [Bug 47477] WebGL shows PNG Textures with indexed colors too dark : [Attachment 80533] Patch

[bugzilla-daemon at webkit.org]

Chris Marrin <cmarrin at apple.com> has granted Kenneth Russell <kbr at google.com>'s
request for review:
Bug 47477: WebGL shows PNG Textures with indexed colors too dark
https://bugs.webkit.org/show_bug.cgi?id=47477

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

------- Additional Comments from Chris Marrin <cmarrin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80533&action=review

Looks good. r=me with minor nits

> Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:132
> +	   // seem to work unless it's premultiplied.

There's no need to explain other ways this could be done, unless this is a
FIXME in which case it should be marked as such. Otherwise a shorter comment
(like the last sentence) would suffice

> Source/WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp:140
> +	   CGContextDrawImage(bitmapContext.get(), CGRectMake(0, 0, width,
height), cgImage);

The style guidelines don't say anything about it, but I think a  blank	here
and after the 'return false' above would be more clear.

> LayoutTests/fast/canvas/webgl/gl-teximage.html:206
> +	for (var ii = 0; ii < 2; ++ii) {

extra space here

> LayoutTests/fast/canvas/webgl/gl-teximage.html:208
> +		if (ii == 0) {

wrong indenting

> LayoutTests/fast/canvas/webgl/gl-teximage.html:232
> +	   }

No braces on either clause here

> LayoutTests/fast/canvas/webgl/gl-teximage.html:238
> +	 assertMsg(ge128Count[jj] > 256 * 0.45,

It would be more clear to put the 0.45 into a const

> LayoutTests/fast/canvas/webgl/gl-teximage.html:389
> +  // The image should be red.

I think a line between the comment and the line above would be more clear