Bug 171077 – Whitelist supported image MIME types

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

This API should return the UTIs of the images instead of MIME types because some of UTIs do not have corresponding MIME types.

[Said Abou-Hallawa]

Created attachment 307637 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307641 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/31743803>

[Build Bot]

Comment on attachment 307641 [details]
Patch

Attachment 307641 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3572422

Number of test failures exceeded the failure limit.

[Build Bot]

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

[Build Bot]

Comment on attachment 307641 [details]
Patch

Attachment 307641 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3572470

Number of test failures exceeded the failure limit.

[Build Bot]

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

[Build Bot]

Comment on attachment 307641 [details]
Patch

Attachment 307641 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3572490

Number of test failures exceeded the failure limit.

[Build Bot]

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

[Build Bot]

Comment on attachment 307641 [details]
Patch

Attachment 307641 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3572469

Number of test failures exceeded the failure limit.

[Build Bot]

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

[Said Abou-Hallawa]

Created attachment 307661 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307674 [details]
Patch

[Build Bot]

Comment on attachment 307674 [details]
Patch

Attachment 307674 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3573990

New failing tests:
http/tests/local/blob/send-hybrid-blob-using-open-panel.html

[Build Bot]

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

[Simon Fraser (smfr)]

Comment on attachment 307674 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307674&action=review

> Source/WebCore/ChangeLog:3
> +        Add an API to return a list of the WebKit supported image formats

I don' think this is API, right? It's not exposed through WKWebView?

[Said Abou-Hallawa]

Created attachment 307734 [details]
Patch

[Tim Horton]

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?

[Said Abou-Hallawa]

Created attachment 307756 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307768 [details]
Patch

[Said Abou-Hallawa]

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.

[Build Bot]

Comment on attachment 307768 [details]
Patch

Attachment 307768 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3577828

Number of test failures exceeded the failure limit.

[Build Bot]

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

[Build Bot]

Comment on attachment 307768 [details]
Patch

Attachment 307768 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3577819

Number of test failures exceeded the failure limit.

[Build Bot]

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

[Build Bot]

Comment on attachment 307768 [details]
Patch

Attachment 307768 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3577851

Number of test failures exceeded the failure limit.

[Build Bot]

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

[Build Bot]

Comment on attachment 307768 [details]
Patch

Attachment 307768 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3578013

Number of test failures exceeded the failure limit.

[Build Bot]

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

[Said Abou-Hallawa]

Created attachment 307809 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307815 [details]
Patch

[Tim Horton]

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.

[Tim Horton]

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.

[Said Abou-Hallawa]

Created attachment 307822 [details]
Patch

[Tim Horton]

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).

[Said Abou-Hallawa]

Created attachment 307834 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307836 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307857 [details]
Patch

[Said Abou-Hallawa]

Created attachment 308013 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 307857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307857&action=review

> Source/WebCore/ChangeLog:15
> +        * platform/graphics/cg/UTIRegistryCG.cpp: Added.

This entry is wrong. The file UTIRegistryCG.cpp was renamed to UTIRegistry.cpp. I will submit an update.

[Tim Horton]

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?

[Said Abou-Hallawa]

Created attachment 308022 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 308022 [details]
Patch

Clearing flags on attachment: 308022

Committed r215706: <http://trac.webkit.org/changeset/215706>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.