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.
Created attachment 444775 [details] Patch
<rdar://problem/84311569>
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 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 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).
Created attachment 444832 [details] Patch
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 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 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 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.
*** Bug 232959 has been marked as a duplicate of this bug. ***