Bug 194190 - [CG] Enable setAdditionalSupportedImageTypes for WK1
Summary: [CG] Enable setAdditionalSupportedImageTypes for WK1
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-02 00:59 PST by Said Abou-Hallawa
Modified: 2019-02-04 16:47 PST (History)
4 users (show)

See Also:


Attachments
Patch (27.44 KB, patch)
2019-02-02 01:03 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (27.55 KB, patch)
2019-02-04 11:39 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (27.87 KB, patch)
2019-02-04 12:56 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (27.80 KB, patch)
2019-02-04 15:43 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (26.00 KB, patch)
2019-02-04 15:51 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2019-02-02 00:59:05 PST
Add a preference key for AdditionalSupportedImageTypes and transfer its value to the UTI registry when the WebPreferences is set to the WebView.
Comment 1 Said Abou-Hallawa 2019-02-02 01:03:27 PST
Created attachment 360969 [details]
Patch
Comment 2 Said Abou-Hallawa 2019-02-02 01:06:02 PST
<rdar://problem/47680862>
Comment 3 Tim Horton 2019-02-04 10:52:17 PST
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)?
Comment 4 Simon Fraser (smfr) 2019-02-04 11:18:14 PST
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
Comment 5 Said Abou-Hallawa 2019-02-04 11:39:54 PST
Created attachment 361079 [details]
Patch
Comment 6 Said Abou-Hallawa 2019-02-04 12:56:56 PST
Created attachment 361087 [details]
Patch
Comment 7 Said Abou-Hallawa 2019-02-04 13:12:03 PST
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.
Comment 8 Tim Horton 2019-02-04 13:37:39 PST
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".
Comment 9 Said Abou-Hallawa 2019-02-04 15:43:20 PST
Created attachment 361114 [details]
Patch
Comment 10 Said Abou-Hallawa 2019-02-04 15:47:06 PST
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.
Comment 11 Said Abou-Hallawa 2019-02-04 15:51:52 PST
Created attachment 361115 [details]
Patch
Comment 12 WebKit Commit Bot 2019-02-04 16:47:23 PST
Comment on attachment 361115 [details]
Patch

Clearing flags on attachment: 361115

Committed r240949: <https://trac.webkit.org/changeset/240949>
Comment 13 WebKit Commit Bot 2019-02-04 16:47:24 PST
All reviewed patches have been landed.  Closing bug.