Bug 47609 – [Qt] Fix http/tests/misc/redirect-to-external-url.html

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit Qt

[Robert Hogan]

Support the test in FrameLoaderClientQt

[Robert Hogan]

Created attachment 70642 [details]
Patch

[Jakub Wieczorek]

> [Qt] Fix http/tests/misc/redirect-to-external-url.html

Nah, you're not fixing it, really.
I recommend looking at what other ports are doing but special-casing a specific situation from a test case inside WebKit is not a solution at all. Not to mention that theoretically each scheme can be supported by the embedder via an extended QNAM.

If this patch was OK, we could go ahead and reduce the failing test list to 0 with similar changes, but in real-world situations QtWebKit would still not work as expected and you'd not be aware of that due to the tests happily passing.

[WebKit Commit Bot]

Comment on attachment 70642 [details]
Patch

Clearing flags on attachment: 70642

Committed r69795: <http://trac.webkit.org/changeset/69795>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

(In reply to comment #2)
> > [Qt] Fix http/tests/misc/redirect-to-external-url.html
> 
> Nah, you're not fixing it, really.
> I recommend looking at what other ports are doing but special-casing a specific situation from a test case inside WebKit is not a solution at all. Not to mention that theoretically each scheme can be supported by the embedder via an extended QNAM.
> 
> If this patch was OK, we could go ahead and reduce the failing test list to 0 with similar changes, but in real-world situations QtWebKit would still not work as expected and you'd not be aware of that due to the tests happily passing.

Oh. should we roll it out?

it is one command on IRC :)

[Robert Hogan]

(In reply to comment #2)
> > [Qt] Fix http/tests/misc/redirect-to-external-url.html
> 
> Nah, you're not fixing it, really.
> I recommend looking at what other ports are doing but special-casing a specific situation from a test case inside WebKit is not a solution at all. Not to mention that theoretically each scheme can be supported by the embedder via an extended QNAM.

Chromium special-cases it. My understanding is that the test is for WebCore functionality, PolicyChecker::continueAfterNavigationPolicy(PolicyAction policy and the canHandleRequest() is expected to fail.

I agree that it would be better to remove the DRT special-casing we have in FrameLoaderClientQt, along with all the other DRT specific messages in there. But that's a different problem.

[Jakub Wieczorek]

(In reply to comment #6)
> (In reply to comment #2)
> > > [Qt] Fix http/tests/misc/redirect-to-external-url.html
> > 
> > Nah, you're not fixing it, really.
> > I recommend looking at what other ports are doing but special-casing a specific situation from a test case inside WebKit is not a solution at all. Not to mention that theoretically each scheme can be supported by the embedder via an extended QNAM.
> 
> Chromium special-cases it. My understanding is that the test is for WebCore functionality, PolicyChecker::continueAfterNavigationPolicy(PolicyAction policy and the canHandleRequest() is expected to fail.

OK. I don't think you should follow Chromium in this case though. Other ports don't seem to do this and yet, they pass this test, so apparently they can get the necessary information from the network stack. QtWebKit, due to its abstract network interface, cannot.

Moreover, if a specific behavior of canHandleRequest() is an assumption in this test case and yet that behavior never occurs in QtWebKit, what's the point of testing this anyway?

I think all this test deserves is a good comment in the skipped list.

> I agree that it would be better to remove the DRT special-casing we have in FrameLoaderClientQt, along with all the other DRT specific messages in there. But that's a different problem.

Yes. I'd suggest we don't make the situation even worse though.

[Antonio Gomes]

Rolled out by bug 47687.

[Jocelyn Turcotte]

=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.