Bug 57850 – [Qt][WK2] Qt port needs test content for QWKHistory

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jamie Cooley]

Since there is no qtest content existing for these APIs, I will write it. In part, to address 57784.. but also because I can't really write tests for 57784 unless this is done.

[Jamie Cooley]

Created attachment 89203 [details]
Proposed added QTestLib content for qwkhistory

[WebKit Review Bot]

Attachment 89203 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:21:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:22:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:25:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:26:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:31:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:33:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:37:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:38:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:40:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:41:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:78:  historyFBTest_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 19 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jamie Cooley]

Created attachment 89216 [details]
Proposed added QTestLib content for qwkhistory

Still have these style errors, but I'm not sure I can do anything about:

<snip> tst_qwkhistory.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]

Isn't actually in my code or any code that I can see or have touched. Must come from includes that include includes?

Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:78:  historyFBTest_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

This is QTestLib's data-driven test format... If I want to use data-driven (which is a pretty useful construct for this test), you have to name it as such.

[WebKit Review Bot]

Attachment 89216 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:78:  historyFBTest_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 89216 [details]
Proposed added QTestLib content for qwkhistory

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

[Jamie Cooley]

Created attachment 91965 [details]
Updated patch addressing review comments

[WebKit Review Bot]

Attachment 91965 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:67:  historyForwardBackTest_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jamie Cooley]

(In reply to comment #7)
> Attachment 91965 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> 
> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:67:  historyForwardBackTest_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> Total errors found: 1 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Once again, a false-positive in this case. This is the format for data-driven QTest.

[Benjamin Poulain]

Comment on attachment 91965 [details]
Updated patch addressing review comments

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.

[Jamie Cooley]

Created attachment 92122 [details]
Addressed latest comments from Benjamin

[WebKit Review Bot]

Attachment 92122 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:62:  historyForwardBackTest_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jamie Cooley]

Created attachment 94611 [details]
Revised/Updated Patch. Patch addresses Benjamin comments

Revised/Updated Patch. Just forwarded the ChangeLog entry.

[WebKit Review Bot]

Attachment 94611 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:62:  historyForwardBackTest_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 94611 [details]
Revised/Updated Patch. Patch addresses Benjamin comments

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().

[Jamie Cooley]

Created attachment 94638 [details]
Updated patch addressing review comments

Addressed Benjamin's latest review comments. I have rolled the "initial" test case in as the first test case of the data-driven test rather than make a new special case for it. (It seemed cleaner this way).

I updated comments and other stuff as suggested.

[WebKit Review Bot]

Attachment 94638 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:62:  historyForwardBackTest_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 94638 [details]
Updated patch addressing review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=94638&action=review

I had a quick look, the test looks good.
Just replace all the comparison based on url().toString() to just comparison of the url directly.

You should ping reviewers when a patch waits forever. I totally forgot about this.

> Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:219
> +    QCOMPARE(testItem->url().toString(), currentItem.url().toString());

Comparing the two QUrl() would be better. Here and in a few places.

[Jamie Cooley]

Created attachment 95431 [details]
Updated patch with Benjamin's review comments. 

Updated patch with comparing URLs directly rather than toString.

[WebKit Review Bot]

Attachment 95431 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/tests/qwkhistory/tst_qwkhistory.cpp:62:  historyForwardBackTest_data is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 95431 [details]
Updated patch with Benjamin's review comments. 

Clearing flags on attachment: 95431

Committed r87727: <http://trac.webkit.org/changeset/87727>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.