Bug 231339 – Add support for icons in web app manifest

• Component: New Bugs
• Status: RESOLVED FIXED

rdar://83844541

Created attachment 441423 [details]
Patch for review

[Brent Fulgham]

Comment on attachment 441423 [details]
Patch for review

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=231339

Please include <rdar> here.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:194
> +        logDeveloperWarning(makeString("The value of icons is not a valid list."_s));

I think you can just pass "The value of icons is not a valid list."_s

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:341
> +String ApplicationManifestParser::parseGenericStringInIcon(RefPtr<WTF::JSONImpl::Object> &manifest, const String& propertyName)

In C++, space should be after the ampersand

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.h:57
> +    String parseGenericStringInIcon(RefPtr<WTF::JSONImpl::Object> &, const String&);

No space before the ampersand.

[Brent Fulgham]

error: missing field 'icons' initializer [-Werror,-Wmissing-field-initializers]

[Brent Fulgham]

It looks like your patch won't compile, because the WebKit layer needs you to initialize the new 'icons' member. Can you add just that bit to _WKApplicationManifest.mm to avoid breaking the build?

[David Quesada]

Comment on attachment 441423 [details]
Patch for review

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

> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:45
> +    enum class Purpose {

The "Purpose" isn't really a property of the manifest itself, so I don't think "ApplicationManifest::Purpose" is the clearest name. What about "ApplicationManifest::IconPurpose" or "ApplicationManifest::Icon::Purpose" ?

> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:51
> +    struct ApplicationManifestIcon {

Since this is embedded *within* ApplicationManifest, I think just calling it "ApplicationManifest::Icon" might be a better name. The full name of "ApplicationManifest::ApplicationManifestIcon" is somewhat redundant.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:67
>      template<class Encoder> void encode(Encoder&) const;

Is there a reason to not also update ApplicationManifest::encode() and ApplicationManifest::decode() in this patch? Those methods will need to learn about `icons` in order to send the icon information from the web process to the UI process.

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:194
>> +        logDeveloperWarning(makeString("The value of icons is not a valid list."_s));
> 
> I think you can just pass "The value of icons is not a valid list."_s

I think calling it an "array" rather than "list" might be more accurate with respect to JSON terminology. (I did just check the spec and notice it uses "list" in this part of the algorithm, but maybe "array" is more familiar term to developers who might see this message.)

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198
> +    for (auto& iconValue : *arrayValue) {

Nit: I think `const auto&` might be preferred here since you're not mutating the icons.

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:216
> +                    logManifestPropertyInvalidURL("scope"_s);

Should this be "src" rather than "scope"?

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:341
>> +String ApplicationManifestParser::parseGenericStringInIcon(RefPtr<WTF::JSONImpl::Object> &manifest, const String& propertyName)
> 
> In C++, space should be after the ampersand

Can `manifest` be made const?

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.h:57
>> +    String parseGenericStringInIcon(RefPtr<WTF::JSONImpl::Object> &, const String&);
> 
> No space before the ampersand.

Why does this method take a `WTF::JSONImpl::Object` while other methods use `JSON::Object` ?

Created attachment 441632 [details]
Patch

(In reply to Brent Fulgham from comment #2)
> Comment on attachment 441423 [details]
> Patch for review
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441423&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=231339
> 
> Please include <rdar> here.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:194
> > +        logDeveloperWarning(makeString("The value of icons is not a valid list."_s));
> 
> I think you can just pass "The value of icons is not a valid list."_s
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:341
> > +String ApplicationManifestParser::parseGenericStringInIcon(RefPtr<WTF::JSONImpl::Object> &manifest, const String& propertyName)
> 
> In C++, space should be after the ampersand
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.h:57
> > +    String parseGenericStringInIcon(RefPtr<WTF::JSONImpl::Object> &, const String&);
> 
> No space before the ampersand.

Thanks for these comments. They should all be fixed in the new patch.

(In reply to David Quesada from comment #5)
> Comment on attachment 441423 [details]
> Patch for review
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441423&action=review
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:45
> > +    enum class Purpose {
> 
> The "Purpose" isn't really a property of the manifest itself, so I don't
> think "ApplicationManifest::Purpose" is the clearest name. What about
> "ApplicationManifest::IconPurpose" or "ApplicationManifest::Icon::Purpose" ?

Just changed "Purpose" to "IconPurpose".
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:51
> > +    struct ApplicationManifestIcon {
> 
> Since this is embedded *within* ApplicationManifest, I think just calling it
> "ApplicationManifest::Icon" might be a better name. The full name of
> "ApplicationManifest::ApplicationManifestIcon" is somewhat redundant.
Just changed to "ApplicationManifest::Icon"
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:67
> >      template<class Encoder> void encode(Encoder&) const;
> 
> Is there a reason to not also update ApplicationManifest::encode() and
> ApplicationManifest::decode() in this patch? Those methods will need to
> learn about `icons` in order to send the icon information from the web
> process to the UI process.
> 
I just added the `icons` to ApplicationManifest::encode() and 
ApplicationManifest::Decode.
I also added ApplicationManifest::Icon:encode() and 
ApplicationManifest::Icon::decode() because they were necessary for the 
existing functions.
> >> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:194
> >> +        logDeveloperWarning(makeString("The value of icons is not a valid list."_s));
> > 
> > I think you can just pass "The value of icons is not a valid list."_s
Just changed this.
> 
> I think calling it an "array" rather than "list" might be more accurate with
> respect to JSON terminology. (I did just check the spec and notice it uses
> "list" in this part of the algorithm, but maybe "array" is more familiar
> term to developers who might see this message.)

Also swapped the phrasing out for "array". I did take "list" from the spec originally,
but since array is used more with JSON terminology this change makes sense.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198
> > +    for (auto& iconValue : *arrayValue) {
> 
> Nit: I think `const auto&` might be preferred here since you're not mutating
> the icons.
Just changed this.
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:216
> > +                    logManifestPropertyInvalidURL("scope"_s);
> 
> Should this be "src" rather than "scope"?
Yes thank you that was a mistake.
> 
> >> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:341
> >> +String ApplicationManifestParser::parseGenericStringInIcon(RefPtr<WTF::JSONImpl::Object> &manifest, const String& propertyName)
> > 
> > In C++, space should be after the ampersand
> 
> Can `manifest` be made const?
> 
> >> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.h:57
> >> +    String parseGenericStringInIcon(RefPtr<WTF::JSONImpl::Object> &, const String&);
> > 
> > No space before the ampersand.
> 
> Why does this method take a `WTF::JSONImpl::Object` while other methods use
> `JSON::Object` ?
By casting the result of `iconValue->asObject()` in `parseIcons` to 
`const JSON::Object *` I was able to get rid of the `parseGenericStringInIcon` 
function and just call `parseGenericString`.

Created attachment 441909 [details]
Patch

[David Quesada]

Comment on attachment 441909 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Tests are added to OpenSource/Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParse.cpp

Nit: OpenSource/Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParse **r** .cpp

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:207
> +            currentIcon.src = { };

Is it valid to have an icon without a src? I would guess no since the `Processing an image resource from JSON` algorithm returns failure if the input src isn't a string or fails to parse as a URL. Should this method filter out those src-less images by replacing these `.src = {}` lines with `continue;`?

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:223
> +                    logDeveloperWarning(makeString("The icon's src url origin of \""_s, srcURLOrigin->toString(), "\" is different from the document's origin of \""_s, documentOrigin->toString(), "\"."_s));

Is this same-origin check part of the spec? This check is specified for comparing the startURL and scope, but I don't think it needs to apply to icons.

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.h:29
> +#import <wtf/cocoa/VectorCocoa.h>

Are these necessary? Unless directly related to WTF-level code, I don't think WebKit headers meant for other clients should be importing WTF headers since it is really meant to be an implementation detail of the WebKit stack. (These, of course, would be ok in _WKApplicationManifestInternal.h if you need to define methods that work with Vector for use in other WebKit-internal code.)

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.h:61
> +@property (nonatomic, readonly) NSArray *icons WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

This should be `NSArray<_WKApplicationManifestIcon *> *`. (With a `@class _WKApplicationManifestIcon;` forward declaration above)

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.h:73
> +@interface _WKApplicationManifestIcon : NSObject <NSSecureCoding>

Nit: WK_CLASS_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA))

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.h:80
> +- (void) src:(NSURL *) srcUrl;

If these setters need to be exposed, it would be better to not include these methods explicitly, but just *not* make the properties readonly. That being said, is it actually necessary to expose these setters?

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:47
> +        case _WKApplicationManifestIconPurposeMonochrome:

There are a few style checker errors here:
ERROR: Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:47:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
ERROR: Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:46:  Missing space before ( in switch(  [whitespace/parens] [5]

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

It looks like other implementations of makeVectorElement() add an -isKindOfClass: check to filter out unexpected classes. I think this function should do the same, unless there's a reason not to here. (Generally we shouldn't assume that the array we get from the NSCoder is guaranteed to be well-formed)

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:64
> +        WKPurposeToWebCorePurpose([item purpose]),

Nit: Dot syntax. item.src, item.sizes, item.type, item.purpose.

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:86
> +    NSArray *icons = [aDecoder decodeObjectOfClass:[NSArray class] forKey:@"icons"];

Nit: NSArray<_WKApplicationManifestIcon *>

And I believe this should use -decodeObjectOfClasses:forKey: with NSArray *and* _WKApplicationManifestIcon, otherwise this line would only be allowed to decode an empty array. (Or an array of empty arrays, or arrays which contain empty arrays, …)

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:188
> +- (NSArray *)icons

Nit: NSArray<_WKApplicationManifestIcon *>

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:190
> +    return nil;

Can you implement this method since you're adding it in this patch?

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:250
> +- (NSArray *)icons

Ditto: NSArray<_WKApplicationManifestIcon *>

> Tools/ChangeLog:11
> +        necessary because icons are in a list, wheras all other

Typo: wheras -> whereas

Created attachment 443585 [details]
Patch

(In reply to David Quesada from comment #10)
> Comment on attachment 441909 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441909&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Tests are added to OpenSource/Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParse.cpp
> 
> Nit: OpenSource/Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParse
> **r** .cpp
Fixed
> 
> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:207
> > +            currentIcon.src = { };
> 
> Is it valid to have an icon without a src? I would guess no since the
> `Processing an image resource from JSON` algorithm returns failure if the
> input src isn't a string or fails to parse as a URL. Should this method
> filter out those src-less images by replacing these `.src = {}` lines with
> `continue;`?
> 
Changed this from `.src = {}` to `continue;`

> > Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:223
> > +                    logDeveloperWarning(makeString("The icon's src url origin of \""_s, srcURLOrigin->toString(), "\" is different from the document's origin of \""_s, documentOrigin->toString(), "\"."_s));
> 
> Is this same-origin check part of the spec? This check is specified for
> comparing the startURL and scope, but I don't think it needs to apply to
> icons.
> 
Removed.
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.h:29
> > +#import <wtf/cocoa/VectorCocoa.h>
> 
> Are these necessary? Unless directly related to WTF-level code, I don't
> think WebKit headers meant for other clients should be importing WTF headers
> since it is really meant to be an implementation detail of the WebKit stack.
> (These, of course, would be ok in _WKApplicationManifestInternal.h if you
> need to define methods that work with Vector for use in other
> WebKit-internal code.)
> 

Moved to _WKApplicationManifestInternal.h
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.h:61
> > +@property (nonatomic, readonly) NSArray *icons WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> This should be `NSArray<_WKApplicationManifestIcon *> *`. (With a `@class
> _WKApplicationManifestIcon;` forward declaration above)
> 
Changed this.
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.h:73
> > +@interface _WKApplicationManifestIcon : NSObject <NSSecureCoding>
> 
> Nit: WK_CLASS_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA))
Added this.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.h:80
> > +- (void) src:(NSURL *) srcUrl;
> 
> If these setters need to be exposed, it would be better to not include these
> methods explicitly, but just *not* make the properties readonly. That being
> said, is it actually necessary to expose these setters?
Removed the setters.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:47
> > +        case _WKApplicationManifestIconPurposeMonochrome:
> 
> There are a few style checker errors here:
> ERROR: Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:47:  A
> case label should not be indented, but line up with its switch statement. 
> [whitespace/indent] [4]
> ERROR: Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:46: 
> Missing space before ( in switch(  [whitespace/parens] [5]
Fixed the style errors.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:59
> > +    _WKApplicationManifestIcon *item = arrayElement;
> 
> It looks like other implementations of makeVectorElement() add an
> -isKindOfClass: check to filter out unexpected classes. I think this
> function should do the same, unless there's a reason not to here. (Generally
> we shouldn't assume that the array we get from the NSCoder is guaranteed to
> be well-formed)
Added that check.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:64
> > +        WKPurposeToWebCorePurpose([item purpose]),
> 
> Nit: Dot syntax. item.src, item.sizes, item.type, item.purpose.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:86
> > +    NSArray *icons = [aDecoder decodeObjectOfClass:[NSArray class] forKey:@"icons"];
> 
> Nit: NSArray<_WKApplicationManifestIcon *>
Fixed this.
> 
> And I believe this should use -decodeObjectOfClasses:forKey: with NSArray
> *and* _WKApplicationManifestIcon, otherwise this line would only be allowed
> to decode an empty array. (Or an array of empty arrays, or arrays which
> contain empty arrays, …)
Fixed this.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:188
> > +- (NSArray *)icons
> 
> Nit: NSArray<_WKApplicationManifestIcon *>
Fixed this.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:190
> > +    return nil;
> 
> Can you implement this method since you're adding it in this patch?
I'm trying to see if I can make a first patch with really just the WebCore changes
and as little as needed in WebKit, just to not break the build.
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:250
> > +- (NSArray *)icons
> 
> Ditto: NSArray<_WKApplicationManifestIcon *>
Fixed this.
> 
> > Tools/ChangeLog:11
> > +        necessary because icons are in a list, wheras all other
> 
> Typo: wheras -> whereas
Fixed this

[Devin Rousso]

Comment on attachment 443585 [details]
Patch

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

> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:46
> +        enum class Purpose {

NIT: `enum class Purpose : uint8_t`

> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:49
> +            Any,

It seems like below this is the default value, so maybe this should be first in the list?

> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:55
> +        Vector<Purpose> purposes;

Can we make `Purpose` into a set of flags (e.g. `1 << 0`, `1 << 1`, etc.) so that we can use `OptionSet` instead?

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:180
> +    auto value = manifest.getValue("icons");

Can we name this something more descriptive?  Maybe `manifestIcons`?  You could then have `manifestIconsArray` down below :)

NIT: `"icons"_s`
NIT: I'd also move this right above `if (!value)`

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:186
> +    auto arrayValue = value->asArray();

NIT: I'd move this one line below so it's right above `if (!arrayValue)

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:195
> +        auto autoObjectValue = iconValue->asObject();

`iconObject`?

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:198
> +        const JSON::Object *objectValue = autoObjectValue.get();

I think you should `const auto& iconJSON = *iconObject;` so that you don't have to keep dereferencing below

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:238
> +                

Style: unnecessary newline

> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:246
> +                        continue;

having the `continue` here means that the following line wont run, which I don't think you want/intend

in fact, you don't really `need` the `continue` at all since this is the end of the `if` chain and there's no other logic below this anyways

Created attachment 443715 [details]
Patch

Comment on attachment 443585 [details]
Patch

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

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:46
>> +        enum class Purpose {
> 
> NIT: `enum class Purpose : uint8_t`

Added this

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:49
>> +            Any,
> 
> It seems like below this is the default value, so maybe this should be first in the list?

Any is the default, moved to the top of the list

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifest.h:55
>> +        Vector<Purpose> purposes;
> 
> Can we make `Purpose` into a set of flags (e.g. `1 << 0`, `1 << 1`, etc.) so that we can use `OptionSet` instead?

Added flags and changed `Vector<Purpose>` to `OptionSet<Purpose>`

>> Source/WebCore/Modules/applicationmanifest/ApplicationManifestParser.cpp:246
>> +                        continue;
> 
> having the `continue` here means that the following line wont run, which I don't think you want/intend
> 
> in fact, you don't really `need` the `continue` at all since this is the end of the `if` chain and there's no other logic below this anyways

Made all the changes recommended in the function.

[Devin Rousso]

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

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

Created attachment 443825 [details]
Patch

(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.

[Brent Fulgham]

Comment on attachment 443825 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParser.cpp:77
> +    ApplicationManifest parseIconFirstTopLevelProperty(const String & key, const String& value)

Extra space between "String &" here

> Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParser.cpp:83
> +    ApplicationManifest parseIconFirstTopLevelPropertyForSrc(const String & key, const String& value)

Ditto

Created attachment 443829 [details]
Patch

(In reply to Brent Fulgham from comment #19)
> Comment on attachment 443825 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443825&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParser.cpp:77
> > +    ApplicationManifest parseIconFirstTopLevelProperty(const String & key, const String& value)
> 
> Extra space between "String &" here
> 
> > Tools/TestWebKitAPI/Tests/WebCore/ApplicationManifestParser.cpp:83
> > +    ApplicationManifest parseIconFirstTopLevelPropertyForSrc(const String & key, const String& value)
> 
> Ditto
Thanks, just fixed this.

[Brent Fulgham]

Comment on attachment 443829 [details]
Patch

Looks good! r=me

[Devin Rousso]

(In reply to rginsberg from comment #18)
> (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/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.
Are you planning on getting `_WKApplicationManifest` working next?  It would be preferable to create a bug for that work and include it in a FIXME comment where there is still work that needs to be done so that it's not forgotten.

(In reply to Devin Rousso from comment #23)
> (In reply to rginsberg from comment #18)
> > (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/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.
> Are you planning on getting `_WKApplicationManifest` working next?  It would
> be preferable to create a bug for that work and include it in a FIXME
> comment where there is still work that needs to be done so that it's not
> forgotten.

There was already a radar for that, but I just created a bugzilla
(https://bugs.webkit.org/show_bug.cgi?id=232959) linked to the radar and will upload a
new patch here with // FIXME: https://bugs.webkit.org/show_bug.cgi?id=232959 in the
places where changes need to be made.

Created attachment 443842 [details]
Patch

Just added `// FIXME: https://bugs.webkit.org/show_bug.cgi?id=232959`
where changes need to be made.

[Brent Fulgham]

Will cq+ once EWS is done.

[EWS]

Traceback (most recent call last):
  File "/Volumes/Data/worker/Commit-Queue/build/Tools/Scripts/webkitpy/common/checkout/changelog.py", line 348, in parse_entries_from_file
    raise StopIteration
StopIteration

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "Tools/Scripts/webkit-patch", line 80, in <module>
    main()
  File "Tools/Scripts/webkit-patch", line 75, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/Volumes/Data/worker/Commit-Queue/build/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/Volumes/Data/worker/Commit-Queue/build/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Volumes/Data/worker/Commit-Queue/build/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/Volumes/Data/worker/Commit-Queue/build/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Volumes/Data/worker/Commit-Queue/build/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/Volumes/Data/worker/Commit-Queue/build/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 52, in run
    changelog_entry = ChangeLog(changelog_path).latest_entry()
  File "/Volumes/Data/worker/Commit-Queue/build/Tools/Scripts/webkitpy/common/checkout/changelog.py", line 377, in latest_entry
    return self.parse_latest_entry_from_file(changelog_file)
  File "/Volumes/Data/worker/Commit-Queue/build/Tools/Scripts/webkitpy/common/checkout/changelog.py", line 323, in parse_latest_entry_from_file
    return next(cls.parse_entries_from_file(changelog_file))
RuntimeError: generator raised StopIteration

Created attachment 443974 [details]
Patch

The changelog was not up to date, which I believe caused the commit bot to fail. Should be all good now.

[Brent Fulgham]

Comment on attachment 443974 [details]
Patch

r=me

[EWS]

Committed r285645 (244146@main): <https://commits.webkit.org/244146@main>

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