Bug 174807 – Modernize NavigationAction code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Modernize NavigationAction code

[Alex Christensen]

Created attachment 316339 [details]
Patch

[Alex Christensen]

Could someone get this building on Linux?  It's probably relatively trivial, but more than EWS iterations could reasonably do.

[Darin Adler]

Comment on attachment 316339 [details]
Patch

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

Broken build on GTK and WPE needs to be fixed.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3660
> +        Ref<API::FrameInfo> destinationFrameInfo = API::FrameInfo::create(*frame, frameSecurityOrigin.securityOrigin());

auto?

> Source/WebKit/UIProcess/WebPageProxy.cpp:3668
> +        bool shouldOpenAppLinks = !m_shouldSuppressAppLinksInNextNavigationPolicyDecision && (destinationFrameInfo->isMainFrame()) && !hostsAreEqual(URL(ParsedURLString, m_mainFrame->url()), request.url()) && navigationActionData.navigationType != WebCore::NavigationType::BackForward;

Remove the parentheses from around (destinationFrameInfo->isMainFrame()) please.

> Source/WebKit/UIProcess/API/APINavigationAction.h:39
> +    static Ref<NavigationAction> create(WebKit::NavigationActionData&& navigationActionData, API::FrameInfo* sourceFrame, API::FrameInfo* targetFrame, WebCore::ResourceRequest&& request, const WebCore::URL& originalURL, bool shouldOpenAppLinks, RefPtr<UserInitiatedAction> userInitiatedAction)

Why is that last argument a RefPtr? It should just be a raw pointer, or maybe a RefPtr&&, but strange to have it just be RefPtr.

Should just be FrameInfo*, not API::FrameInfo*.

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

Ditto.

> Source/WebKit/UIProcess/API/APINavigationClient.h:98
> +    virtual void decidePolicyForNavigationAction(WebKit::WebPageProxy&, Ref<API::NavigationAction>&&, Ref<WebKit::WebFramePolicyListenerProxy>&& listener, API::Object*)

No need for the API:: prefixes here.

> Source/WebKit/UIProcess/API/APIUIClient.h:76
> +    virtual RefPtr<WebKit::WebPageProxy> createNewPage(WebKit::WebPageProxy*, API::FrameInfo&, WebCore::ResourceRequest&&, const WebCore::WindowFeatures&, WebKit::NavigationActionData&&) { return nullptr; }
> +    virtual void createNewPageAsync(WebKit::WebPageProxy*, API::FrameInfo&, WebCore::ResourceRequest&&, const WebCore::WindowFeatures&, WebKit::NavigationActionData&&, WTF::Function<void(RefPtr<WebKit::WebPageProxy>)>&& completionHandler) { }

No need for the API prefixes here.

Why does the completionHandler take a RefPtr? It should just take a raw pointer.

> Source/WebKit/UIProcess/Cocoa/UIDelegate.h:85
> +        RefPtr<WebKit::WebPageProxy> createNewPage(WebKit::WebPageProxy*, API::FrameInfo&, WebCore::ResourceRequest&&, const WebCore::WindowFeatures&, WebKit::NavigationActionData&&) override;
> +        void createNewPageAsync(WebKit::WebPageProxy*, API::FrameInfo&, WebCore::ResourceRequest&&, const WebCore::WindowFeatures&, WebKit::NavigationActionData&&, WTF::Function<void(RefPtr<WebKit::WebPageProxy>)>&& completionHandler) final;
> +        bool canCreateNewPageAsync() final;
> +        RefPtr<WebKit::WebPageProxy> createNewPageCommon(WebKit::WebPageProxy*, API::FrameInfo&, WebCore::ResourceRequest&&, const WebCore::WindowFeatures&, WebKit::NavigationActionData&&, WTF::Function<void(RefPtr<WebKit::WebPageProxy>)>&& completionHandler);

Seems like the completion handler functions could just take a raw pointer, no need for RefPtr.

[Carlos Garcia Campos]

Created attachment 316363 [details]
Patch to fix glib API build

[Alex Christensen]

Created attachment 316367 [details]
Patch

[Build Bot]

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

[Alex Christensen]

> > +        RefPtr<WebKit::WebPageProxy> createNewPageCommon(WebKit::WebPageProxy*, API::FrameInfo&, WebCore::ResourceRequest&&, const WebCore::WindowFeatures&, WebKit::NavigationActionData&&, WTF::Function<void(RefPtr<WebKit::WebPageProxy>)>&& completionHandler);
> 
> Seems like the completion handler functions could just take a raw pointer,
> no need for RefPtr.
I made them take a RefPtr&& instead of a raw pointer because the call sites give them the last use of a RefPtr and the caller eventually makes a RefPtr out of them.

[Alex Christensen]

http://trac.webkit.org/r219871

[Alex Christensen]

http://trac.webkit.org/r219881