Bug 178720 – [GTK][WPE] Switch to use API::NavigationClient

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

Instead of Policy and Loader clients.

[Carlos Garcia Campos]

Created attachment 324664 [details]
WIP

I've started to work on this, but I need some more feedback to finish it:

 - API::NavigationClient::decidePolicyForNavigationAction() is now used for both decidePolicyForNavigationAction and decidePolicyForNewWindowAction, but it doesn't provide the frame name. Is there any other way to get it?

 - Why API::Navigationclient::decidePolicyForNavigationResponse() receives a non-const NavigationResponse reference? Shouldn't it receive a Ref<NavigationResponse>&& instead?

 - WebPageProxy::decidePolicyForNewWindowAction() creates the API::NavigationAction using the request URL as original URL this way WTFMove(request), request.url(). I think this is using request after the move.

[Carlos Garcia Campos]

Forgot another question:

 - didDisplayInsecureContentForFrame() and didRunInsecureContentForFrame() are only provided by the LoaderClient, so I could remove the policy client implementation, but not the loader client one.

[Alex Christensen]

(In reply to Carlos Garcia Campos from comment #1)
> Created attachment 324664 [details]
> WIP
> 
> I've started to work on this, but I need some more feedback to finish it:
> 
>  - API::NavigationClient::decidePolicyForNavigationAction() is now used for
> both decidePolicyForNavigationAction and decidePolicyForNewWindowAction, but
> it doesn't provide the frame name. Is there any other way to get it?
Could we add that to API::FrameInfo?
> 
>  - Why API::Navigationclient::decidePolicyForNavigationResponse() receives a
> non-const NavigationResponse reference? Shouldn't it receive a
> Ref<NavigationResponse>&& instead?
Sure.
> 
>  - WebPageProxy::decidePolicyForNewWindowAction() creates the
> API::NavigationAction using the request URL as original URL this way
> WTFMove(request), request.url(). I think this is using request after the
> move.
Parameter evaluation order is undefined behavior.  Right now it probably works correctly on some systems.  We should move the request.url() to the previous line to make sure we use a copy of the URL.  Good catch!

(In reply to Carlos Garcia Campos from comment #2)
> Forgot another question:
> 
>  - didDisplayInsecureContentForFrame() and didRunInsecureContentForFrame()
> are only provided by the LoaderClient, so I could remove the policy client
> implementation, but not the loader client one.
If they're used, we should add them to the navigation client instead of the loader client.  If they're not used we should remove them.

[Carlos Garcia Campos]

(In reply to Alex Christensen from comment #3)
> (In reply to Carlos Garcia Campos from comment #1)
> > Created attachment 324664 [details]
> > WIP
> > 
> > I've started to work on this, but I need some more feedback to finish it:
> > 
> >  - API::NavigationClient::decidePolicyForNavigationAction() is now used for
> > both decidePolicyForNavigationAction and decidePolicyForNewWindowAction, but
> > it doesn't provide the frame name. Is there any other way to get it?
> Could we add that to API::FrameInfo?

No, at this point the frame doesn't really exist, we only have a name. I think we could add it to NavigationAction as targetFrameName().

> > 
> >  - Why API::Navigationclient::decidePolicyForNavigationResponse() receives a
> > non-const NavigationResponse reference? Shouldn't it receive a
> > Ref<NavigationResponse>&& instead?
> Sure.

I'll do it then.

> > 
> >  - WebPageProxy::decidePolicyForNewWindowAction() creates the
> > API::NavigationAction using the request URL as original URL this way
> > WTFMove(request), request.url(). I think this is using request after the
> > move.
> Parameter evaluation order is undefined behavior.  Right now it probably
> works correctly on some systems.  We should move the request.url() to the
> previous line to make sure we use a copy of the URL.  Good catch!
> 
> (In reply to Carlos Garcia Campos from comment #2)
> > Forgot another question:
> > 
> >  - didDisplayInsecureContentForFrame() and didRunInsecureContentForFrame()
> > are only provided by the LoaderClient, so I could remove the policy client
> > implementation, but not the loader client one.
> If they're used, we should add them to the navigation client instead of the
> loader client.  If they're not used we should remove them.

Yes, we expose this in our API, I'll add those methods to the navigation client then, and remove the GLib loader client.

[Carlos Garcia Campos]

Created attachment 324800 [details]
Patch

[Alex Christensen]

Comment on attachment 324800 [details]
Patch

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

> Source/WebKit/UIProcess/API/APINavigationAction.h:44
> +    static Ref<NavigationAction> create(WebKit::NavigationActionData&& navigationActionData, API::FrameInfo* sourceFrame, std::optional<WTF::String> targetFrameName, WebCore::ResourceRequest&& request, bool shouldOpenAppLinks, RefPtr<UserInitiatedAction>&& userInitiatedAction)

I think we should just have one create function.

> Source/WebKit/UIProcess/API/APINavigationAction.h:85
> +    std::optional<WTF::String> m_targetFrameName;

Could we have a null string meaning there's no frame name instead of an optional string?

[Carlos Garcia Campos]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 324800 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324800&action=review
> 
> > Source/WebKit/UIProcess/API/APINavigationAction.h:44
> > +    static Ref<NavigationAction> create(WebKit::NavigationActionData&& navigationActionData, API::FrameInfo* sourceFrame, std::optional<WTF::String> targetFrameName, WebCore::ResourceRequest&& request, bool shouldOpenAppLinks, RefPtr<UserInitiatedAction>&& userInitiatedAction)
> 
> I think we should just have one create function.

Why? targetFrame and targetFrameName are mutually exclusive.

> > Source/WebKit/UIProcess/API/APINavigationAction.h:85
> > +    std::optional<WTF::String> m_targetFrameName;
> 
> Could we have a null string meaning there's no frame name instead of an
> optional string?

Yes, sure, I guess we just need to decide which way is preferred in WebKit in general, because other reviewers have suggested the opposite in other patches.

[Carlos Garcia Campos]

Alex?

[Alex Christensen]

Comment on attachment 324800 [details]
Patch

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

These aren't very important.  The important thing is that API::NavigationClient is used.  This patch is a huge step in a good direction.

>>> Source/WebKit/UIProcess/API/APINavigationAction.h:44
>>> +    static Ref<NavigationAction> create(WebKit::NavigationActionData&& navigationActionData, API::FrameInfo* sourceFrame, std::optional<WTF::String> targetFrameName, WebCore::ResourceRequest&& request, bool shouldOpenAppLinks, RefPtr<UserInitiatedAction>&& userInitiatedAction)
>> 
>> I think we should just have one create function.
> 
> Why? targetFrame and targetFrameName are mutually exclusive.

If someone in the future is deciding which create function to use or modify, having one create function will be simpler.  They will just use nullptr/nullopt/{ } for the fields that don't apply rather than wondering what else is different.

>>> Source/WebKit/UIProcess/API/APINavigationAction.h:85
>>> +    std::optional<WTF::String> m_targetFrameName;
>> 
>> Could we have a null string meaning there's no frame name instead of an optional string?
> 
> Yes, sure, I guess we just need to decide which way is preferred in WebKit in general, because other reviewers have suggested the opposite in other patches.

I don't really care in this case.

[Carlos Garcia Campos]

Created attachment 326571 [details]
Patch for landing

[Build Bot]

Comment on attachment 326571 [details]
Patch for landing

Attachment 326571 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5176090

New failing tests:
http/tests/workers/service/service-worker-clear.html

[Build Bot]

Created attachment 326574 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Carlos Garcia Campos]

Committed r224677: <https://trac.webkit.org/changeset/224677>