| Summary: | [Qt][WK2] Implement decidePolicyForResponse in our PolicyClient | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jesus Sanchez-Palencia <jesus> | ||||||||||
| Component: | WebKit Qt | Assignee: | Jesus Sanchez-Palencia <jesus> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | cshu, gopal.1.raghavan, webkit.review.bot, yael, zalan | ||||||||||
| Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | 70115 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Jesus Sanchez-Palencia
2011-10-11 05:44:58 PDT
Created attachment 110519 [details]
Patch
Comment on attachment 110519 [details]
Patch
It looks to me the patch is missing some code.
Created attachment 110544 [details]
Patch
(In reply to comment #2) > (From update of attachment 110519 [details]) > It looks to me the patch is missing some code. Yeah, sorry, I forgot the QtWebPageProxy piece. Here it goes... Comment on attachment 110544 [details]
Patch
The implementation in WK1 may output "Ignore" policy. Can you explain why we don't need it here?
Also, is it possible to create some test cases?
This could be duplicate. There is a related patch for navigationPolicy in https://bugs.webkit.org/show_bug.cgi?id=69572. Comment on attachment 110544 [details]
Patch
Patch is missing a Ignore for downloadable content from subframes. I will update it and upload a new version.
(In reply to comment #6) > This could be duplicate. There is a related patch for navigationPolicy in https://bugs.webkit.org/show_bug.cgi?id=69572. Gopal, this patch is implemented decidePolicyForResponse that we currently missing in our PolicyClient. This function used to be called decidePolicyForMIMEType. Created attachment 110561 [details]
Patch
(In reply to comment #5) > (From update of attachment 110544 [details]) > The implementation in WK1 may output "Ignore" policy. Can you explain why we don't need it here? This new patch is covering the Ignore cases, according to decisions made for N9's browser (https://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/blobs/master/WebKit2/UIProcess/API/qt/ClientImpl.cpp#line439) > Also, is it possible to create some test cases? There is no API directly related to this so it isn't testable directly but it will be covered when the Download API patch is reviewed and landed. I will block the other bug (#68692) with this one to keep this ongoing "relation". This is inline with N9's browser policy decision. There's no perfect way to handle non-supported subframe content AFAICS. This looks good to me. (as long as there isnt a priority use-case that overrides it) Comment on attachment 110561 [details] Patch Clearing flags on attachment: 110561 Committed r97375: <http://trac.webkit.org/changeset/97375> All reviewed patches have been landed. Closing bug. This broke QtWebKit2 trunk making it impossible to load pages. While working on it I had my Download patch (https://bugs.webkit.org/show_bug.cgi?id=68962) applied locally and I didn't realize they depended on each other. I don't believe this patch should only work with the download one, so I will investigate and clarify it. Sorry for breaking QtWK2... Created attachment 111069 [details]
Patch
(In reply to comment #15) > Created an attachment (id=111069) [details] > Patch We were dealing with an empty ResourceResponse so this patch adds the needed serialization code. It was working locally because I had this implemented on another patch... Nothing has changed on the decidePolicyForResponse implementation. Comment on attachment 111069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111069&action=review > Source/WebKit2/UIProcess/qt/ClientImpl.cpp:274 > + // It makes the browser intentionally behave differently when it comes to text(application)/xml content in subframes vs. mainframe. I think we should not write "browser" :-) maybe client? The W3C specs call it "user agent" (In reply to comment #17) > I think we should not write "browser" :-) maybe client? The W3C specs call it "user agent" Ok, Kenneth, I will fix this and land the patch. Thanks for the review! Committed r97608: <http://trac.webkit.org/changeset/97608> |