Reviewing merge request #453: Patch to QTBUG-3168
Modified _scheme() and parse() methods to handle wrong schemes. Method _scheme() now returns a bool that is true if the scheme is valid, and false otherwise. The boolean variable is false only if the url starts with anything other than an alpha character AND is the type of url with ':' (used in http, etc.).
- Requested by:
Carolina Simões Gomes - Status: Merged
- Source repository: carolinagomes-qt:patch-branch
- Target repository: qt:master
- Created at: January 28th, 2010 03:20
Commits that would be merged:
Comments
An autotest would be nice for this patch.
There is an autotest already, actually. In test/auto/qrl/tst_qurl.cpp, go to test case invalidSchemeValidator(). It is being skipped right now, but if you erase the first line (“return;”), the bug fix can be tested.
Then do not hesitate to remove the SKIP from the test in this patch. This show even better what you are fixing :)
I removed the SKIP from tst_qurl.cpp. Hope it’s good now :-D
+ if(first) {
According to the coding style, you need a space between the if and the parenthesis.
You should also not put brackets when the content of the if() is one line.
Benjamin Poulain |
2010-01-31 17:59:08 UTC
on src/corelib/io/qurl.cpp
I removed the SKIP from tst_qurl.cpp. Hope it’s good now :-D
Great!
Thiago would be the best to review the patch. If he does not come by, I’ll review the patch this week.
Corrected coding style.
Added tst_qurl.cpp separately.
+ isSchemeValid = false;
Wouldn’t it make sense to replace:
if (first)
isSchemeValid = false;
by
if (first) {
isSchemeValid = false;
break;
}
?
Otherwise, we still loop over the other characters even if the scheme is invalid.
Benjamin Poulain |
2010-02-02 10:54:58 UTC
on src/corelib/io/qurl.cpp
+ } else if (isSchemeValid == false || ch != '
This is not the right placed to do this check. You should add a dedicated if() after the call to
bool isSchemeValid = _scheme(ptr, &parseData);
With this test here, when the url is invalid, the error info is “expected end of URL”, which is not the error in question.
By putting the test for isSchemeValid earlier, you can stop the parsing earlier.
Benjamin Poulain |
2010-02-02 11:02:20 UTC
on src/corelib/io/qurl.cpp
There are two minor issue. Otherwise the patch looks good.
Sorry I did not see that earlier.
Changed the test for isSchemeValid as suggested.
However, looping over the characters even if the scheme is invalid makes sense. The reason is, that there are cases when the url doesn’t have a ‘:’ (ie, ‘192.168.0.1’), and in such cases the url is correct even if the scheme is supposedly invalid (since it is only invalid because the user didn’t write ‘http://192.168.0.1’, but only ‘192.168.0.1’ instead).
Small correction to the patch. Please consider all 3 commits.
+ if (isSchemeValid == false) {
maybe do?:
if (!isSchemeValid)
WebKit enforce this kind of style, Qt not really.
Benjamin Poulain |
2010-02-16 18:27:01 UTC
on src/corelib/io/qurl.cpp
Looks good. :)
I cannot do the full review now. I can do it next week if nobody else does it in the meantime. Don’t hesitate to ping me on IRC next week if I forget.
Your patch looks good. I have pushed it to Oslo-staging-1, in the 4.6 branch.
I made the following change after your patch:
1) squash the two commits
2) removing trailing white spaces
3)
diff --git a/tests/auto/qurl/tst_qurl.cpp b/tests/auto/qurl/tst_qurl.cpp
index bc5a0af..83109b5 100644
--- a/tests/auto/qurl/tst_qurl.cpp
+++ b/tests/auto/qurl/tst_qurl.cpp
@@ -2516,7 +2516,6 @@ void tst_QUrl::invalidSchemeValidator()
// test that if scheme does not start with an ALPHA, QUrl::isValid() returns false
{
QUrl url("1http://qt.nokia.com", QUrl::StrictMode);
- qDebug() << url;
QCOMPARE(url.isValid(), false);
}
{
Add a new comment:
Login or create an account to post a comment