[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]

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