Bug 233350 – Add support for web app manifest icons in WebKit/UI Process layer

• Component: WebKit2
• Status: RESOLVED FIXED

[Brent Fulgham]

Various icon features were added to WebCore in Bug 231339. This change threads these concepts through the WebKit layer so they can be accessed by WKWebView clients.

[Brent Fulgham]

Created attachment 444775 [details]
Patch

[Brent Fulgham]

<rdar://problem/84311569>

[Chris Dumez]

Comment on attachment 444775 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:56
> +static NSArray<NSNumber *> *fromPurposes(const OptionSet<WebCore::ApplicationManifest::Icon::Purpose>& purposes)

An OptionSet is just an integer, we should pass it by value.

Also, this function should probably return a RetainPtr<NSArray<NSNumber *>>. The call site can call .autorelease() if needed.

> Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParser.cpp:168
> +        EXPECT_TRUE(testIndex < value.size());

Could use EXPECT_LT()

[Brent Fulgham]

Comment on attachment 444775 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:56
>> +static NSArray<NSNumber *> *fromPurposes(const OptionSet<WebCore::ApplicationManifest::Icon::Purpose>& purposes)
> 
> An OptionSet is just an integer, we should pass it by value.
> 
> Also, this function should probably return a RetainPtr<NSArray<NSNumber *>>. The call site can call .autorelease() if needed.

Sounds good -- will fix!

[David Kilzer (:ddkilzer)]

Comment on attachment 444775 [details]
Patch

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

The leaks/over-releases need to be fixed before landing this.  You can do that and use Chris' r+ if desired.

>> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:56
>> +static NSArray<NSNumber *> *fromPurposes(const OptionSet<WebCore::ApplicationManifest::Icon::Purpose>& purposes)
> 
> An OptionSet is just an integer, we should pass it by value.
> 
> Also, this function should probably return a RetainPtr<NSArray<NSNumber *>>. The call site can call .autorelease() if needed.

+1 to return RetainPtr<> here to avoid the autorelease().

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:60
> +        [purposeArray addObject:[NSNumber numberWithUnsignedChar:static_cast<uint8_t>(purpose)]];

Could probably use OptionSet<WebCore::ApplicationManifest::Icon::Purpose>::StorageType  instead of uint8_t here since Purpose is also defined as a uint8_t.

Could also use std::underlying_type<WebCore::ApplicationManifest::Icon::Purpose> instead of uint8_t.

Not required to land the patch.  Also, the first pattern is not used anywhere else--I just now realized it may be possible.  The second pattern is used in a couple places.

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:69
> +    _WKApplicationManifestIcon *icon = arrayElement;

This should be:

        auto icon = dynamic_objc_cast<_WKApplicationManifestIcon>(arrayElement);
        if (!icon)
            return std::nullopt;

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:94
> +    _src = [coder decodeObjectOfClass:[NSString class] forKey:@"src"];
> +    _sizes = [coder decodeObjectOfClasses:[NSSet setWithArray:@[[NSArray class], [NSString class]]] forKey:@"sizes"];
> +    _type = [coder decodeObjectOfClass:[NSString class] forKey:@"type"];
> +    _purposes = [coder decodeObjectOfClasses:[NSSet setWithArray:@[[NSArray class], [NSString class]]] forKey:@"purposes"];

These will all be over-released once -dealloc method is fixed, or if any setter is used.  (If WebKit was built with ARC enabled, no changes would be required unless you wanted to enforce the "copy" @property attribute.)

You need to either add a -copy call to every one (to implement the @property attribute of `copy`), OR you need to use `self.src =`, `self.sizes =`, etc. to set the values.

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:105
> +        _src = icon->src;

This needs to use -copy, OR use `self.src =`.

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:108
> +        _sizes = createNSArray(icon->sizes, [] (auto& size) -> NSString * {
> +            return size;
> +        }).leakRef();

This is okay as-is.

However, if you change all of them to `self.sizes =`, you need to use .get() instead of .leakRef() here.

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:109
> +        _type = icon->type;

This needs to use -copy, OR use `self.type =`.

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:110
> +        _purposes = fromPurposes(icon->purposes);

Change to .leakRef() if `fromPurposes()` returns a RetainPtr<>, OR change to use`self.purposes =` and .get().

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:121
> +    [coder encodeObject:self.src forKey:@"src"];
> +    [coder encodeObject:self.sizes forKey:@"sizes"];
> +    [coder encodeObject:self.type forKey:@"type"];
> +    [coder encodeObject:self.purposes forKey:@"purposes"];

This will be more efficient if you use _src, _sizes, _type, _purposes here with (or without) ARC.

Without ARC, you also force each of these objects to be autoreleased by calling its getter.

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:130
> +- (void)dealloc
> +{
> +    // FIXME: https://bugs.webkit.org/show_bug.cgi?id=232959
> +    if (WebCoreObjCScheduleDeallocateOnMainRunLoop(_WKApplicationManifestIcon.class, self))
> +        return;
> +    [super dealloc];
> +}

The following code is missing and must be added to release the properties with "copy" attributes:

        [_src release];
        [_sizes release];
        [_type release];
        [_purposes release];

If you didn't fix the -init methods to properly retain the values, this would not crash most of the time, unless any setter was called to set new values.

Under ARC, none of the -release calls would be necessary.  (I'm trying to show why switching to ARC will save you a lot of grief.)

Note that the clang static analyzer (if you're patient enough to run it for all of WebKit) would catch this issue (and some of the others).

[Brent Fulgham]

Created attachment 444832 [details]
Patch

[EWS]

Committed r286073 (244460@main): <https://commits.webkit.org/244460@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444832 [details].

[Devin Rousso]

Comment on attachment 444832 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:96
> +    _purposes = [[coder decodeObjectOfClasses:[NSSet setWithArray:@[[NSArray class], [NSString class]]] forKey:@"purposes"] copy];

Should this be `@[[NSArray class], [NSNumber class]]`?

[Brent Fulgham]

Comment on attachment 444832 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:96
>> +    _purposes = [[coder decodeObjectOfClasses:[NSSet setWithArray:@[[NSArray class], [NSString class]]] forKey:@"purposes"] copy];
> 
> Should this be `@[[NSArray class], [NSNumber class]]`?

I think you are correct. I'll file a bug to correct.

[Brent Fulgham]

Comment on attachment 444832 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:96
>>> +    _purposes = [[coder decodeObjectOfClasses:[NSSet setWithArray:@[[NSArray class], [NSString class]]] forKey:@"purposes"] copy];
>> 
>> Should this be `@[[NSArray class], [NSNumber class]]`?
> 
> I think you are correct. I'll file a bug to correct.

See Bug 233602.

[Sam Sneddon [:gsnedders]]

*** Bug 232959 has been marked as a duplicate of this bug. ***