[Webkit-unassigned] [Bug 231339] Add support for icons in web app manifest

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=231339

--- Comment #18 from rginsberg at apple.com ---
(In reply to Devin Rousso from comment #16)
> Comment on attachment 443715 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443715&action=review
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:252
> > +        imageResources.append(currentIcon);
> 
> Style: I'd add a newline before this
> 
> NIT: I think you can `WTFMove(currentIcon)`
Made these two changes
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.h:74
> > +
> 
> Oops?
I'm just trying to get the changes in WebCore/Modules/applicationmanifest/* made here.
I had to add a little to the _WKApplicationManifest files to not break the build.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:51
> > +    return WebCore::ApplicationManifest::Icon {
> > +    };
> 
> Oops?
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:82
> > +        Vector<WebCore::ApplicationManifest::Icon>(makeVector<WebCore::ApplicationManifest::Icon>(icons)),
> 
> you shouldn't need the cast, e.g. just
> `makeVector<WebCore::ApplicationManifest::Icon>(icons)`
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:176
> > +    return nil;
> 
> Oops?
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:269
> > +    [super dealloc];
> 
> I think this may also need to manually delete the WebCore object too,
> assuming you're gonna add an ivar that holds one.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifestInternal.h:31
> > +#import <wtf/Vector.h>
> > +#import <wtf/cocoa/VectorCocoa.h>
> 
> Are these needed?
<Wtf/cocoa/VectorCocoa.h> is needed for the `makeVector` function
I removed <wtf/Vector.h> as it was not needed.
> 
> > Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParser.cpp:375
> > +    OptionSet<ApplicationManifest::Icon::Purpose> purpose_any;
> > +    purpose_any.add(ApplicationManifest::Icon::Purpose::Any);
> 
> You can simplify this to just
> ```
>     OptionSet<ApplicationManifest::Icon::Purpose> purposeAny {
> ApplicationManifest::Icon::Purpose::Any };
> ```
> ditto for the others below too
> 
> Style: we prefer camelCase over snake_case in C++/ObjC :)
Made these changes.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211110/2999dafd/attachment.htm>