Currently WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction considers only 'WKBundlePagePolicyActionUse' policy action returned by the InjectedBundle and skips the 'WKBundlePagePolicyActionPassThrough' result. It's leading to fast/loader/onload-policy-ignore-for-frame.html test failure. The reason for it is apparently that 'WKBundlePagePolicyActionPassThrough' is returned also from InjectedBundlePagePolicyClient when there is no WKBundlePagePolicyClient callback provided. So we do not distinguish between two cases: 1) there's no WKBundlePagePolicyClient decidePolicyForNavigationAction callback provided 2) WKBundlePagePolicyClient decidePolicyForNavigationAction callback returns 'WKBundlePagePolicyActionPassThrough' value
> It's leading to fast/loader/onload-policy-ignore-for-frame.html test failure. Actually fast/loader/onload-policy-ignore-for-frame.html will need also bug42546 to be fixed.
Created attachment 162480 [details] patch
Comment on attachment 162480 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=162480&action=review Where is the unskipped test? > Source/WebKit2/ChangeLog:11 > + Before the change WebFrameLoaderClient::dispatchDecidePolicy callbacks considered only 'WKBundlePagePolicyActionUse' policy action returned by the > + InjectedBundle and skipped the 'WKBundlePagePolicyActionPassThrough' affecting fast/loader/onload-policy-ignore-for-frame.html test output. > + > + The reason for it was apparently that 'WKBundlePagePolicyActionPassThrough' was returned also from InjectedBundlePagePolicyClient when there was no I would wrap this a bit...
(In reply to comment #3) > (From update of attachment 162480 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162480&action=review > > Where is the unskipped test? > test fast/loader/onload-policy-ignore-for-frame.html will be unskipped in bug42546 patch, I'm working at it at the moment. > > I would wrap this a bit... Could you please clarify?
Comment on attachment 162480 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=162480&action=review > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePagePolicyClient.cpp:40 > +bool InjectedBundlePagePolicyClient::decidePolicyForNavigationAction(WebPage* page, WebFrame* frame, InjectedBundleNavigationAction* action, const ResourceRequest& resourceRequest, WKBundlePagePolicyAction& result, RefPtr<APIObject>& userData) > { > if (!m_client.decidePolicyForNavigationAction) > - return WKBundlePagePolicyActionPassThrough; > + return false; This doesn't really look right to me. What's the caller to do with two separate results - a boolean and a WKBundlePagePolicyAction? Calling decidePolicyForNavigationAction must result in a decision, not in an array of vaguely defined data points the caller is advised to make its own decision from.
(In reply to comment #5) > (From update of attachment 162480 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162480&action=review > > > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePagePolicyClient.cpp:40 > > +bool InjectedBundlePagePolicyClient::decidePolicyForNavigationAction(WebPage* page, WebFrame* frame, InjectedBundleNavigationAction* action, const ResourceRequest& resourceRequest, WKBundlePagePolicyAction& result, RefPtr<APIObject>& userData) > > { > > if (!m_client.decidePolicyForNavigationAction) > > - return WKBundlePagePolicyActionPassThrough; > > + return false; > > This doesn't really look right to me. What's the caller to do with two separate results - a boolean and a WKBundlePagePolicyAction? Calling decidePolicyForNavigationAction must result in a decision, not in an array of vaguely defined data points the caller is advised to make its own decision from. boolean result just shows who actually should make the decision: injected bundle or UI process. If returns 'true' decision is taken from injected bundle otherwise from UI process.
Yes, this is exactly what looks not quite right to me.
(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 162480 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=162480&action=review > > > > > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePagePolicyClient.cpp:40 > > > +bool InjectedBundlePagePolicyClient::decidePolicyForNavigationAction(WebPage* page, WebFrame* frame, InjectedBundleNavigationAction* action, const ResourceRequest& resourceRequest, WKBundlePagePolicyAction& result, RefPtr<APIObject>& userData) > > > { > > > if (!m_client.decidePolicyForNavigationAction) > > > - return WKBundlePagePolicyActionPassThrough; > > > + return false; > > > > This doesn't really look right to me. What's the caller to do with two separate results - a boolean and a WKBundlePagePolicyAction? Calling decidePolicyForNavigationAction must result in a decision, not in an array of vaguely defined data points the caller is advised to make its own decision from. > > boolean result just shows who actually should make the decision: injected bundle or UI process. If returns 'true' decision is taken from injected bundle otherwise from UI process. That is what pass through is supposed to mean.
> > boolean result just shows who actually should make the decision: injected bundle or UI process. If returns 'true' decision is taken from injected bundle otherwise from UI process. > > That is what pass through is supposed to mean. Yes, currently 'pass through' from injected bundle means that injected bundle does not make decision and should be ignored. But in some cases this 'pass through' from injected bundle should be applied to frame loader, and fast/loader/onload-policy-ignore-for-frame.html, for instance, relies on it.
(In reply to comment #7) > Yes, this is exactly what looks not quite right to me. Yeah, my current makes the InjectedBundlePagePolicyClient interface weird, will try to find better solution.
Created attachment 162737 [details] patch v2 The proposed solution adds new method 'isNull()' to the base APIClient class. This method returns 'true' if API client was not initialized and returns 'false' otherwise. This method is then used to detect who should make the policy decision in WebFrameLoaderClient (bundle client if WKBundlePagePolicyClient is initialized and UIProcess otherwise). Also ChangeLog description improved.
Created attachment 162744 [details] patch v3 Found a problem in my previous patch: m_isNull was not reset in initialize() method. Fixed now.
Alexey, Sam could you please take a look?
Sam or Anders should review this.
Comment on attachment 162744 [details] patch v3 I don't think this is correct. "Pass through" isn't the same thing as ignore. Due to the way state is managed in the UI process, it's not possible to ignore a load from the bundle.
(In reply to comment #15) > (From update of attachment 162744 [details]) > I don't think this is correct. "Pass through" isn't the same thing as ignore. Due to the way state is managed in the UI process, it's not possible to ignore a load from the bundle. Thanks for taking a look! Yeah, my problem was probably that I treated "Pass through" as "Ignore". I guess proper fix should add "Ignore" policy for bundle client.
Created attachment 166423 [details] patch v4 Added WKBundlePagePolicyActionIgnore policy to be used by bundle client.
(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 162744 [details] [details]) > > I don't think this is correct. "Pass through" isn't the same thing as ignore. Due to the way state is managed in the UI process, it's not possible to ignore a load from the bundle. > > Thanks for taking a look! > Yeah, my problem was probably that I treated "Pass through" as "Ignore". > I guess proper fix should add "Ignore" policy for bundle client. Like I said, if we add an Ignore policy things get out of sync in the UI process.
(In reply to comment #18) > (In reply to comment #16) > > (In reply to comment #15) > > > (From update of attachment 162744 [details] [details] [details]) > > > I don't think this is correct. "Pass through" isn't the same thing as ignore. Due to the way state is managed in the UI process, it's not possible to ignore a load from the bundle. > > > > Thanks for taking a look! > > Yeah, my problem was probably that I treated "Pass through" as "Ignore". > > I guess proper fix should add "Ignore" policy for bundle client. > > Like I said, if we add an Ignore policy things get out of sync in the UI process. I'll add more to what Anders is saying. What we have here is a failing layout test. To fix it we should not add Ignore to the WebProcess. Instead we should properly implement Ignore support in the UIProcess of WebKitTestRunner.
(In reply to comment #19) > I'll add more to what Anders is saying. > > What we have here is a failing layout test. To fix it we should not add Ignore to the WebProcess. Instead we should properly implement Ignore support in the UIProcess of WebKitTestRunner. Thank you very much indeed for the hint! Changing the bug title accordingly.
Created attachment 167283 [details] patch v5 WTR bundle client notifies UI process about Custom Policy Delegate via newly added message, so that TestController is aware of whether Custom Policy Delegate as enabled and permissive. This data is used then in PlatformWebViewEfl.cpp to unskip fast/loader/onload-policy-ignore-for-frame.html for EFL WK2.
Comment on attachment 167283 [details] patch v5 I - admittedly - haven't looked at WKTR to get the answer to this myself. But could this not be done in a cross platform way? This policy delegate isn't really about platform specific ports so much as it is the cross platform UI Process of WKTR itself.
(In reply to comment #22) > (From update of attachment 167283 [details]) > I - admittedly - haven't looked at WKTR to get the answer to this myself. But could this not be done in a cross platform way? This policy delegate isn't really about platform specific ports so much as it is the cross platform UI Process of WKTR itself. It is possible to make solution cross platform if WTR Test controller sets PagePolicyClient. The reason why I did not do it from the beginning is that PagePolicyClient is usually set in port implementation and it has some logic that won't be tested if PagePolicyClient is reset by WTR. However I see that other clients (WKPageUIClient, WKPageLoaderClient) are reset in WTR and probably PagePolicyClient should not be exception. Obviously port-specific logic is supposed to be tested by port-specific API unit tests rather than by WTR. So I will change my solution. Thanks for having a look!
Created attachment 167748 [details] patch v6 Added PagePolicyClient to WTR so that solution applies to all the ports.
Could please anyone review?
Comment on attachment 167748 [details] patch v6 Clearing flags on attachment: 167748 Committed r130967: <http://trac.webkit.org/changeset/130967>
All reviewed patches have been landed. Closing bug.
(In reply to comment #26) > (From update of attachment 167748 [details]) > Clearing flags on attachment: 167748 > > Committed r130967: <http://trac.webkit.org/changeset/130967> It caused a regression on Qt-WK2 - https://bugs.webkit.org/show_bug.cgi?id=98994 Could you check and fix it, please?