[webkit-reviews] review denied: [Bug 57850] [Qt][WK2] Qt port needs test content for qwkhistory : [Attachment 89216] Proposed added QTestLib content for qwkhistory
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 21 17:50:07 PDT 2011
Andreas Kling <kling 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 89216: Proposed added QTestLib content for qwkhistory
https://bugs.webkit.org/attachment.cgi?id=89216&action=review
------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=89216&action=review
Awesome! Only a minor monsoon of style issues to iron out. :)
> Source/WebKit2/ChangeLog:5
> + [Qt][WK2] Qt port needs test content for qwkhistory
qwkhistory -> QWKHistory
> Source/WebKit2/ChangeLog:8
> + Created data-driven QTestLib tests for existing qwkhistory apis.
apis -> API's
> Source/WebKit2/ChangeLog:15
> + webkit and retrieved via the qwkhistory apis.
webkit -> WebKit
> Source/WebKit2/ChangeLog:21
> + Four added canned pages
Period at end of sentence.
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:20
> +
Unnecessary newline.
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:41
> +#define NEWTESTROW(__cmd, __testItem, __waitFlag) \
> + QTest::newRow(QString::number(testId++).toLatin1()) << \
> + int(__cmd) << expBack << __testItem << expFwd << __waitFlag;
> +
> +#define HISTCOMPARETITLE(__testHistItem, __histItem) \
> + QCOMPARE(__testHistItem->title(), __histItem.title());
> +
> +#define HISTCOMPAREURL(__testHistItem, __histItem) \
> + QCOMPARE(__testHistItem->url().toString(), __histItem.url().toString());
These macros are all too trivial to justify their existence.
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:45
> + TestHistoryItem(QLatin1String, QLatin1String);
The arguments should have names, as it's not obvious which is which.
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:47
> + inline QString title() const
Pointless 'inline'.
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:52
> + inline QUrl url() const
Ditto.
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:79
> + void historyFBTest_data();
> + void historyFBTest();
We strive to avoid acronyms in WebKit code, so please call these
historyBackForward* instead (assuming FB is short for "ForwardBack".)
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:82
> + QGraphicsView * m_gv;
Coding style, asterisk(*) should follow the type immediately without
whitespace.
Also, the name 'm_gv' is overly terse, 'm_graphicsView' would be better.
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:89
> + // define testhistoryactions at -20
> + // to reserve -10 to 10 for
> + // future loadFromHistory tests
This seems unnecessary, it's not like we can't break backwards compatibility in
the autotests.
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:124
> + // m_gv->show();
We never put new commented-out code in WebKit.
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:135
> + delete(m_webView);
> + delete(m_gv);
> + delete(m_testItemBlank);
> + delete(m_testItemA);
> + delete(m_testItemB);
> + delete(m_testItemC);
> + delete(m_testItemD);
delete(x); -> delete x;
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:151
> + QList<TestHistoryItem*> expBack;
> + QList<TestHistoryItem*> expFwd;
Avoid abbreviations.
expBack -> expectedBackList
expFwd -> expectedForwardList
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:185
> + QFETCH(int, cmd);
cmd -> command
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:220
> + // Check current
current what?
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:259
> + QWKHistoryItem fwrdItem = m_history->forwardItem();
fwrdItem -> forwardItem
More information about the webkit-reviews
mailing list