[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