[webkit-reviews] review denied: [Bug 57850] [Qt][WK2] Qt port needs test content for QWKHistory : [Attachment 91965] Updated patch addressing review comments
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue May 3 00:29:30 PDT 2011
Benjamin Poulain <benjamin at webkit.org> has denied Jamie Cooley
<james.cooley at nokia.com>'s request for review:
Bug 57850: [Qt][WK2] Qt port needs test content for QWKHistory
https://bugs.webkit.org/show_bug.cgi?id=57850
Attachment 91965: Updated patch addressing review comments
https://bugs.webkit.org/attachment.cgi?id=91965&action=review
------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91965&action=review
> Source/WebKit2/UIProcess/API/qt/tests/html/a.htm:4
> +</HTML>
Wow, it has been a long time since I last saw tags in uppercase :)
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:21
> +#include <QDebug>
What is this for?
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:22
> +#include <QScopedPointer>
You don't seem to use this anywhere...
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:34
> + TestHistoryItem(QLatin1String title, QLatin1String url);
This is strange API, even for some testing:
1) 1 would expect ref to const since it is the convention for Qt and WebKit to
some extend
2) QLatin1String -> QString. QLatin1String() is really only useful for creation
of QString from literal
3) "url" is actually not at all an url, it is the filename
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:54
> + if (url == QLatin1String(""))
QLatin1String("") != QLatin1String() :)
There is QString.isEmpty() for this case.
>> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:67
>> + void historyForwardBackTest_data();
>
> historyForwardBackTest_data is incorrectly named. Don't use underscores in
your identifier names. [readability/naming] [4]
No worry about this.
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:76
> + QObject m_parent;
Whaaaat?
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:79
> + TestLoad = 0,
This "= 0" is needed?
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:100
> + m_testItemBlank = new TestHistoryItem(QLatin1String(""),
QLatin1String(""));
QLatin1String("") != QLatin1String(). And in this case, you could just use
QString() if the API of TestHistoryItem is changed as I commented.
The default constructor QString() is trivial, it just share a common null
implementation of QString. By using a empty literal: QString(""), one would
force passing by the decoders which is stupid. QLatin1String(""), what you do
is really create an empty litteral encoded directly with QString::fromLatin1().
This is not too bad but totally unecessary.
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:122
> + delete m_webView;
> + delete m_graphicsView;
> + delete m_testItemBlank;
> + delete m_testItemA;
> + delete m_testItemB;
> + delete m_testItemC;
> + delete m_testItemD;
I would prefer if you used smart pointers for all of that. Manual memory
management is error prone.
You included ScopedPointer, maybe an opportunity to use it :)
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:136
> + int testId = 0;
This is pretty much useless...
The string passed when creating a new test row is used when outputing the
results. It should describe what is being tested.
You passed the test number, and added comments to show what is tested. E.g.:
QTest::newRow(QString::number(testId++).toLatin1()) << int(TestLoad) <<
expectedBackList << m_testItemA << expectedForwardList << true; // [] (a) []
Instead, you should not put any comment and use the description in the data.
E.g:
QTest::newRow("[] (a) [] ") << int(TestLoad) << expectedBackList << m_testItemA
<< expectedForwardList << true;
I stop the review here, lots of comments already :). Please re-read your code
before updading.
More information about the webkit-reviews
mailing list