Created attachment 307650[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 307651[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
Created attachment 307653[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 307657[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 307691[details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 307734[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=307734&action=review> Source/WebCore/platform/ImageTypeIdentifierRegistry.cpp:27
> +#include "ImageTypeIdentifierRegistry.h"
I think we should make this generic and specific, and similar to MIMETypeRegistry: "UTIRegistry", so that in the future when we want to do the same for e.g. video or audio or document types we can, without renaming!
Also, it should probably be USE(CG)-specific, UTIs are an Apple concept.
> Source/WebCore/platform/ImageTypeIdentifierRegistry.cpp:48
> + static NeverDestroyed<HashSet<String>> unsupportedTypetypeIdentifiers(std::initializer_list<String> {
"unsupportedTypetypeIdentifiers" -- type type?
> Source/WebCore/platform/ImageTypeIdentifierRegistry.cpp:88
> + return unsupportedTypetypeIdentifiers.get().contains(typeIdentifier);
This looks like a blacklist to me, not a whitelist; what's to guarantee that new versions of CG don't add support for new image formats that we don't want to expose to the web?
Comment on attachment 307734[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=307734&action=review>> Source/WebCore/platform/ImageTypeIdentifierRegistry.cpp:27
>> +#include "ImageTypeIdentifierRegistry.h"
>
> I think we should make this generic and specific, and similar to MIMETypeRegistry: "UTIRegistry", so that in the future when we want to do the same for e.g. video or audio or document types we can, without renaming!
>
> Also, it should probably be USE(CG)-specific, UTIs are an Apple concept.
The name is fixed. Regarding making the file USE(CG)-specific, I do not think I can do that. Some of the supported image formats don't have corresponding MIME types, so CG has to use the UTI's. For none-CG platforms I made them return a UTI, please see the https://bugs.webkit.org/show_bug.cgi?id=171042. And the plan is use isSupportedImageUTI() on all platforms.
>> Source/WebCore/platform/ImageTypeIdentifierRegistry.cpp:48
>> + static NeverDestroyed<HashSet<String>> unsupportedTypetypeIdentifiers(std::initializer_list<String> {
>
> "unsupportedTypetypeIdentifiers" -- type type?
Fixed.
>> Source/WebCore/platform/ImageTypeIdentifierRegistry.cpp:88
>> + return unsupportedTypetypeIdentifiers.get().contains(typeIdentifier);
>
> This looks like a blacklist to me, not a whitelist; what's to guarantee that new versions of CG don't add support for new image formats that we don't want to expose to the web?
Fixed.
Created attachment 307778[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 307780[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
Created attachment 307781[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 307794[details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 307815[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=307815&action=review> Source/WebCore/platform/UTIRegistry.cpp:46
> +static void initializeWhitelistImageUTIs(HashSet<String>& whitelistImageUTIs)
If this returns the HashSet instead, then you can...
> Source/WebCore/platform/UTIRegistry.cpp:88
> + static NeverDestroyed<HashSet<String>> s_whitelistImageUTIs;
... just do an = initializeWhitelistImageUTIs(); here...
> Source/WebCore/platform/UTIRegistry.cpp:91
> + if (!s_whitelistImageUTIs.get().size())
> + initializeWhitelistImageUTIs(s_whitelistImageUTIs.get());
allowing you to get rid of this entirely.
Comment on attachment 307815[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=307815&action=review> Source/WebCore/platform/UTIRegistry.cpp:81
> + // Assume that all implementations at least support the following standard
> + // image types:
> + whitelistImageUTIs = std::initializer_list<String> {
> + "public.jpeg",
> + "public.png",
> + "com.compuserve.gif",
> + "com.microsoft.bmp",
> + "com.microsoft.ico",
> +#if USE(WEBP)
> + "com.google.webp"
> +#endif
This is only called from USE(CG) code, so I think you should just delete this and make the whole thing guarded by USE(CG).
> Source/WebCore/platform/UTIRegistry.h:33
> +HashSet<String>& whitelistImageUTIs();
allowedImageUTIs(), I think.
Comment on attachment 307822[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=307822&action=review> Source/WebCore/ChangeLog:14
> + * platform/graphics/cg/UTIRegistryCG.cpp: Added.
Not likely that there will ever be a non-CG version of this, so you could just leave the CG off (usually we add the suffix if it's the CG parts of a cross-platform class).
Comment on attachment 308013[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=308013&action=review> Source/WebCore/ChangeLog:9
> + WebKit should support only a whitelist image formats. WebKit will not support new
> + image formats till they get broader industry adoption.
"should support only whitelisted image formats".
Also, this isn't a generic WebKit policy (obviously), this is about CG ports. I think you should drop the second sentence, too, since even that isn't a guarantee of support, of course. Maybe write something helpful about the code change you made instead?
2017-04-20 14:36 PDT, Said Abou-Hallawa
2017-04-20 14:56 PDT, Said Abou-Hallawa
2017-04-20 15:44 PDT, Build Bot
2017-04-20 15:45 PDT, Build Bot
2017-04-20 15:51 PDT, Build Bot
2017-04-20 16:20 PDT, Build Bot
2017-04-20 16:52 PDT, Said Abou-Hallawa
2017-04-20 18:52 PDT, Said Abou-Hallawa
2017-04-20 22:07 PDT, Build Bot
2017-04-21 08:55 PDT, Said Abou-Hallawa
2017-04-21 11:20 PDT, Said Abou-Hallawa
2017-04-21 12:06 PDT, Said Abou-Hallawa
2017-04-21 13:01 PDT, Build Bot
2017-04-21 13:04 PDT, Build Bot
2017-04-21 13:05 PDT, Build Bot
2017-04-21 13:59 PDT, Build Bot
2017-04-21 14:26 PDT, Said Abou-Hallawa
2017-04-21 15:14 PDT, Said Abou-Hallawa
2017-04-21 16:05 PDT, Said Abou-Hallawa
2017-04-21 16:29 PDT, Said Abou-Hallawa
2017-04-21 16:39 PDT, Said Abou-Hallawa
2017-04-21 18:12 PDT, Said Abou-Hallawa
2017-04-24 14:46 PDT, Said Abou-Hallawa
2017-04-24 16:32 PDT, Said Abou-Hallawa