Bug 194190 – [CG] Enable setAdditionalSupportedImageTypes for WK1

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

[Said Abou-Hallawa]

Add a preference key for AdditionalSupportedImageTypes and transfer its value to the UTI registry when the WebPreferences is set to the WebView.

[Said Abou-Hallawa]

Created attachment 360969 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/47680862>

[Tim Horton]

Comment on attachment 360969 [details]
Patch

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

> Source/WebKitLegacy/mac/WebView/WebPreferences.h:208
> +/*!
> + @property additionalSupportedImageTypes
> + */
> +@property (nonatomic, retain) NSArray<NSString *> *additionalSupportedImageTypes;
> +

Shouldn't this be SPI? (in a private header, with underscores probably?)

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:459
> +        @[@""],                         WebKitAdditionalSupportedImageTypesKey,

Shouldn't the default probably be an empty array, not an array with one empty string in it?

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:1186
> +    [self _setStringArrayValueForKey: imageTypes forKey: WebKitAdditionalSupportedImageTypesKey];

I see you're just following surrounding code but we don't put spaces after colons.

> Source/WebKitLegacy/mac/WebView/WebView.mm:2870
> +#if PLATFORM(MAC)

Why PLATFORM(MAC)?

[Simon Fraser (smfr)]

Comment on attachment 360969 [details]
Patch

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

> Source/WebKitLegacy/mac/WebView/WebPreferences.h:206
> +/*!
> + @property additionalSupportedImageTypes
> + */

More words please. What is this list? MIME types? UTIs?

>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:459
>> +        @[@""],                         WebKitAdditionalSupportedImageTypesKey,
> 
> Shouldn't the default probably be an empty array, not an array with one empty string in it?

Why does the key need to be in the list at all?

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:763
> +    id s = [self _valueForKey:key];

better name for 's' please.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:764
> +    return [s isKindOfClass:[NSArray<NSString *> class]] ? (NSArray<NSString *> *)s : nil;

This doesn't do what you expect. isKindOfClass:[NSArray<NSString *> class] does NOT check the types of the objects in the array.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:769
> +    NSString *_key = KEY(key);

What does that do?

> Tools/TestWebKitAPI/Tests/mac/AdditionalSupportedImageTypes.mm:56
> +    [preferences setAdditionalSupportedImageTypes:@[@"com.truevision.tga-image"]];

You should also test:
* empty array
* array with [NSNull null]
* array with things that are not strings
* array with things that are not UTIs

[Said Abou-Hallawa]

Created attachment 361079 [details]
Patch

[Said Abou-Hallawa]

Created attachment 361087 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 360969 [details]
Patch

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

>> Source/WebKitLegacy/mac/WebView/WebPreferences.h:206
>> + */
> 
> More words please. What is this list? MIME types? UTIs?

I added the following comment before the property:

// The input of this SPI is an array of image UTI (Uniform Type Identifier).

>> Source/WebKitLegacy/mac/WebView/WebPreferences.h:208
>> +
> 
> Shouldn't this be SPI? (in a private header, with underscores probably?)

The property is moved to WebPreferencesPrivate.h

>>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:459
>>> +        @[@""],                         WebKitAdditionalSupportedImageTypesKey,
>> 
>> Shouldn't the default probably be an empty array, not an array with one empty string in it?
> 
> Why does the key need to be in the list at all?

Yes there is no need to specify a default value for this property. So I removed this entry in the default dictionary.

>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:763
>> +    id s = [self _valueForKey:key];
> 
> better name for 's' please.

I renamed it to "value"

>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:764
>> +    return [s isKindOfClass:[NSArray<NSString *> class]] ? (NSArray<NSString *> *)s : nil;
> 
> This doesn't do what you expect. isKindOfClass:[NSArray<NSString *> class] does NOT check the types of the objects in the array.

Yes you right. I added two checks. The first one checks whether the value is an NSArray or not. The second check loops through the entries in this array and checks whether the entry is an NSString or not.

>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:769
>> +    NSString *_key = KEY(key);
> 
> What does that do?

If the preferences was initWithIdentifier, all the keys will be prefixed by that identifier. KEY is defined to be the macro:

#define KEY(x) (_private->identifier ? [_private->identifier.get() stringByAppendingString:(x)] : (x))

>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:1186
>> +    [self _setStringArrayValueForKey: imageTypes forKey: WebKitAdditionalSupportedImageTypesKey];
> 
> I see you're just following surrounding code but we don't put spaces after colons.

Spaces were removed.

>> Source/WebKitLegacy/mac/WebView/WebView.mm:2870
>> +#if PLATFORM(MAC)
> 
> Why PLATFORM(MAC)?

PLATFORM(MAC) was removed since by default the code in this source file is enabled for macOS and iOS.

>> Tools/TestWebKitAPI/Tests/mac/AdditionalSupportedImageTypes.mm:56
>> +    [preferences setAdditionalSupportedImageTypes:@[@"com.truevision.tga-image"]];
> 
> You should also test:
> * empty array
> * array with [NSNull null]
> * array with things that are not strings
> * array with things that are not UTIs

Tests were added.

[Tim Horton]

Comment on attachment 361087 [details]
Patch

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

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:1192
> +- (void)setAdditionalSupportedImageTypes: (NSArray<NSString *> *)imageTypes

:<no space>(

Also, it's funny that you're being so careful with the type checks when reading out of prefs but not when writing to them.

> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:629
> +// The input of this SPI is an array of image UTI (Uniform Type Identifier).

I would leave off "the input of this SPI". Maybe just "additionalSupportedImageTypes is an array of image UTIs".

[Said Abou-Hallawa]

Created attachment 361114 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 361087 [details]
Patch

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

>> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:1192
>> +- (void)setAdditionalSupportedImageTypes: (NSArray<NSString *> *)imageTypes
> 
> :<no space>(
> 
> Also, it's funny that you're being so careful with the type checks when reading out of prefs but not when writing to them.

The space was removed.

I just followed what other functions, in the same file, are doing. For example _setIntegerValue() sets the value without checking whether it can be an integer or not. But _integerValueForKey() returns 0 if the value of the key is not an integer.

>> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:629
>> +// The input of this SPI is an array of image UTI (Uniform Type Identifier).
> 
> I would leave off "the input of this SPI". Maybe just "additionalSupportedImageTypes is an array of image UTIs".

The comment was changed.

[Said Abou-Hallawa]

Created attachment 361115 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 361115 [details]
Patch

Clearing flags on attachment: 361115

Committed r240949: <https://trac.webkit.org/changeset/240949>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.