Bug 190454

Summary: Add an SPI to allow WebView clients to add additional supported image formats
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: ImagesAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, dbates, dino, esprehn+autocc, ews-watchlist, graouts, gyuyoung.kim, japhet, kondapallykalyan, mitz, sam, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=178758
Bug Depends on: 190838    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2018-10-10 18:20:22 PDT
Currently WebKit supports a fixed set of image formats. WebKit cancels loading the image, if its formats is not in the whitelist, and marks it as a broken image. The rationale for having a fixed small list is WebKit wants to support only standardized and well tested image formats.

But some WebView clients had requested to render still-none-standardized image formats, like HEIF for example, if the system supports decoding and rendering them. This SPI will allow reading the image format whitelist and setting it to a new value.
Comment 1 Said Abou-Hallawa 2018-10-10 18:26:52 PDT
Created attachment 352007 [details]
Patch
Comment 2 Said Abou-Hallawa 2018-10-10 18:29:17 PDT
*** Bug 178821 has been marked as a duplicate of this bug. ***
Comment 3 Said Abou-Hallawa 2018-10-10 18:30:41 PDT
<rdar://problem/32840877>
Comment 4 Simon Fraser (smfr) 2018-10-10 18:39:52 PDT
Comment on attachment 352007 [details]
Patch

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

This doesn't seem like the right approach.

> Source/WebCore/page/Settings.yaml:587
> +imageFormatWhitelist:
> +    type: String
> +    defaultValue: defaultImageFormatWhitelist()

It's not clear to me that the whitelist should be a setting.

> Source/WebCore/platform/graphics/Image.h:203
> +    String m_imageFormatWhitelist;

Why does every image have to store the whitelist? The whitelist should live in some object that gets consulted (next to MimeTypeRegistery or something).
Comment 5 Wenson Hsieh 2018-10-10 19:06:41 PDT
Comment on attachment 352007 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKPreferencesPrivate.h:81
> +@property (nonatomic, setter=_setImageFormatWhitelist:) NSString *_imageFormatWhitelist WK_API_AVAILABLE(macosx(10.14), ios(12.0));

Nit - TBA?
Comment 6 Said Abou-Hallawa 2018-10-12 22:17:25 PDT
Created attachment 352252 [details]
Patch
Comment 7 Said Abou-Hallawa 2018-10-12 22:28:26 PDT
I have these open questions:

1. Is the SPI name AdditionalAllowedImageUTIs() appropriate?
2. Should the allowed formats be passed as UTIs or mime types?
3. It is okay to pass the image formats as one string? Or do we really have to have the allowed formats be passed as an array of strings? 

In WebPreferencesStore, the value of the preference is stored as this variant.

using Value = Variant<String, bool, uint32_t, double>;

