[webkit-reviews] review denied: [Bug 194190] [CG] Enable setAdditionalSupportedImageTypes for WK1 : [Attachment 360969] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 194190: [CG] Enable setAdditionalSupportedImageTypes for WK1
https://bugs.webkit.org/show_bug.cgi?id=194190

Attachment 360969: Patch

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




--- Comment #4 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 360969
  --> https://bugs.webkit.org/attachment.cgi?id=360969
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