Bug 233350 - Add support for web app manifest icons in WebKit/UI Process layer
Summary: Add support for web app manifest icons in WebKit/UI Process layer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks: 233602
  Show dependency treegraph
 
Reported: 2021-11-18 19:38 PST by Brent Fulgham
Modified: 2021-11-29 15:43 PST (History)
7 users (show)

See Also:


Attachments
Patch (21.25 KB, patch)
2021-11-18 19:57 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (21.51 KB, patch)
2021-11-19 10:22 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2021-11-18 19:38:24 PST
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.
Comment 1 Brent Fulgham 2021-11-18 19:57:35 PST
Created attachment 444775 [details]
Patch
Comment 2 Brent Fulgham 2021-11-18 19:58:28 PST
<rdar://problem/84311569>
Comment 3 Chris Dumez 2021-11-19 07:27:12 PST
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()
Comment 4 Brent Fulgham 2021-11-19 09:24:17 PST
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!
Comment 5 David Kilzer (:ddkilzer) 2021-11-19 09:27:57 PST
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).
Comment 6 Brent Fulgham 2021-11-19 10:22:42 PST
Created attachment 444832 [details]
Patch
Comment 7 EWS 2021-11-19 13:21:02 PST
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].
Comment 8 Devin Rousso 2021-11-29 15:23:37 PST
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]]`?
Comment 9 Brent Fulgham 2021-11-29 15:39:34 PST
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.
Comment 10 Brent Fulgham 2021-11-29 15:43:41 PST
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.