[webkit-reviews] review granted: [Bug 174807] Modernize NavigationAction code : [Attachment 316339] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 174807: Modernize NavigationAction code
https://bugs.webkit.org/show_bug.cgi?id=174807

Attachment 316339: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 316339
  --> https://bugs.webkit.org/attachment.cgi?id=316339
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.