If we need to pass it as an array on strings, I will need to add a new type to this Variant and add the necessary encoding and decoding. But this can be done in a separate patch before or after landing this patch.
Comment 8 Tim Horton 2018-10-12 23:21:49 PDT
(In reply to Said Abou-Hallawa from comment #7)
> I have these open questions:
> 
> 1. Is the SPI name AdditionalAllowedImageUTIs() appropriate?

Doesn't seem crazy. Not sure about the "UTIs" part.

> 2. Should the allowed formats be passed as UTIs or mime types?

This is Cocoa-y API so probably UTIs. Also didn't you say that we can't represent all of the desired types as MIME types?

> 3. It is okay to pass the image formats as one string? Or do we really have
> to have the allowed formats be passed as an array of strings? 

Definitely think the SPI should take `NSArray <NSString *> *`. Weird delimited strings are bad API design.

> In WebPreferencesStore, the value of the preference is stored as this
> variant.
> 
> using Value = Variant<String, bool, uint32_t, double>;
> 
> If we need to pass it as an array on strings, I will need to add a new type
> to this Variant and add the necessary encoding and decoding. But this can be
> done in a separate patch before or after landing this patch.
Comment 9 Tim Horton 2018-10-12 23:24:07 PDT
Comment on attachment 352252 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:104
> +@property (nonatomic, setter=_setAdditionalAllowedImageUTIs:) NSString *_additionalAllowedImageUTIs;

Missing availability macros. Also strings need to be `copy` because they can be secretly mutable. And actually you do copy, you just don't say it here.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:214
> +#include <WebCore/UTIRegistry.h>

Why?

> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1203
> +				5558482521718CDA00FB7FFF /* tga-image.png in Copy Resources */,

Why's the TGA a .png?
Comment 10 Daniel Bates 2018-10-13 22:33:26 PDT
Comment on attachment 352252 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:73
> +static String& additionalAllowedImageUTIs()

The name of this function reads like it returns a collection because you use the pluralized acronym form of Uniform Type Identifier, UTIs. But this function only returns a single String and the rest of the patch talks in pluralities though is singular in nature. I would prefer that the solution we come up with is to allow more than one UTI to added to the list of UTIs to recognize rather than making all name singular.
Comment 11 Said Abou-Hallawa 2018-10-14 15:38:39 PDT
Created attachment 352281 [details]
Patch
Comment 12 Said Abou-Hallawa 2018-10-14 15:49:03 PDT
Comment on attachment 352252 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:73
>> +static String& additionalAllowedImageUTIs()
> 
> The name of this function reads like it returns a collection because you use the pluralized acronym form of Uniform Type Identifier, UTIs. But this function only returns a single String and the rest of the patch talks in pluralities though is singular in nature. I would prefer that the solution we come up with is to allow more than one UTI to added to the list of UTIs to recognize rather than making all name singular.

I changed the return type to be Vector<String> which contains the UTIs of the additional allowed image formats.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:104
>> +@property (nonatomic, setter=_setAdditionalAllowedImageUTIs:) NSString *_additionalAllowedImageUTIs;
> 
> Missing availability macros. Also strings need to be `copy` because they can be secretly mutable. And actually you do copy, you just don't say it here.

I added the (copy) attribute to the definition of the property. I added also availability macros: WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA);

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:214
>> +#include <WebCore/UTIRegistry.h>
> 
> Why?

Because I need to call WebCore::setAdditionalAllowedImageFormats.

>> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1203
>> +				5558482521718CDA00FB7FFF /* tga-image.png in Copy Resources */,
> 
> Why's the TGA a .png?

I did this trick to make WebKit open the image file as an ImageDocument. WebKit does not open files with TGA extension as ImageDocument. So I changed the file extension to "png" with keeping the data file as is.  The alternative solution is to open the image through an html file which contains the markup "<img src='tga-image.tga'>"
Comment 13 Said Abou-Hallawa 2018-10-14 16:02:35 PDT
Created attachment 352282 [details]
Patch
Comment 14 Said Abou-Hallawa 2018-10-14 23:29:05 PDT
Created attachment 352301 [details]
Patch
Comment 15 Said Abou-Hallawa 2018-10-15 00:06:36 PDT
Created attachment 352302 [details]
Patch
Comment 16 Tim Horton 2018-10-15 12:29:26 PDT
Comment on attachment 352302 [details]
Patch

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

We talked about how to get rid of all the preference code.

> Source/WebKit/Platform/mac/StringUtilities.mm:90
> +    [nsStringArray enumerateObjectsUsingBlock:[&] (NSString *nsString, NSUInteger, BOOL *) {
> +        stringList.append(String(nsString));
> +    }];

Why not just:

for (NSString *string in nsStringArray)
    strings.append(string);

And get rid of StringList.

You probably also want uncheckedAppend/reserveInitialCapacity.

