Bug 95974

Summary:

[WK2][WTR] WebKitTestRunner UI process should be aware of Custom Policy Delegate

Product:

WebKit

Reporter:

Mikhail Pozdnyakov <mikhail.pozdnyakov>

Component:

WebKit2

Assignee:

Mikhail Pozdnyakov <mikhail.pozdnyakov>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, ap, beidson, kenneth, ossy, sam, tonikitoo, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

98994

Bug Blocks:

98527

Attachments:

Description	Flags
patch	none
patch v2	none
patch v3	andersca: review-, andersca: commit-queue-
patch v4	none
patch v5	none
patch v6	none

Mikhail Pozdnyakov

Reported 2012-09-06 04:35:11 PDT

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

Attachments
patch (11.54 KB, patch) 2012-09-06 05:16 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch v2 (7.17 KB, patch) 2012-09-07 04:26 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch v3 (7.19 KB, patch) 2012-09-07 04:44 PDT, Mikhail Pozdnyakov	andersca: review- andersca: commit-queue-	Details Formatted Diff Diff
patch v4 (7.43 KB, patch) 2012-10-01 01:21 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch v5 (12.34 KB, patch) 2012-10-05 01:51 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch v6 (11.73 KB, patch) 2012-10-09 06:29 PDT, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Mikhail Pozdnyakov

Comment 1 2012-09-06 04:40:12 PDT

> 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.

Mikhail Pozdnyakov

Comment 2 2012-09-06 05:16:10 PDT

Created attachment 162480 [details] patch

Kenneth Rohde Christiansen

Comment 3 2012-09-06 06:25:32 PDT

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...

Mikhail Pozdnyakov

Comment 4 2012-09-06 06:31:11 PDT

(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?

Alexey Proskuryakov

Comment 5 2012-09-06 10:22:10 PDT

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.

Mikhail Pozdnyakov

Comment 6 2012-09-06 11:14:52 PDT

(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.

Alexey Proskuryakov

Comment 7 2012-09-06 11:41:25 PDT

Yes, this is exactly what looks not quite right to me.

Sam Weinig

Comment 8 2012-09-06 14:21:46 PDT

(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.

Mikhail Pozdnyakov

Comment 9 2012-09-06 23:46:00 PDT

> > 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.

Mikhail Pozdnyakov

Comment 10 2012-09-07 01:41:19 PDT

(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.

Mikhail Pozdnyakov

Comment 11 2012-09-07 04:26:03 PDT

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.

Mikhail Pozdnyakov

Comment 12 2012-09-07 04:44:22 PDT

Created attachment 162744 [details] patch v3 Found a problem in my previous patch: m_isNull was not reset in initialize() method. Fixed now.

Mikhail Pozdnyakov

Comment 13 2012-09-11 11:41:56 PDT

Alexey, Sam could you please take a look?

Alexey Proskuryakov

Comment 14 2012-09-11 16:32:18 PDT

Sam or Anders should review this.

Anders Carlsson

Comment 15 2012-09-13 12:12:45 PDT

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.

Mikhail Pozdnyakov

Comment 16 2012-10-01 01:10:06 PDT

(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.

Mikhail Pozdnyakov

Comment 17 2012-10-01 01:21:44 PDT

Created attachment 166423 [details] patch v4 Added WKBundlePagePolicyActionIgnore policy to be used by bundle client.

Anders Carlsson

Comment 18 2012-10-01 10:47:31 PDT

(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.

Brady Eidson

Comment 19 2012-10-01 10:52:26 PDT

(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.

Mikhail Pozdnyakov

Comment 20 2012-10-05 01:23:36 PDT

(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.

Mikhail Pozdnyakov

Comment 21 2012-10-05 01:51:52 PDT

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.

Brady Eidson

Comment 22 2012-10-08 12:24:26 PDT

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.

Mikhail Pozdnyakov

Comment 23 2012-10-09 04:34:53 PDT

(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!

Mikhail Pozdnyakov

Comment 24 2012-10-09 06:29:06 PDT

Created attachment 167748 [details] patch v6 Added PagePolicyClient to WTR so that solution applies to all the ports.

Mikhail Pozdnyakov

Comment 25 2012-10-10 09:31:44 PDT

Could please anyone review?

WebKit Review Bot

Comment 26 2012-10-10 15:11:28 PDT

Comment on attachment 167748 [details] patch v6 Clearing flags on attachment: 167748 Committed r130967: <http://trac.webkit.org/changeset/130967>

WebKit Review Bot

Comment 27 2012-10-10 15:11:34 PDT

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

Comment 28 2012-10-11 00:32:38 PDT

(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?

Note You need to log in before you can comment on or make changes to this bug.