[webkit-reviews] review denied: [Bug 57850] [Qt][WK2] Qt port needs test content for QWKHistory : [Attachment 94611] Revised/Updated Patch. Patch addresses Benjamin comments

[bugzilla-daemon at webkit.org]

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 94611: Revised/Updated Patch. Patch addresses Benjamin comments
https://bugs.webkit.org/attachment.cgi?id=94611&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=94611&action=review

Quick look, some little stuff.

> Source/WebKit2/ChangeLog:10
> +	   This walks through loading four simple canned pages, navigating

canned pages? We probably don't can pages the same way :)

> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:102
> +    m_page = m_webView.data()->page();

Smart pointers overload the operator->(), you don't need to call data().

> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:107
> +    QCOMPARE(m_history->backListCount(), 0);
> +    QCOMPARE(m_history->forwardListCount(), 0);
> +    QCOMPARE(m_history->count(), 0);

We don't put tests in the init functions.
Those tests make sense, you can just put then in a new test slot.

> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:197
> +	   QVERIFY(waitForSignal(m_page, SIGNAL(loadFinished(bool)), 30000));

Doesn't waitForSignal has a default time value?
This 30000 seems to come frome nowhere.

> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:209
> +    // Check current history item

Comments should be sentence, ending with a period (same with the other
comments).

> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:248
> +	   QCOMPARE(m_testItemBlank.data()->title(),

Smart pointers overload the operator->(), you don't need to call data().

> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:250
> +	   QCOMPARE(m_testItemBlank.data()->url().toString(),

Smart pointers overload the operator->(), you don't need to call data().

> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:264
> +	   QCOMPARE(m_testItemBlank.data()->title(),

Smart pointers overload the operator->(), you don't need to call data().

> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:266
> +	   QCOMPARE(m_testItemBlank.data()->url().toString(),

Smart pointers overload the operator->(), you don't need to call data().