[webkit-reviews] review granted: [Bug 48170] [Chromium] Add ICC support for PNG on Mac : [Attachment 72552] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 48170: [Chromium] Add ICC support for PNG on Mac
https://bugs.webkit.org/show_bug.cgi?id=48170

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72552&action=review

> WebCore/platform/image-decoders/ImageDecoder.h:50
> +    typedef Vector<char> ColorProfile;

Should this just be String?

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:74
>      DEFINE_STATIC_LOCAL(RetainPtr<CGColorSpaceRef>, deviceColorSpace,
(AdoptCF, CGColorSpaceCreateDeviceRGB()));
> +
> +    if (colorProfile.isEmpty()) {
> +	   CFRetain(deviceColorSpace.get());
> +	   return deviceColorSpace.get();
> +    }

Why bother storing the local?  Do we know that CG doesn't already use a shared
instance under the covers?

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:260
> +    char* profileName;
> +    int compression;
> +    char* profile;
> +    png_uint_32 profileLength;
> +    png_get_iCCP(png, info, &profileName, &compression, &profile,
&profileLength);
> +    m_colorProfile.clear();
> +    if (profile)
> +	   m_colorProfile.append(profile, profileLength);

I still think this should be a helper function which just returns a
"ColorProfile" (string or otherwise) for better encapsulation.	No need for all
those locals to be in the calling function, etc.