You might even be able to find one of these somewhere already.
Comment 17 Tim Horton 2018-10-15 12:30:01 PDT
(In reply to Said Abou-Hallawa from comment #12)
> Comment on attachment 352252 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352252&action=review

> >> Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:1203
> >> +				5558482521718CDA00FB7FFF /* tga-image.png in Copy Resources */,
> > 
> > Why's the TGA a .png?
> 
> I did this trick to make WebKit open the image file as an ImageDocument.
> WebKit does not open files with TGA extension as ImageDocument. So I changed
> the file extension to "png" with keeping the data file as is.  The
> alternative solution is to open the image through an html file which
> contains the markup "<img src='tga-image.tga'>"

This is weird. If I'm allowing TGA, why aren't I allowed to ImageDocument it?
Comment 18 Said Abou-Hallawa 2018-10-15 15:34:12 PDT
Created attachment 352392 [details]
Patch
Comment 19 Said Abou-Hallawa 2018-10-19 13:01:51 PDT
Created attachment 352818 [details]
Patch
Comment 20 Tim Horton 2018-10-19 13:38:05 PDT
Comment on attachment 352818 [details]
Patch

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

> Source/WebCore/platform/MIMETypeRegistry.cpp:96
> +    for (auto& uti : additionalAllowedImageFormats()) {

Are you just getting lucky that this isn't called before you initialize additionalAllowedImageFormats()? Otherwise they'll be missing here, right?

Can you add some assertions that that's the case (maybe in the setter assert that we haven't already done initializeSupportedImageMIMETypes)?

Or make it invalidate the lists. Or dynamically add things.

> Source/WebKit/Platform/mac/StringUtilities.mm:85
> +Vector<String> webCoreStringListFromNsStringArray(NSArray<NSString *> *nsStringArray)

NS should be capitalized. Also I swear we have a version of this somewhere already.

Also Why "list" instead of "vector".
Comment 21 Said Abou-Hallawa 2018-10-19 16:57:03 PDT
Created attachment 352835 [details]
Patch
Comment 22 Said Abou-Hallawa 2018-10-19 17:01:49 PDT
Comment on attachment 352818 [details]
Patch

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

>> Source/WebCore/platform/MIMETypeRegistry.cpp:96
>> +    for (auto& uti : additionalAllowedImageFormats()) {
> 
> Are you just getting lucky that this isn't called before you initialize additionalAllowedImageFormats()? Otherwise they'll be missing here, right?
> 
> Can you add some assertions that that's the case (maybe in the setter assert that we haven't already done initializeSupportedImageMIMETypes)?
> 
> Or make it invalidate the lists. Or dynamically add things.

I dynamically add the mime types of the additionalAllowedImageFormats once they are set. But I assume they won't change even if multiple WebPages are created.

>> Source/WebKit/Platform/mac/StringUtilities.mm:85
>> +Vector<String> webCoreStringListFromNsStringArray(NSArray<NSString *> *nsStringArray)
> 
> NS should be capitalized. Also I swear we have a version of this somewhere already.
> 
> Also Why "list" instead of "vector".

Done.

I could not find an implementation for this conversions. And I found this code in TestControllerCocoa.mm

    [globalWebViewConfiguration.websiteDataStore _getAllStorageAccessEntriesFor:parentView->platformView() completionHandler:^(NSArray<NSString *> *nsDomains) {
        Vector<String> domains;
        domains.reserveInitialCapacity(nsDomains.count);
        for (NSString *domain : nsDomains)
            domains.uncheckedAppend(domain);
        m_currentInvocation->didReceiveAllStorageAccessEntries(domains);
    }];
Comment 23 Said Abou-Hallawa 2018-10-19 17:39:46 PDT
Created attachment 352840 [details]
Patch
Comment 24 Said Abou-Hallawa 2018-10-19 18:19:23 PDT
Created attachment 352845 [details]
Patch
Comment 25 Simon Fraser (smfr) 2018-10-19 21:56:43 PDT
Comment on attachment 352845 [details]
Patch

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

Seems mostly OK but let's see one more version.

> Source/WebCore/platform/MIMETypeRegistry.cpp:61
> +static HashSet<String, ASCIICaseInsensitiveHash>* supportedImageAdditionalMIMETypes;

This doesn't read well. Maybe supportedAdditionalImageMIMETypes (though the "supported" is then confusing).

> Source/WebCore/platform/MIMETypeRegistry.cpp:194
> +        supportedImageAdditionalMIMETypes = new HashSet<String, ASCIICaseInsensitiveHash>;

The modern way to do all of these would be with a static NeverDestroyed<> and a std::once in the caller.

> Source/WebCore/platform/MIMETypeRegistry.cpp:683
> +    // Reinitialize the supportedImageAdditionalMIMETypes if they have not been set or were set to an empty list.
> +    if (!supportedImageAdditionalMIMETypes || !supportedImageAdditionalMIMETypes->isEmpty())
> +        return;
> +    initializeSupportedImageAdditionalMIMETypes();

Why doesn't this just null out supportedImageAdditionalMIMETypes so that the next call triggers a rebuild? The function can be called supportedImageAdditionalMIMETypesChanged().

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:88
> +    // We are suppoed to set the additionalAllowedImageFormats only once even
> +    // if multiple WebPages are created.

It's not clear if this comment is describing a bug (in which case it should be a FIXME) or explaining the code.

> Source/WebCore/platform/graphics/cg/UTIRegistry.h:36
> +const HashSet<String>& defaultAllowedImageFormats();
> +const HashSet<String>& additionalAllowedImageFormats();
> +WEBCORE_EXPORT void setAdditionalAllowedImageFormats(const Vector<String>&);
> +bool isAllowedImageFormat(const String&);

The rename from "UTI" to "format" makes it less clear that these return UTIs.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:104
> +@property (nonatomic, copy, setter=_setAdditionalAllowedImageFormats:) NSArray<NSString *> *_additionalAllowedImageFormats WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

We might want to think about this naming. Does the caller of the API understand the "additional" part?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/AdditionalAllowedImageFormats.mm:40
> +    [configuration _setAdditionalAllowedImageFormats:@[@"com.truevision.tga-image"]];

Tests should also cover
* adding a format that is already supported
* adding a format that webkit can't render
* passing an array with the same format listed twice
Comment 26 Said Abou-Hallawa 2018-10-22 14:14:19 PDT
Created attachment 352908 [details]
Patch
Comment 27 Said Abou-Hallawa 2018-10-22 16:42:37 PDT
Created attachment 352924 [details]
Patch
Comment 28 Simon Fraser (smfr) 2018-10-22 22:50:51 PDT
Comment on attachment 352924 [details]
Patch

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

> Source/WebCore/platform/MIMETypeRegistry.cpp:62
> +    static NeverDestroyed<HashSet<String, ASCIICaseInsensitiveHash>> supportedImageMIMETypes;
> +    static std::once_flag onceFlag;
> +    std::call_once(onceFlag, [] {

This should be done in a separate patch.
Comment 29 Said Abou-Hallawa 2018-10-24 14:33:07 PDT
Created attachment 353054 [details]
Patch
Comment 30 Simon Fraser (smfr) 2018-11-08 11:36:01 PST
Comment on attachment 353054 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:104
> +@property (nonatomic, copy, setter=_setSupportedAdditionalImageSourceTypes:) NSArray<NSString *> *_supportedAdditionalImageSourceTypes WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

This name is cumbersome and doesn't make it clear that the arguments are UTIs. Maybe better as _additionalSupportedImageTypes: with a comment saying that the argument is an array of UTIs.
Comment 31 Said Abou-Hallawa 2018-11-08 14:54:49 PST
Created attachment 354278 [details]
Patch
Comment 32 EWS Watchlist 2018-11-08 14:56:38 PST
Attachment 354278 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:3621:  developmentRegion is not en.  [xcodeproj/settings] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 33 Said Abou-Hallawa 2018-11-08 14:59:27 PST
Comment on attachment 353054 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfigurationPrivate.h:104
>> +@property (nonatomic, copy, setter=_setSupportedAdditionalImageSourceTypes:) NSArray<NSString *> *_supportedAdditionalImageSourceTypes WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> This name is cumbersome and doesn't make it clear that the arguments are UTIs. Maybe better as _additionalSupportedImageTypes: with a comment saying that the argument is an array of UTIs.

I changed all the names from ImageSourceTypes to ImageTypes all the way up till the mime registry, the UTI registry and the ImageDecoderCG. And I flipped SupportedAdditional to AdditionalSupported in the names as well.
Comment 34 Said Abou-Hallawa 2018-11-08 16:21:23 PST
Created attachment 354288 [details]
Patch
Comment 35 WebKit Commit Bot 2018-11-08 19:23:25 PST
Comment on attachment 354288 [details]
Patch

Clearing flags on attachment: 354288

Committed r238015: <https://trac.webkit.org/changeset/238015>
Comment 36 WebKit Commit Bot 2018-11-08 19:23:27 PST
All reviewed patches have been landed.  Closing bug.