[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 #12 from rginsberg at apple.com ---
(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
> > + 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)
> 
Changed this.
> > 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))
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

-- 
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/20211108/7ca17cc0/attachment-0001.htm>