Instead of Policy and Loader clients.
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.
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.
(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.
(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.
Created attachment 324800 [details] Patch
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?
(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.
Alex?
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.
Created attachment 326571 [details] Patch for landing
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
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
Committed r224677: <https://trac.webkit.org/changeset/224677>