The image URL can be used to get the type identifier hint. Without providing this hint, the image type identifier is not accurate for image formats.
Created attachment 307557 [details] Patch
<rdar://problem/31725669>
Created attachment 307572 [details] Patch
Comment on attachment 307572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307572&action=review > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:51 > +CFStringRef CGImageSourceGetTypeWithURL(CFURLRef, CFStringRef); This function should not be declared in the .cpp file. Please use an actual *SPI.h header, similar to WebCore/platform/spi/cg/CoreGraphicsSPI.h. I don't think that we have ImageIOSPI.h yet - maybe we didn't need it before, or maybe those went into some other header. Furthermore, this pattern is not quite correct. We want to redeclare functions when APPLE_INTERNAL_SDK is true - that way, the compiler has a chance to detect mistakes and changes in signatures. Ditto for kCGImageSourceSubsampleFactor/kCGImageSourceShouldCacheImmediately. These are exported from ImageIO.framework, so we should be using the actual exported symbol even in open source builds, not create a new constant with the same name.
Created attachment 307598 [details] Patch
Comment on attachment 307598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307598&action=review > Source/WebCore/ChangeLog:80 > + * platform/spi/cg/ImageIOSPI.h: Added. Not sure if this goes to cocoa/ or to cg/. > Source/WebCore/platform/spi/cg/ImageIOSPI.h:35 > +extern const CFStringRef kCGImageSourceSubsampleFactor; > +extern const CFStringRef kCGImageSourceShouldCacheImmediately; I think that constants can and should be redeclared, too.
Created attachment 307604 [details] Patch
Created attachment 307609 [details] Patch
Comment on attachment 307609 [details] Patch Attachment 307609 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3571451 Number of test failures exceeded the failure limit.
Created attachment 307622 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 307609 [details] Patch Attachment 307609 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3571470 Number of test failures exceeded the failure limit.
Created attachment 307624 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 307609 [details] Patch Attachment 307609 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3571585 Number of test failures exceeded the failure limit.
Created attachment 307625 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 307609 [details] Patch Attachment 307609 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3571526 Number of test failures exceeded the failure limit.
Created attachment 307631 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 307669 [details] Patch
Created attachment 307671 [details] Patch
Comment on attachment 307671 [details] Patch Attachment 307671 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3573462 Number of test failures exceeded the failure limit.
Created attachment 307676 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 307671 [details] Patch Attachment 307671 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3573464 Number of test failures exceeded the failure limit.
Created attachment 307678 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 307671 [details] Patch Attachment 307671 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3573492 Number of test failures exceeded the failure limit.
Created attachment 307680 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 307671 [details] Patch Attachment 307671 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3573475 Number of test failures exceeded the failure limit.
Created attachment 307683 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 307743 [details] Patch
Comment on attachment 307743 [details] Patch Attachment 307743 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3577069 Number of test failures exceeded the failure limit.
Created attachment 307747 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 307749 [details] Patch
Comment on attachment 307749 [details] Patch Attachment 307749 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3577349 Number of test failures exceeded the failure limit.
Created attachment 307762 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 307749 [details] Patch Attachment 307749 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3577373 Number of test failures exceeded the failure limit.
Created attachment 307763 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 307749 [details] Patch Attachment 307749 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3577368 Number of test failures exceeded the failure limit.
Created attachment 307765 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 307773 [details] Patch
Comment on attachment 307773 [details] Patch Attachment 307773 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3578125 Number of test failures exceeded the failure limit.
Created attachment 307792 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 307773 [details] Patch Attachment 307773 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3578039 Number of test failures exceeded the failure limit.
Created attachment 307796 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 307773 [details] Patch Attachment 307773 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3578202 Number of test failures exceeded the failure limit.
Created attachment 307798 [details] Archive of layout-test-results from ews112 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 307773 [details] Patch Attachment 307773 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3578191 Number of test failures exceeded the failure limit.
Created attachment 307799 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 307814 [details] Patch
Created attachment 307840 [details] Patch
Created attachment 307844 [details] Patch
Created attachment 307990 [details] Patch
Created attachment 307992 [details] Patch
Comment on attachment 307990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307990&action=review > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:162 > + RetainPtr<CFMutableDictionaryRef> options = adoptCF(CFDictionaryCreateMutable(nullptr, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks)); Why Mutable? Should be able to just use CFDictionaryCreate with your one value. > Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:242 > +EncodedDataStatus PNGImageDecoder::encodedDataStatus() const I don't think all this consting and const_casting is good. Can you just make the new thing mutable instead?
Created attachment 308006 [details] Patch
Comment on attachment 307990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=307990&action=review >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:162 >> + RetainPtr<CFMutableDictionaryRef> options = adoptCF(CFDictionaryCreateMutable(nullptr, 0, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks)); > > Why Mutable? Should be able to just use CFDictionaryCreate with your one value. Done. >> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:242 >> +EncodedDataStatus PNGImageDecoder::encodedDataStatus() const > > I don't think all this consting and const_casting is good. Can you just make the new thing mutable instead? I agree that this looks ugly but I think it is okay since the const here is a logical const. I think the best fix is to move the call to decode() into ImageDecoder::setData() and make encodedDataStatus() just returns a member of the ImageDecoder class. Please see my comment https://bugs.webkit.org/show_bug.cgi?id=170730#c10. If Miguel takes this approach, things will be fixed the way we want. If not I am going to file a follow up bug to remove this ugliness.
Comment on attachment 308006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308006&action=review > Source/WebCore/platform/graphics/Image.cpp:80 > -String Image::sourceURL() const > +URL Image::sourceURL() const If it were me, I would do the mechanical change in a separate patch, but this is fine too. > Source/WebCore/platform/graphics/Image.h:124 > + virtual String filenameExtension() const { return emptyString(); } // null string if unknown The comment is no longer true. Empty string != null string. That change seems potentially problematic for callers, too. > Source/WebCore/platform/image-decoders/ImageDecoder.h:44 > +class URL; This file is indented wrong, but you should probably stick with it instead of doing your own thing.
Created attachment 308025 [details] Patch
Comment on attachment 308025 [details] Patch Rejecting attachment 308025 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 308025, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ecoder.h patching file Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp patching file Source/WebCore/platform/image-decoders/png/PNGImageDecoder.h patching file Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp patching file Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.h patching file Source/WebCore/platform/spi/cg/ImageIOSPI.h Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/3597784
Created attachment 308029 [details] Patch
Comment on attachment 308029 [details] Patch Clearing flags on attachment: 308029 Committed r215710: <http://trac.webkit.org/changeset/215710>
All reviewed patches have been landed. Closing bug.
Comment on attachment 308029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308029&action=review > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:159 > + RetainPtr<CFURLRef> url = sourceURL.createCFURL(); > + RetainPtr<CFStringRef> utiHint = adoptCF(CGImageSourceGetTypeWithURL(url.get(), nullptr)); What does this do with (potentially huge) data URIs?
Comment on attachment 308029 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308029&action=review >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:159 >> + RetainPtr<CFStringRef> utiHint = adoptCF(CGImageSourceGetTypeWithURL(url.get(), nullptr)); > > What does this do with (potentially huge) data URIs? This code has changed in <http://trac.webkit.org/changeset/217246>. The URL is not used anymore to get the type hint. Instead we get the hint from the data and we call CGImageSourceGetTypeWithData() instead of calling CGImageSourceGetTypeWithURL().