Bug 69832 – [Qt][WK2] Implement decidePolicyForResponse in our PolicyClient

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jesus Sanchez-Palencia]

Our PolicyClient implementation is missing decidePolicyForResponse and, therefore, can't check for a response mime type and decide whether it should download or load it.

[Jesus Sanchez-Palencia]

Created attachment 110519 [details]
Patch

[Chang Shu]

Comment on attachment 110519 [details]
Patch

It looks to me the patch is missing some code.

[Jesus Sanchez-Palencia]

Created attachment 110544 [details]
Patch

[Jesus Sanchez-Palencia]

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

[Chang Shu]

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?

[Gopal Raghavan]

This could be duplicate. There is a related patch for navigationPolicy in https://bugs.webkit.org/show_bug.cgi?id=69572.

[Jesus Sanchez-Palencia]

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.

[Jesus Sanchez-Palencia]

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

[Jesus Sanchez-Palencia]

Created attachment 110561 [details]
Patch

[Jesus Sanchez-Palencia]

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

[alan]

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)

[WebKit Review Bot]

Comment on attachment 110561 [details]
Patch

Clearing flags on attachment: 110561

Committed r97375: <http://trac.webkit.org/changeset/97375>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jesus Sanchez-Palencia]

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

[Jesus Sanchez-Palencia]

Created attachment 111069 [details]
Patch

[Jesus Sanchez-Palencia]

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

[Kenneth Rohde Christiansen]

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"

[Jesus Sanchez-Palencia]

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

[Jesus Sanchez-Palencia]

Committed r97608: <http://trac.webkit.org/changeset/97608>