Created attachment 368485 [details] Patch

<rdar://problem/48781098>

Created attachment 368632 [details] Patch

Comment on attachment 368632 [details] Patch Can we add layout tests for this?

Created attachment 369055 [details] Patch

Created attachment 369059 [details] Patch

Comment on attachment 369059 [details] Patch Attachment 369059 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12097566 New failing tests: fast/images/animated-heics.html

Created attachment 369063 [details] Archive of layout-test-results from ews107 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 369059 [details] Patch Attachment 369059 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12098014 New failing tests: svg/repaint/change-background-color.html

Created attachment 369067 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Comment on attachment 369059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369059&action=review r- because I don't think this is correctly resetting state between tests. > Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:81 > void setAdditionalSupportedImageTypes(const Vector<String>& imageTypes) This seem more like appendAdditionSupportedImageTypes, since you can't unregister a type, right? That also implies that you're never cleaning up this state between tests correctly. > Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:94 > +void setAdditionalSupportedImageTypes(const String& imageTypes) If this is only used for testing, maybe append "ForTesting" to the function name.

Created attachment 369128 [details] Patch

Comment on attachment 369128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369128&action=review > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:50 > +const CFStringRef WebCoreCGImagePropertyHEICSUnclampedDelayTime = CFSTR("UnclampedDelayTime"); This is unused. > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:367 > + if (CFNumberRef num = (CFNumberRef)CFDictionaryGetValue(heicsProperties, WebCoreCGImagePropertyHEICSDictionary)) Is the property name for the number value really also WebCoreCGImagePropertyHEICSDictionary? It seems like maybe this was supposed to be WebCoreCGImagePropertyHEICSUnclampedDelayTime?

Created attachment 369136 [details] Patch

Comment on attachment 369059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369059&action=review >> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:81 >> void setAdditionalSupportedImageTypes(const Vector<String>& imageTypes) > > This seem more like appendAdditionSupportedImageTypes, since you can't unregister a type, right? > > That also implies that you're never cleaning up this state between tests correctly. For DRT and WTR, this function is now called twice. It is called first for the TestOptions then it is called when WebView or WebPage is created. So it is set correctly the first time but it is cleared the second time. I did it this way to prevent clearing it from creating the WebView and the WebPage. But you are right, this fix will not clean the state between tests correctly. I think the solution is to move the condition imageTypes.isEmpty() to WebView and WebPage creation functions. >> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:94 >> +void setAdditionalSupportedImageTypes(const String& imageTypes) > > If this is only used for testing, maybe append "ForTesting" to the function name. Done.

Comment on attachment 369128 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369128&action=review >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:367 >> + if (CFNumberRef num = (CFNumberRef)CFDictionaryGetValue(heicsProperties, WebCoreCGImagePropertyHEICSDictionary)) > > Is the property name for the number value really also WebCoreCGImagePropertyHEICSDictionary? It seems like maybe this was supposed to be WebCoreCGImagePropertyHEICSUnclampedDelayTime? Fixed. Thanks for catching this error.

