Bug 97034 – [WK2][WTR] Policy client: dumping from decidePolicyForResponse callback

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Mikhail Pozdnyakov]

WTR WKBundlePagePolicyClient should dump data from decidePolicyForResponse callback in order to unskip 
some of http/tests/download tests.

[Mikhail Pozdnyakov]

Created attachment 164599 [details]
patch

[Build Bot]

Comment on attachment 164599 [details]
patch

Attachment 164599 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13903222

[Chris Dumez]

Comment on attachment 164599 [details]
patch

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1321
> +    if (!isHTTPOrHTTPSScheme(scheme.get()))

I don't see this check in Mac port's DRT implementation. Why is it needed?

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1329
> +        const size_t position = disposition.find('=');

The suggested filename is already parsed in the resource response. I think it would be nicer to add the corresponding C API in WKURLResponse to retrieve the suggested file name from the response.

[Build Bot]

Comment on attachment 164599 [details]
patch

Attachment 164599 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13897251

[Alexey Proskuryakov]

Comment on attachment 164599 [details]
patch

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

It's surprising that linker complains. Are you using a different WebCore method for WK2 than is used in WK1?

> LayoutTests/platform/wk2/Skipped:858
> +# Bundle client is not asked to decide policy for response.

Could you elaborate? InjectedBundlePagePolicyClient::decidePolicyForResponse certainly exists. Is it not being called?

[Alexey Proskuryakov]

Comment on attachment 164599 [details]
patch

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

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1329
>> +        const size_t position = disposition.find('=');
> 
> The suggested filename is already parsed in the resource response. I think it would be nicer to add the corresponding C API in WKURLResponse to retrieve the suggested file name from the response.

It's not necessarily just parsed - on some platforms, it is produced by code outside WebKit.

Re-implementing WebCore code in WKTR is not what we should be doing to test WebCore code. We are not testing it if we have a separate implementation in the tester.

[Mikhail Pozdnyakov]

Created attachment 164912 [details]
patch v2

Took comments from Chris and Alexey into consideration.

[Mikhail Pozdnyakov]

Created attachment 164913 [details]
patch v3

Removed also unnecessary scheme check from InjectedBundlePage::decidePolicyForResponse.

[Kenneth Rohde Christiansen]

Comment on attachment 164913 [details]
patch v3

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

> Source/WebKit2/Shared/API/c/WKURLResponse.cpp:55
> +WKStringRef WKURLResponseSuggestedFilename(WKURLResponseRef responseRef)

WKURLResponseCopySuggestedFilename? Shouldn't it indicate that it is a copy?

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1321
> +        InjectedBundle::shared().stringBuilder()->append(toWTFString(WKURLResponseSuggestedFilename(response)));

isn't it leaking here?

[Alexey Proskuryakov]

> Shouldn't it indicate that it is a copy?

Yes.

[Alexey Proskuryakov]

Comment on attachment 164913 [details]
patch v3

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

r- per Kenneth's comments.

> Source/WebKit2/Shared/API/c/WKURLResponse.cpp:60
> +bool WKURLResponseIsAttachment(WKURLResponseRef responseRef)

I'm not sure if I like this API, it might be too specific. But I don't have an alternative proposal.

[Mikhail Pozdnyakov]

Created attachment 164975 [details]
patch v4

Kenneth, thanks for the review!

[Mikhail Pozdnyakov]

Created attachment 164976 [details]
patch v5

rebased

[Build Bot]

Comment on attachment 164976 [details]
patch v5

Attachment 164976 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13935332

[Mikhail Pozdnyakov]

Created attachment 165066 [details]
patch v6

Exported WebCore::ResourceResponseBase::isAttachment() method for MAC port (to fix build on MAC).

[Mikhail Pozdnyakov]

Any review or comments?

[Kenneth Rohde Christiansen]

Comment on attachment 165066 [details]
patch v6

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

> Source/WebKit2/Shared/API/c/WKURLResponse.h:45
>  WK_EXPORT WKStringRef WKURLResponseCopyMIMEType(WKURLResponseRef);
>  
>  WK_EXPORT int32_t WKURLResponseHTTPStatusCode(WKURLResponseRef);
>  
> +WK_EXPORT WKStringRef WKURLResponseCopySuggestedFilename(WKURLResponseRef);
> +
> +WK_EXPORT bool WKURLResponseIsAttachment(WKURLResponseRef);

I wonder why these are not grouped

[WebKit Review Bot]

Comment on attachment 165066 [details]
patch v6

Rejecting attachment 165066 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
tching file Source/WebKit2/Shared/API/c/WKURLResponse.h
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp
Hunk #1 FAILED at 1314.
1 out of 1 hunk FAILED -- saving rejects to file Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kenneth Ro..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14027449

[Mikhail Pozdnyakov]

Created attachment 165806 [details]
to be landed

[WebKit Review Bot]

Comment on attachment 165806 [details]
to be landed

Clearing flags on attachment: 165806

Committed r129653: <http://trac.webkit.org/changeset/129653>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.