Bug 180294

Summary: [Web App Manifest] Add SPI for fetching the manifest
Product: WebKit Reporter: David Quesada <david_quesada>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dbates, ews-watchlist, ggaren, joepeck, mkwst, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch v1
ggaren: review+
Patch for landing
none
Patch for landing (v2)
none
Patch for landing v3
none
Patch for landing v4
none
Patch for landing - v5 none

Description David Quesada 2017-12-01 16:20:11 PST
Add SPI for embedders to fetch the associated manifest of the current document.
Comment 1 David Quesada 2017-12-01 16:21:21 PST
rdar://problem/34747968
Comment 2 David Quesada 2017-12-04 11:59:06 PST
Created attachment 328369 [details]
Patch v1

Patch for review. This builds on 180292, so it won't apply yet.
Comment 3 Geoffrey Garen 2017-12-04 15:37:39 PST
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.
Comment 4 David Quesada 2017-12-05 10:32:37 PST
Created attachment 328466 [details]
Patch for landing
Comment 5 Joseph Pecoraro 2017-12-05 14:27:37 PST
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.
Comment 6 David Quesada 2017-12-05 14:56:08 PST
(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.
Comment 7 David Quesada 2017-12-05 17:15:10 PST
Created attachment 328531 [details]
Patch for landing (v2)

Rebased, addressed Joe's comments.
Comment 8 David Quesada 2017-12-06 10:54:37 PST
Created attachment 328590 [details]
Patch for landing v3

Attempted to fix EWS errors when the feature is disabled.
Comment 9 Joseph Pecoraro 2017-12-06 11:56:10 PST
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]?
Comment 10 David Quesada 2017-12-06 12:11:04 PST
(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.
Comment 11 David Quesada 2017-12-06 12:39:09 PST
Created attachment 328608 [details]
Patch for landing v4
Comment 12 Joseph Pecoraro 2017-12-06 13:14:25 PST
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.
Comment 13 David Quesada 2017-12-06 13:53:57 PST
Created attachment 328619 [details]
Patch for landing - v5
Comment 14 WebKit Commit Bot 2017-12-06 14:26:37 PST
Comment on attachment 328619 [details]
Patch for landing - v5

Clearing flags on attachment: 328619

Committed r225598: <https://trac.webkit.org/changeset/225598>
Comment 15 WebKit Commit Bot 2017-12-06 14:26:38 PST
All reviewed patches have been landed.  Closing bug.