Bug 180294 – [Web App Manifest] Add SPI for fetching the manifest

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[David Quesada]

Add SPI for embedders to fetch the associated manifest of the current document.

[David Quesada]

rdar://problem/34747968

[David Quesada]

Created attachment 328369 [details]
Patch v1

Patch for review. This builds on 180292, so it won't apply yet.

[Geoffrey Garen]

Comment on attachment 328369 [details]
Patch v1

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

r=me with comments

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:4354
> +    _page->getApplicationManifest([completion = makeBlockPtr(completionHandler)](const std::optional<WebCore::ApplicationManifest>& manifest, WebKit::CallbackBase::Error error) {

You can just call this "completionHandler" as we do elsewhere, since the lambda creates a new lexical scope.

> Source/WebKit/UIProcess/WebPageProxy.cpp:5323
> +        // FIXME: Log error or assert.
> +        // this can validly happen if a load invalidated the callback, though

This comment contradicts itself, so you should remove it.

[David Quesada]

Created attachment 328466 [details]
Patch for landing

[Joseph Pecoraro]

Comment on attachment 328369 [details]
Patch v1

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

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:40
> +- (instancetype)initWithCoder:(NSCoder *)aDecoder
> +{
> +    [self release];

I'll admit to not being totally familiar with WebKit2's API objects, but this pattern strikes me as odd.

Could you use API::Object::constructInWrapper<...>? Does the wrapped object need to be destructed in a -dealloc to avoid leaks? My suspicion is that there can be leaks as written.

See _WKFrameHandle or _WKProcessPoolConfiguration for examples of the kinds of patterns I was expected to see.

[David Quesada]

(In reply to Joseph Pecoraro from comment #5)
> Comment on attachment 328369 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328369&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:40
> > +- (instancetype)initWithCoder:(NSCoder *)aDecoder
> > +{
> > +    [self release];
> 
> I'll admit to not being totally familiar with WebKit2's API objects, but
> this pattern strikes me as odd.
> 
> Could you use API::Object::constructInWrapper<...>? 

I'll try this.

> Does the wrapped object
> need to be destructed in a -dealloc to avoid leaks? My suspicion is that
> there can be leaks as written.

I believe I do need a dealloc. I'll put up a new patch with this.
> 
> See _WKFrameHandle or _WKProcessPoolConfiguration for examples of the kinds
> of patterns I was expected to see.

[David Quesada]

Created attachment 328531 [details]
Patch for landing (v2)

Rebased, addressed Joe's comments.

[David Quesada]

Created attachment 328590 [details]
Patch for landing v3

Attempted to fix EWS errors when the feature is disabled.

[Joseph Pecoraro]

Comment on attachment 328590 [details]
Patch for landing v3

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

Thanks for addressing the follow-up comments!

> LayoutTests/applicationmanifest/multiple-links-expected.txt:8
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x584

It is super weird to have Render Tree output on a test that doesn't need it.

You should be able to add `testRunner.dumpAsText()` to the test to avoid this unnecessary output (which you do up above with developer-warnings.html.

> LayoutTests/applicationmanifest/multiple-links.html:8
> +		testRunner.notifyDone();

Nit: Weird indent.

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:38
> +- (instancetype)initWithCoder:(NSCoder *)aDecoder

This reads much better to me now. Thanks!

> Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:130
> +- (instancetype)initWithCoder:(NSCoder *)aDecoder
> +{
> +    UNUSED_PARAM(aDecoder);
> +    return nil;
> +}

I suppose technically this won't be reached, but it might need a [self release]?

[David Quesada]

(In reply to Joseph Pecoraro from comment #9)
> Comment on attachment 328590 [details]
> Patch for landing v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328590&action=review
> 
> Thanks for addressing the follow-up comments!
> 
> > LayoutTests/applicationmanifest/multiple-links-expected.txt:8
> > +layer at (0,0) size 800x600
> > +  RenderView at (0,0) size 800x600
> > +layer at (0,0) size 800x600
> > +  RenderBlock {HTML} at (0,0) size 800x600
> > +    RenderBody {BODY} at (8,8) size 784x584
> 
> It is super weird to have Render Tree output on a test that doesn't need it.
> 
> You should be able to add `testRunner.dumpAsText()` to the test to avoid
> this unnecessary output (which you do up above with developer-warnings.html.

Sure, I'll add the dumpAsText() call. I assume that won't stop dumpResourceLoadCallbacks() from printing the resource load callbacks?

> 
> > LayoutTests/applicationmanifest/multiple-links.html:8
> > +		testRunner.notifyDone();
> 
> Nit: Weird indent.

Ugh. Tabs vs spaces! Will fix.
 
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:38
> > +- (instancetype)initWithCoder:(NSCoder *)aDecoder
> 
> This reads much better to me now. Thanks!
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKApplicationManifest.mm:130
> > +- (instancetype)initWithCoder:(NSCoder *)aDecoder
> > +{
> > +    UNUSED_PARAM(aDecoder);
> > +    return nil;
> > +}
> 
> I suppose technically this won't be reached, but it might need a [self
> release]?

Sure.

[David Quesada]

Created attachment 328608 [details]
Patch for landing v4

[Joseph Pecoraro]

Comment on attachment 328608 [details]
Patch for landing v4

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

> Tools/WebKitTestRunner/TestInvocation.cpp:1258
> +#else
> +        WKRetainPtr<WKStringRef> messageName = adoptWK(WKStringCreateWithUTF8CString("DidGetApplicationManifest"));
> +            WKPagePostMessageToInjectedBundle(TestController::singleton().mainWebView()->page(), messageName.get(), 0);
> +#endif

Nit: Weird indent on 1257.

Last comment.

This strikes me as misleading. In the non __BLOCKS__ case, this does nothing. So if another port enables APPLICATION_MANIFEST the tests may pass (if they don't dump console messages or loader delegates), but they won't actually be doing anything?

I think we may want to put a FIXME here to add an API to actually get the application manifest if !__BLOCKS__. Perhaps even a compile time error (#error), to immediately tell developers they need a provide an API to get the manifest without block.

Otherwise a port enabling this feature might not know they they need to provide an API to get things working.

[David Quesada]

Created attachment 328619 [details]
Patch for landing - v5

[WebKit Commit Bot]

Comment on attachment 328619 [details]
Patch for landing - v5

Clearing flags on attachment: 328619

Committed r225598: <https://trac.webkit.org/changeset/225598>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.