Add SPI for embedders to fetch the associated manifest of the current document.
rdar://problem/34747968
Created attachment 328369 [details] Patch v1 Patch for review. This builds on 180292, so it won't apply yet.
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.
Created attachment 328466 [details] Patch for landing
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.
(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.
Created attachment 328531 [details] Patch for landing (v2) Rebased, addressed Joe's comments.
Created attachment 328590 [details] Patch for landing v3 Attempted to fix EWS errors when the feature is disabled.
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]?
(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.
Created attachment 328608 [details] Patch for landing v4
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.
Created attachment 328619 [details] Patch for landing - v5
Comment on attachment 328619 [details] Patch for landing - v5 Clearing flags on attachment: 328619 Committed r225598: <https://trac.webkit.org/changeset/225598>
All reviewed patches have been landed. Closing bug.