Summary: | [WK2][WTR] Policy client: dumping from decidePolicyForResponse callback | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mikhail Pozdnyakov <mikhail.pozdnyakov> | ||||||||||||||||
Component: | WebKit2 | Assignee: | 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
Mikhail Pozdnyakov
2012-09-18 11:50:06 PDT
Created attachment 164599 [details]
patch
Comment on attachment 164599 [details] patch Attachment 164599 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13903222 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 on attachment 164599 [details] patch Attachment 164599 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13897251 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 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. Created attachment 164912 [details]
patch v2
Took comments from Chris and Alexey into consideration.
Created attachment 164913 [details]
patch v3
Removed also unnecessary scheme check from InjectedBundlePage::decidePolicyForResponse.
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? > Shouldn't it indicate that it is a copy?
Yes.
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. Created attachment 164975 [details]
patch v4
Kenneth, thanks for the review!
Created attachment 164976 [details]
patch v5
rebased
Comment on attachment 164976 [details] patch v5 Attachment 164976 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13935332 Created attachment 165066 [details]
patch v6
Exported WebCore::ResourceResponseBase::isAttachment() method for MAC port (to fix build on MAC).
Any review or comments? 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 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 Created attachment 165806 [details]
to be landed
Comment on attachment 165806 [details] to be landed Clearing flags on attachment: 165806 Committed r129653: <http://trac.webkit.org/changeset/129653> All reviewed patches have been landed. Closing bug. |