Bug 174807 - Modernize NavigationAction code
Summary: Modernize NavigationAction code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-24 18:55 PDT by Alex Christensen
Modified: 2017-07-25 13:47 PDT (History)
6 users (show)

See Also:


Attachments
Patch (43.56 KB, patch)
2017-07-24 18:56 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch to fix glib API build (1.54 KB, patch)
2017-07-25 04:48 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (50.33 KB, patch)
2017-07-25 09:45 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2017-07-24 18:55:16 PDT
Modernize NavigationAction code
Comment 1 Alex Christensen 2017-07-24 18:56:37 PDT
Created attachment 316339 [details]
Patch
Comment 2 Alex Christensen 2017-07-24 21:32:50 PDT
Could someone get this building on Linux?  It's probably relatively trivial, but more than EWS iterations could reasonably do.
Comment 3 Darin Adler 2017-07-24 22:46:48 PDT
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.
Comment 4 Carlos Garcia Campos 2017-07-25 04:48:20 PDT
Created attachment 316363 [details]
Patch to fix glib API build
Comment 5 Alex Christensen 2017-07-25 09:45:59 PDT
Created attachment 316367 [details]
Patch
Comment 6 Build Bot 2017-07-25 09:47:40 PDT
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
Comment 7 Alex Christensen 2017-07-25 10:06:03 PDT
> > +        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.
Comment 8 Alex Christensen 2017-07-25 10:18:08 PDT
http://trac.webkit.org/r219871
Comment 9 Alex Christensen 2017-07-25 13:47:14 PDT
http://trac.webkit.org/r219881