Bug 187826 – Add WKNavigation/WKNavigationAction related SPI

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Add WKNavigation/WKNavigationAction related SPI

This includes:
- Exposing the WKNavigation on WKNavigationAction if there is one
- Exposing a C-API method to start a load and get back a WKNavigation

[Brady Eidson]

<rdar://problem/42387710>

[Brady Eidson]

Created attachment 345394 [details]
Patch

[Tim Horton]

Comment on attachment 345394 [details]
Patch

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

> Tools/ChangeLog:13
> [phone]  Brady Eidson  <[e-mail]>

What's going on here

[Brady Eidson]

Created attachment 345395 [details]
Patch

[Chris Dumez]

Comment on attachment 345395 [details]
Patch

r=me

[Alex Christensen]

Comment on attachment 345395 [details]
Patch

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

> Source/WebKit/UIProcess/API/APINavigationAction.h:82
> +    NavigationAction(WebKit::NavigationActionData&& navigationActionData, API::FrameInfo* sourceFrame, API::FrameInfo* targetFrame, std::optional<WTF::String> targetFrameName, WebCore::ResourceRequest&& request, const WebCore::URL& originalURL, bool shouldOpenAppLinks, RefPtr<UserInitiatedAction>&& userInitiatedAction)

Can't you use a default value instead of making another constructor?
API::Navigation* mainFrameNavigation = nullptr

[Alex Christensen]

Comment on attachment 345395 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKNavigationActionPrivate.h:59
> +@property (nonatomic, readonly) WKNavigation *_mainFrameNavigation WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

This feels messy.  Is there really no way to access this from the client?  We already tell the client about each navigation, and the client can know whether the frame is the main frame or not.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4026
> +    API::Navigation *mainFrameNavigation = frame->isMainFrame() ? navigation.get() : nullptr;

* is in the wrong place.

[Brady Eidson]

(In reply to Alex Christensen from comment #7)
> Comment on attachment 345395 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345395&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKNavigationActionPrivate.h:59
> > +@property (nonatomic, readonly) WKNavigation *_mainFrameNavigation WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> This feels messy.  Is there really no way to access this from the client? 
> We already tell the client about each navigation, and the client can know
> whether the frame is the main frame or not.

We do not tell the client about every navigation, that's actually wrong.

Regardless, at "decide policy" time the client has no way of knowing which navigation is being considered, and that ability is what this patch adds
(Note: Only main frames have WKNavigations, hence the name of the variable)

> > Source/WebKit/UIProcess/WebPageProxy.cpp:4026
> > +    API::Navigation *mainFrameNavigation = frame->isMainFrame() ? navigation.get() : nullptr;
> 
> * is in the wrong place.

Indeed will fix.

[Brady Eidson]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 345395 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345395&action=review
> 
> > Source/WebKit/UIProcess/API/APINavigationAction.h:82
> > +    NavigationAction(WebKit::NavigationActionData&& navigationActionData, API::FrameInfo* sourceFrame, API::FrameInfo* targetFrame, std::optional<WTF::String> targetFrameName, WebCore::ResourceRequest&& request, const WebCore::URL& originalURL, bool shouldOpenAppLinks, RefPtr<UserInitiatedAction>&& userInitiatedAction)
> 
> Can't you use a default value instead of making another constructor?
> API::Navigation* mainFrameNavigation = nullptr

Will do.

[Brady Eidson]

(In reply to Brady Eidson from comment #9)
> (In reply to Alex Christensen from comment #6)
> > Comment on attachment 345395 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=345395&action=review
> > 
> > > Source/WebKit/UIProcess/API/APINavigationAction.h:82
> > > +    NavigationAction(WebKit::NavigationActionData&& navigationActionData, API::FrameInfo* sourceFrame, API::FrameInfo* targetFrame, std::optional<WTF::String> targetFrameName, WebCore::ResourceRequest&& request, const WebCore::URL& originalURL, bool shouldOpenAppLinks, RefPtr<UserInitiatedAction>&& userInitiatedAction)
> > 
> > Can't you use a default value instead of making another constructor?
> > API::Navigation* mainFrameNavigation = nullptr
> 
> Will do.

Actually, no, sorry I misunderstood in the moment. I think default values to arguments to function calls are bad. :P

[Brady Eidson]

Created attachment 345416 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[EWS Watchlist]

Comment on attachment 345416 [details]
Patch

Attachment 345416 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8595681

New failing tests:
http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html

[EWS Watchlist]

Created attachment 345428 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Brady Eidson]

Layout test failure is unrelated.

[Brady Eidson]

Bizarre, it claims 32bit mac build broke but the output doesn't say why - the compilation of the file it claims failed isn't actually in the output.

Will build 32-bit locally I guess...  =/

[Brady Eidson]

(In reply to Brady Eidson from comment #16)
> Bizarre, it claims 32bit mac build broke but the output doesn't say why -
> the compilation of the file it claims failed isn't actually in the output.
> 
> Will build 32-bit locally I guess...  =/

I can't even get passed JSC locally with 32 bit.

resubmitting the patch to see if i get more helpful info

[Brady Eidson]

Created attachment 345442 [details]
Patch

[Brady Eidson]

Created attachment 345448 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 345448 [details]
Patch

Clearing flags on attachment: 345448

Committed r234052: <https://trac.webkit.org/changeset/234052>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42435157>