[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 #10 from David Quesada <david_quesada at apple.com> ---
Comment on attachment 441909
  --> https://bugs.webkit.org/attachment.cgi?id=441909
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
> + at 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
> + at 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

-- 
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/20211021/ef48500e/attachment-0001.htm>