Bug 48170

Created attachment 71617 [details] work in progress

Created attachment 71629 [details] work in progress

This patch is also required on the Chromium side to enable this feature of libpng: http://codereview.chromium.org/4060002

Created attachment 72432 [details] Patch

Attachment 72432 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/platform/image-decoders/ImageDecoder.h:50: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 72432 [details] did not build on qt: Build output: http://queues.webkit.org/results/4870043

Attachment 72432 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4763085

Comment on attachment 72432 [details] Patch 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?

Created attachment 72490 [details] Patch

> > 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? They're owned by libpng, that's why we copy them into a vector immediately. libpng is responsible for deleting them. > Maybe we should put this into a separate function with a nice WebKity API? One that returns the m_colorProfile or some such? The goal of PNGImageDecoder::headerAvailable is to copy data out of the png_infop and into WebKity land. PNGImageDecoder then makes it available with a more sensible API.

Created attachment 72552 [details] Patch

Attachment 72552 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/platform/image-decoders/ImageDecoder.h:50: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 72552 [details] Patch 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.

Created attachment 72574 [details] for landing

Committed r71065: <http://trac.webkit.org/changeset/71065>

http://trac.webkit.org/changeset/71065 might have broken GTK Linux 64-bit Debug The following tests are not passing: accessibility/aria-activedescendant-crash.html accessibility/aria-checkbox-text.html accessibility/aria-hidden-update.html accessibility/contenteditable-hidden-div.html accessibility/crash-with-noelement-selectbox.html accessibility/crashing-a-tag-in-map.html accessibility/document-attributes.html accessibility/first-letter-text-transform-causes-crash.html accessibility/hang-in-isignored.html accessibility/nested-layout-crash.html accessibility/nochildren-elements.html accessibility/non-data-table-cell-title-ui-element.html accessibility/non-native-image-crash.html accessibility/radio-button-checkbox-size.html accessibility/removed-anonymous-block-child-causes-crash.html accessibility/removed-continuation-element-causes-crash.html accessibility/table-modification-crash.html accessibility/table-nofirstbody.html accessibility/table-notbody.html accessibility/table-with-empty-thead-causes-crash.html

Comment on attachment 72574 [details] for landing View in context: https://bugs.webkit.org/attachment.cgi?id=72574&action=review > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:230 > + png_get_iCCP(png, info, &profileName, &compressionType, &profile, &profileLength); Turns out you need to check the return value. Otherwise your values are garbage.

Created attachment 72608 [details] now with null check

Committed r71098: <http://trac.webkit.org/changeset/71098>