Bug 57850 - [Qt][WK2] Qt port needs test content for QWKHistory
Summary: [Qt][WK2] Qt port needs test content for QWKHistory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jamie Cooley
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 57784
  Show dependency treegraph
 
Reported: 2011-04-05 08:08 PDT by Jamie Cooley
Modified: 2011-05-31 08:38 PDT (History)
4 users (show)

See Also:


Attachments
Proposed added QTestLib content for qwkhistory (13.12 KB, patch)
2011-04-12 08:06 PDT, Jamie Cooley
no flags Details | Formatted Diff | Diff
Proposed added QTestLib content for qwkhistory (13.20 KB, patch)
2011-04-12 09:29 PDT, Jamie Cooley
kling: review-
Details | Formatted Diff | Diff
Updated patch addressing review comments (14.98 KB, patch)
2011-05-02 13:28 PDT, Jamie Cooley
benjamin: review-
Details | Formatted Diff | Diff
Addressed latest comments from Benjamin (14.46 KB, patch)
2011-05-03 13:54 PDT, Jamie Cooley
no flags Details | Formatted Diff | Diff
Revised/Updated Patch. Patch addresses Benjamin comments (14.47 KB, patch)
2011-05-24 07:53 PDT, Jamie Cooley
benjamin: review-
Details | Formatted Diff | Diff
Updated patch addressing review comments (15.38 KB, patch)
2011-05-24 10:23 PDT, Jamie Cooley
benjamin: review+
benjamin: commit-queue-
Details | Formatted Diff | Diff
Updated patch with Benjamin's review comments. (15.20 KB, patch)
2011-05-31 07:25 PDT, Jamie Cooley
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jamie Cooley 2011-04-05 08:08:15 PDT
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.
Comment 1 Jamie Cooley 2011-04-12 08:06:33 PDT
Created attachment 89203 [details]
Proposed added QTestLib content for qwkhistory
Comment 2 WebKit Review Bot 2011-04-12 08:08:30 PDT
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.
Comment 3 Jamie Cooley 2011-04-12 09:29:29 PDT
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.
Comment 4 WebKit Review Bot 2011-04-12 09:34:17 PDT
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.
Comment 5 Andreas Kling 2011-04-21 17:50:07 PDT
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
Comment 6 Jamie Cooley 2011-05-02 13:28:28 PDT
Created attachment 91965 [details]
Updated patch addressing review comments
Comment 7 WebKit Review Bot 2011-05-02 13:30:37 PDT
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.
Comment 8 Jamie Cooley 2011-05-02 13:38:24 PDT
(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.
Comment 9 Benjamin Poulain 2011-05-03 00:29:30 PDT
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.
Comment 10 Jamie Cooley 2011-05-03 13:54:24 PDT
Created attachment 92122 [details]
Addressed latest comments from Benjamin
Comment 11 WebKit Review Bot 2011-05-03 13:57:51 PDT
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.
Comment 12 Jamie Cooley 2011-05-24 07:53:39 PDT
Created attachment 94611 [details]
Revised/Updated Patch. Patch addresses Benjamin comments

Revised/Updated Patch. Just forwarded the ChangeLog entry.
Comment 13 WebKit Review Bot 2011-05-24 07:56:02 PDT
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.
Comment 14 Benjamin Poulain 2011-05-24 08:35:28 PDT
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().
Comment 15 Jamie Cooley 2011-05-24 10:23:11 PDT
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.
Comment 16 WebKit Review Bot 2011-05-24 10:26:09 PDT
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.
Comment 17 Benjamin Poulain 2011-05-30 15:21:38 PDT
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.
Comment 18 Jamie Cooley 2011-05-31 07:25:33 PDT
Created attachment 95431 [details]
Updated patch with Benjamin's review comments. 

Updated patch with comparing URLs directly rather than toString.
Comment 19 WebKit Review Bot 2011-05-31 07:32:54 PDT
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.
Comment 20 WebKit Commit Bot 2011-05-31 08:38:49 PDT
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>
Comment 21 WebKit Commit Bot 2011-05-31 08:38:56 PDT
All reviewed patches have been landed.  Closing bug.