Bug 32723

load method works fine with any url

Please follow the QtWebKit bug reporting guidelines when reporting bugs. See http://trac.webkit.org/wiki/QtWebKitBugs Specifically: - The 'QtWebKit' component should only be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit http://trac.webkit.org/wiki/QtWebKitBugs#Component - Add the keyword 'Qt' to signal that it's a Qt-related bug http://trac.webkit.org/wiki/QtWebKitBugs#Keywords

Reproduced on Linux against r55658.

Created attachment 50487 [details] Testcase

Note: this breaks kchmviewer application http://www.kchmviewer.net/

Cannot reproduce on Linux for r71415

Still reproducible in qt-4.7.1. Jenya was that revision supposed to be in 4.7.1?

No, that was on webkit trunk.

*** Bug 55370 has been marked as a duplicate of this bug. ***

Reproduced with Linux 64 bits, webkit trunk r80142 and Qt from 4.7 branch commit b31b4c46e70401745572b808c916088afb839614. Investigating the problem.

Created attachment 84489 [details] patch

Comment on attachment 84489 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=84489&action=review kenneth'd...sorry caio... > Source/WebKit/qt/ChangeLog:-4 > -- why this?

Comment on attachment 84489 [details] patch Alexis, I've checked and the patch is clean, so I'm changing to r? cq? again. The "remove line" you saw is part of the signature generated by git and not part of the patch contents. See bug 29317 ;-)

Comment on attachment 84489 [details] patch Investigating a possible regression caused by this change in one of the tests.

Comment on attachment 84489 [details] patch Caio, I think it needs tests. r- due to the lack of tests.

Please check https://bugs.webkit.org/show_bug.cgi?id=55370 and https://bugs.webkit.org/show_bug.cgi?id=54380 for further discussion on this problem and a related problem.

Created attachment 84601 [details] autotest for qt api This is not the fix, just the autotest.

Comment on attachment 84601 [details] autotest for qt api Clearing flags on attachment: 84601 Committed r80314: <http://trac.webkit.org/changeset/80314>

All reviewed patches have been landed. Closing bug.

Reopening since the bug is still there.

The patch breaks url() because it depended on the value of the URL being stored during the clearance of the frame. I'm testing a new patch that remove this dependency to complement the current patch.

Created bug 55842 to simplify logic of requestedUrl() and permit we use it for implementing our url() after a setUrl() to an invalid url. The biggest issue I'm trying to solve here is to not depend on an invalid url be stored in our document loader.

Created attachment 85471 [details] patch v2, all qt api tests pass now

Comment on attachment 85471 [details] patch v2, all qt api tests pass now This looks good (I like the inline function to clarify what is going on there). But I would like to see a bit more test coverage. Like what is in the history, what if I call setUrl with an default constructed QUrl(), what is the referer for the next request, etc.

Created attachment 85741 [details] idea of how to stress more the setUrl() -- not a real patch yet Benjamin, thanks for the review. For the existing implementation, it seems that empty QUrl() doesn't affect history. The attached new test case describes the existing behavior regarding empty QUrl(). Is this kind of test cases you had in mind? Do you have any tip on how can I track the referrer for the next request using the Qt API? I tried to "grep" for something similar in current autotests but couldn't find.

Comment on attachment 85741 [details] idea of how to stress more the setUrl() -- not a real patch yet View in context: https://bugs.webkit.org/attachment.cgi?id=85741&action=review You should also test that ::setUrl() and url() are consistent. Especially because of the check for "if (!url.isValid())" in ::url(). > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:653 > + void emptyUrls(); The name is not very representative of what you are testing.

(In reply to comment #25) > Do you have any tip on how can I track the referrer for the next request using the Qt API? I tried to "grep" for something similar in current autotests but couldn't find. I was thinking of subclassing QNetworkAccessManager, and checking the header of the QNetworkRequest.

Writing those test cases, I stepped into bug 29595, which I'm adding here as a dependency, since when fixed will get us more test cases.

Created attachment 87252 [details] patch v3, storing the url used in setter New version of the patch. After bug 29595 we have more tests to verify the behavior. This new version of the patch adds another auto test, that breaks previous idea of using requestedUrl as fallback -- since load() calls can alter the requestedUrl() without affecting the current url(). Benjamin, I've tried to get some referrer test working, by simulating a click, but even then couldn't get the information available in QNAM (just inside debug statements after the request). More investigation is needed to create such a test, but it seems to me that this could be in another bug, like "[Qt] Create autotest to verify HTTP Referrer when using setUrl() and load()" or something similar. What do you think?

Comment on attachment 87252 [details] patch v3, storing the url used in setter I would like test coverage for (1) I setUrl() twice with the same url. (2) I set the same base url but with a new fragment If that is already covered by existing test, just r? again and ping me :)

Created attachment 87611 [details] patch v4, more autotest coverage More autotest coverage. This commit adds autotests to document current behavior when we set the same URL twice. Currently setUrl() acts more like a "clear(); load()" and less like a typical setter function (that wouldn't do anything if the url to be set is the same as current).

Comment on attachment 87611 [details] patch v4, more autotest coverage View in context: https://bugs.webkit.org/attachment.cgi?id=87611&action=review > Source/WebKit/qt/Api/qwebframe.cpp:736 > +static inline void clearFrame(WebCore::Frame* frame) > +{ > + frame->loader()->activeDocumentLoader()->writer()->begin(); > + frame->loader()->activeDocumentLoader()->writer()->end(); > +} Maybe "clearCoreFrame" ? as this it in in qwebframe. > Source/WebKit/qt/Api/qwebframe_p.h:115 > + WebCore::KURL urlFromSetter; What if I call load? shouldn't this be cleared somehow? Btw, "...used when THE document URL is not YET available" > Source/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2419 > +void tst_QWebFrame::requestedUrlSetAndLoadFails() Failed?

(In reply to comment #32) > > Source/WebKit/qt/Api/qwebframe_p.h:115 > > + WebCore::KURL urlFromSetter; > > What if I call load? shouldn't this be cleared somehow? Nice catch. Thanks! Not clearing was intentional, but I was not considering this load() case. I'll work on a different version of the patch to take care of this.

Created another bug for the autotests that cover url() and friends but are not related to this patch. Bug 57865 patch contains tests that cover issues pointed by comment 30 and comment 32.

Created attachment 88509 [details] patch v5, keep a member with URL Use a member to track the URL. Both setUrl() and FrameLoaderClient callbacks will update it, the latter at the same moment that urlChanged() signal is emitted. Benjamin, rationale for KURL is in the ChangeLog. Kenneth, previous approach worked even in the case you mentioned (at least as much as I could track in the auto tests created). So no cleanup of urlFromSetter wasn't necessary ;-). However, as we discussed before, the current approach is simpler ("URL is in one well-known place"), and will be less confusing to debug in the future.

Comment on attachment 88509 [details] patch v5, keep a member with URL Looks good

The commit-queue encountered the following flaky tests while processing attachment 88509 [details]: http/tests/websocket/tests/multiple-connections.html bug 53825 (author: abarth@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 88509 [details] patch v5, keep a member with URL Clearing flags on attachment: 88509 Committed r83184: <http://trac.webkit.org/changeset/83184>

All reviewed patches have been landed. Closing bug.

This change breaks QWebFrame::url() for iframes. The urlChanged signal is never emitted, and QWebFrame::url() returns an invalid URL. This looks to be the source of the issues reported in bug 32723.

Correction, bug 82995.