[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 #16 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 443715
  --> https://bugs.webkit.org/attachment.cgi?id=443715
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)`

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.h:74
> +

Oops?

> 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?

> 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 :)

-- 
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/20211109/17fc201c/attachment-0001.htm>