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

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied 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 72432: Patch
https://bugs.webkit.org/attachment.cgi?id=72432&action=review

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

Overall this looks like a fantastic patch!  I look forward to seeing other
platforms support this!

I'd like to see the code moved/my comments answered before landing though.

r- for the below and the build break.

> WebCore/platform/image-decoders/cg/ImageDecoderCG.cpp:80
> +	   RetainPtr<CGDataProviderRef> profileDataProvider(AdoptCF,
CGDataProviderCreateWithCFData(profileData.get()));
> +	   CGFloat ranges[] = {0.0, 255.0, 0.0, 255.0, 0.0, 255.0};
> +	   colorSpace.adoptCF(CGColorSpaceCreateICCBased(3, ranges,
profileDataProvider.get(), deviceColorSpace.get()));

If you worked at Apple, you would just add a WKCGColorSpaceCreateWithICCProfile
to WebKitSystemInterface.lib (since undoubtably this method existed on Leopard
but was just private). Since you don't, I would suggest you create a
WKCGColorSpaceCreateWithICCProfile anyway, just make it local to this file, and
on SnowLeopard and later use the nice API and have this crazy code only exist
on Leopard.

Make sense?

Then an Apple person could move your method into WebKitSystemInterface if
desired at some point, but it's likely better to keep it public until we drop
Leopard support anyway.

We should consider a deterimineColorSpace() helper method here which just
returned the CGColorSpace?  Or maybe this is so clean with
WKCGColorSpaceCreateWithICCProfile that we won't need that?

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:257
> +    png_get_iCCP(png, info, &profileName, &compression, &profile,
&profileLength);

Who owns the returned strings?	Do we need to release them?

Maybe we should put this into a separate function with a nice WebKity API?  One
that returns the m_colorProfile or some such?