(In reply to Said Abou-Hallawa from comment #16) > Comment on attachment 369128 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=369128&action=review > > >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:367 > >> + if (CFNumberRef num = (CFNumberRef)CFDictionaryGetValue(heicsProperties, WebCoreCGImagePropertyHEICSDictionary)) > > > > Is the property name for the number value really also WebCoreCGImagePropertyHEICSDictionary? It seems like maybe this was supposed to be WebCoreCGImagePropertyHEICSUnclampedDelayTime? > > Fixed. Thanks for catching this error. Please add a test which exercise this path.

Comment on attachment 369136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369136&action=review > Source/WebKit/WebProcess/WebPage/WebPage.cpp:642 > + if (!parameters.additionalSupportedImageTypes.isEmpty()) > + WebCore::setAdditionalSupportedImageTypes(parameters.additionalSupportedImageTypes); Why the isEmpty check here? Is this making up for the lack of an appendAdditionalSupportedImageTypes? > Source/WebKitLegacy/mac/WebView/WebView.mm:2886 > + if ([[preferences additionalSupportedImageTypes] count]) > + WebCore::setAdditionalSupportedImageTypes(WebCore::webCoreStringVectorFromNSStringArray([preferences additionalSupportedImageTypes])); Why the count check here? > Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:173 > bool booleanForKey(WKDictionaryRef, const char* key); > + String stringForKey(WKDictionaryRef, const char* key); These can be static, I think?

Created attachment 369169 [details] Patch

Comment on attachment 369136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369136&action=review >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:642 >> + WebCore::setAdditionalSupportedImageTypes(parameters.additionalSupportedImageTypes); > > Why the isEmpty check here? Is this making up for the lack of an appendAdditionalSupportedImageTypes? Fixed. The isEmpty() check was removed. I thought the TestOptions will parsed first. Then [WKWebView _initializeWithConfiguration] will be called to create the WTR WKWebView. It turned out I was wrong and [WKWebView _initializeWithConfiguration] is not called in the WTR case. >> Source/WebKitLegacy/mac/WebView/WebView.mm:2886 >> + WebCore::setAdditionalSupportedImageTypes(WebCore::webCoreStringVectorFromNSStringArray([preferences additionalSupportedImageTypes])); > > Why the count check here? Fixed. the count check was removed. >> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:173 >> + String stringForKey(WKDictionaryRef, const char* key); > > These can be static, I think? No. They can't be static nor can they even be const. The reason is these functions call InjectedBundle::outputText() which is not static or const.

(In reply to Sam Weinig from comment #17) > (In reply to Said Abou-Hallawa from comment #16) > > Comment on attachment 369128 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=369128&action=review > > > > >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:367 > > >> + if (CFNumberRef num = (CFNumberRef)CFDictionaryGetValue(heicsProperties, WebCoreCGImagePropertyHEICSDictionary)) > > > > > > Is the property name for the number value really also WebCoreCGImagePropertyHEICSDictionary? It seems like maybe this was supposed to be WebCoreCGImagePropertyHEICSUnclampedDelayTime? > > > > Fixed. Thanks for catching this error. > > Please add a test which exercise this path. I do not know any tool which can edit/create a HEICS image.

Comment on attachment 369169 [details] Patch Attachment 369169 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12116421 New failing tests: legacy-animation-engine/compositing/reflections/load-video-in-reflection.html

Created attachment 369205 [details] Archive of layout-test-results from ews213 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews213 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

(In reply to Said Abou-Hallawa from comment #21) > (In reply to Sam Weinig from comment #17) > > (In reply to Said Abou-Hallawa from comment #16) > > > Comment on attachment 369128 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=369128&action=review > > > > > > >> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:367 > > > >> + if (CFNumberRef num = (CFNumberRef)CFDictionaryGetValue(heicsProperties, WebCoreCGImagePropertyHEICSDictionary)) > > > > > > > > Is the property name for the number value really also WebCoreCGImagePropertyHEICSDictionary? It seems like maybe this was supposed to be WebCoreCGImagePropertyHEICSUnclampedDelayTime? > > > > > > Fixed. Thanks for catching this error. > > > > Please add a test which exercise this path. > > I do not know any tool which can edit/create a HEICS image. Can you ask the ImageIO folks for one?

Created attachment 369524 [details] Patch

Created attachment 369538 [details] Patch

Created attachment 369556 [details] Patch

I cleaned ImageDecoderCG::repetitionCount() and ImageDecoderCG::frameDurationAtIndex() by deleting the repeating code and avoiding unnecessary calls to the image properties dictionary.

Comment on attachment 369556 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=369556&action=review > Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:340 > + if (properties) > + frameProperties = (CFDictionaryRef)CFDictionaryGetValue(properties.get(), kCGImagePropertyGIFDictionary); > + > + if (properties && !frameProperties) > + frameProperties = (CFDictionaryRef)CFDictionaryGetValue(properties.get(), kCGImagePropertyPNGDictionary); > + This looks like the code above. Maybe share into a small static function.

Created attachment 369819 [details] Patch

Comment on attachment 369819 [details] Patch Clearing flags on attachment: 369819 Committed r245280: <https://trac.webkit.org/changeset/245280>

All reviewed patches have been landed. Closing bug.

The tests, which were submitted with this patch, have to be skipped in WebKit for now. Committed r245292: <https://trac.webkit.org/changeset/245292>