Bug 97034

Summary: [WK2][WTR] Policy client: dumping from decidePolicyForResponse callback
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: WebKit2Assignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, gyuyoung.kim, kenneth, rakuco, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
ap: review-, buildbot: commit-queue-
patch v2
none
patch v3
ap: review-, ap: commit-queue-
patch v4
none
patch v5
buildbot: commit-queue-
patch v6
kenneth: review+, webkit.review.bot: commit-queue-
to be landed none

Description Mikhail Pozdnyakov 2012-09-18 11:50:06 PDT
WTR WKBundlePagePolicyClient should dump data from decidePolicyForResponse callback in order to unskip 
some of http/tests/download tests.
Comment 1 Mikhail Pozdnyakov 2012-09-18 11:59:50 PDT
Created attachment 164599 [details]
patch
Comment 2 Build Bot 2012-09-18 12:13:09 PDT
Comment on attachment 164599 [details]
patch

Attachment 164599 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13903222
Comment 3 Chris Dumez 2012-09-18 12:24:42 PDT
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.
Comment 4 Build Bot 2012-09-18 12:25:29 PDT
Comment on attachment 164599 [details]
patch

Attachment 164599 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13897251
Comment 5 Alexey Proskuryakov 2012-09-18 14:01:02 PDT
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?
Comment 6 Alexey Proskuryakov 2012-09-18 14:03:17 PDT
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.
Comment 7 Mikhail Pozdnyakov 2012-09-20 07:01:39 PDT
Created attachment 164912 [details]
patch v2

Took comments from Chris and Alexey into consideration.
Comment 8 Mikhail Pozdnyakov 2012-09-20 07:10:43 PDT
Created attachment 164913 [details]
patch v3

Removed also unnecessary scheme check from InjectedBundlePage::decidePolicyForResponse.
Comment 9 Kenneth Rohde Christiansen 2012-09-20 07:50:02 PDT
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?
Comment 10 Alexey Proskuryakov 2012-09-20 08:23:33 PDT
> Shouldn't it indicate that it is a copy?

Yes.
Comment 11 Alexey Proskuryakov 2012-09-20 08:26:49 PDT
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.
Comment 12 Mikhail Pozdnyakov 2012-09-20 13:43:45 PDT
Created attachment 164975 [details]
patch v4

Kenneth, thanks for the review!
Comment 13 Mikhail Pozdnyakov 2012-09-20 13:51:38 PDT
Created attachment 164976 [details]
patch v5

rebased
Comment 14 Build Bot 2012-09-20 15:59:53 PDT
Comment on attachment 164976 [details]
patch v5

Attachment 164976 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13935332
Comment 15 Mikhail Pozdnyakov 2012-09-21 00:23:09 PDT
Created attachment 165066 [details]
patch v6

Exported WebCore::ResourceResponseBase::isAttachment() method for MAC port (to fix build on MAC).
Comment 16 Mikhail Pozdnyakov 2012-09-25 00:09:49 PDT
Any review or comments?
Comment 17 Kenneth Rohde Christiansen 2012-09-26 02:41:35 PDT
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
Comment 18 WebKit Review Bot 2012-09-26 02:47:40 PDT
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
Comment 19 Mikhail Pozdnyakov 2012-09-26 08:20:24 PDT
Created attachment 165806 [details]
to be landed
Comment 20 WebKit Review Bot 2012-09-26 09:12:36 PDT
Comment on attachment 165806 [details]
to be landed

Clearing flags on attachment: 165806

Committed r129653: <http://trac.webkit.org/changeset/129653>
Comment 21 WebKit Review Bot 2012-09-26 09:12:43 PDT
All reviewed patches have been landed.  Closing bug.