[webkit-reviews] review granted: [Bug 76804] [chromium] PNG image with CMYK ICC color profile renders color-inverted and squashed : [Attachment 123693] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted noel gordon
<noel.gordon at gmail.com>'s request for review:
Bug 76804: [chromium] PNG image with CMYK ICC color profile renders
color-inverted and squashed
https://bugs.webkit.org/show_bug.cgi?id=76804

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=123693&action=review


> Source/WebCore/platform/image-decoders/ImageDecoder.h:304
> +	   enum Dimension { iccColorProfileHeaderLength = 128 };

You can also make this an anonymous enum, if you're not going to use the type
anywhere.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:312
> +	       if (!memcmp(&profileData[16], "RGB ", 4))
> +		   return true;
> +	       return false;

Why not just:

return !memcmp(&profileData[16], "RGB ", 4);

?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:323
> +	       if (!memcmp(&profileData[12], "mntr", 4))
> +		   return true;
> +	       if (!memcmp(&profileData[12], "scnr", 4))
> +		   return true;
> +	       return false;

Similarly:

return !memcmp(&profileData[12], "mntr", 4) || !memcmp(&profileData[12],
"scnr", 4);

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:256
> +    if (profileLength < ImageDecoder::iccColorProfileHeaderLength)
> +	   ignoreProfile = true;
> +    else if (!ImageDecoder::rgbColorProfile(profileData, profileLength))
> +	   ignoreProfile = true;
> +    else if (!ImageDecoder::inputDeviceColorProfile(profileData,
profileLength))
> +	   ignoreProfile = true;
> +
> +    if (!ignoreProfile)

I would have just combined this all into one if statement rather than using a
temporary, but i'm not sure it's a big deal.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:297
> -	   m_colorProfile = readColorProfile(png, info);
> +	   readColorProfile(png, info, m_colorProfile);

Should we assert that m_colorProfile is empty?	readColorProfile doesn't zero
it out when the profile isn't legit.