Summary: | [Qt] Fix http/tests/misc/redirect-to-external-url.html | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robert Hogan <robert> | ||||
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED INVALID | ||||||
Severity: | Normal | CC: | commit-queue, jwieczorek, tonikitoo | ||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | OS X 10.5 | ||||||
Bug Depends on: | 47687 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
Robert Hogan
2010-10-13 11:48:38 PDT
Created attachment 70642 [details]
Patch
> [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.
Comment on attachment 70642 [details] Patch Clearing flags on attachment: 70642 Committed r69795: <http://trac.webkit.org/changeset/69795> All reviewed patches have been landed. Closing bug. (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 :) (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. (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. === 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. |