Bug 34167 - [Qt] DRT WebHistory support
Summary: [Qt] DRT WebHistory support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-01-26 07:56 PST by Diego Gonzalez
Modified: 2010-01-27 05:52 PST (History)
5 users (show)

See Also:


Attachments
Activate history in DRT (2.43 KB, patch)
2010-01-26 08:23 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Proposed patch to pass history tests (5.81 KB, patch)
2010-01-26 11:28 PST, Diego Gonzalez
kenneth: review-
Details | Formatted Diff | Diff
Proposed patch v0.2 (5.81 KB, patch)
2010-01-26 11:35 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Proposed patch v0.3 (5.81 KB, patch)
2010-01-26 11:44 PST, Diego Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Gonzalez 2010-01-26 07:56:31 PST
Activate WebHistory support in Qt DRT.

Implement the keepWebHistory() and webHistoryItemCount() in LayoutTestController 

LayoutTests: http/tests/history
Comment 1 Diego Gonzalez 2010-01-26 08:23:07 PST
Created attachment 47412 [details]
Activate history in DRT

Possible way to activate the history in Qt DRT
Comment 2 Diego Gonzalez 2010-01-26 09:36:32 PST
The tests is not passing with the patch above (47412) because in the redirect file page (LayoutTests/http/tests/history/resources/redirect-target.html) the webHistoryItemCount is increased in one according the code below.

function testHistoryItemCount()
{
    var expected = parseInt(location.hash.slice(1));
    var actual = layoutTestController.webHistoryItemCount + 1; // Add one to include the referring page, which loaded before we started recording history.
    if (actual === expected)
        log("PASS: History item count should be " + expected + " and is.");
    else
        log("FAIL: History item count should be " + expected + " but instead is " + actual + ".");
}

In our case is not necessary to add one to webHistoryItemCount because QWebHistory, created in QWebPage, records the referred page.

One possible way to make the tests succeed is use specific expected files for Qt port use in these tests.

Other ways coukd be: decrease in one the return of layoutTestController.webHistoryItemCount(); use the backItems() size; or limit the maximum count of QWebHistory.

Someone has any suggestion or comment of the the best way to solve it?
Comment 3 Antonio Gomes 2010-01-26 09:42:43 PST
> In our case is not necessary to add one to webHistoryItemCount because
> QWebHistory, created in QWebPage, records the referred page.
> 
> One possible way to make the tests succeed is use specific expected files for
> Qt port use in these tests.

I vote for this option.

> 
> Other ways coukd be: decrease in one the return of
> layoutTestController.webHistoryItemCount(); use the backItems() size; or limit
> the maximum count of QWebHistory.

This way, we would be adapting our implementation to match a test result.

@kenneth, simon: thoughts ?
Comment 4 Kenneth Rohde Christiansen 2010-01-26 09:46:13 PST
(In reply to comment #3)
> > In our case is not necessary to add one to webHistoryItemCount because
> > QWebHistory, created in QWebPage, records the referred page.
> > 
> > One possible way to make the tests succeed is use specific expected files for
> > Qt port use in these tests.
> 
> I vote for this option.

Expected results saying FAIL. That doesn't sound good. Better just add 1 to the result.
Comment 5 Diego Gonzalez 2010-01-26 09:55:29 PST
(In reply to comment #4)
> (In reply to comment #3)
> > > In our case is not necessary to add one to webHistoryItemCount because
> > > QWebHistory, created in QWebPage, records the referred page.
> > > 
> > > One possible way to make the tests succeed is use specific expected files for
> > > Qt port use in these tests.
> > 
> > I vote for this option.
> 
> Expected results saying FAIL. That doesn't sound good. Better just add 1 to the
> result.

Kenneth, you mean add 1 in the expected file or when the method returns?
Comment 6 Diego Gonzalez 2010-01-26 11:28:25 PST
Created attachment 47427 [details]
Proposed patch to pass history tests

With this patch the following tests are passing now:

http/tests/history/redirect-js-form-submit-2-seconds.html
http/tests/history/redirect-js-location-2-seconds.html
http/tests/history/redirect-js-location-assign-before-load.html
http/tests/history/redirect-js-location-href-2-seconds.html
http/tests/history/redirect-js-location-href-0-seconds.html
http/tests/history/redirect-js-location-replace-2-seconds.html
http/tests/history/redirect-js-document-location-before-load.html
http/tests/history/redirect-js-location-assign-0-seconds.html
http/tests/history/redirect-js-location-0-seconds.html
http/tests/history/redirect-meta-refresh-2-seconds.html
http/tests/history/redirect-js-document-location-2-seconds.html
http/tests/history/redirect-js-location-replace-before-load.html
http/tests/history/redirect-js-form-submit-0-seconds.html
http/tests/history/redirect-js-location-before-load.html
http/tests/history/redirect-js-location-href-before-load.html
http/tests/history/redirect-js-form-submit-before-load.html
http/tests/history/redirect-js-document-location-0-seconds.html
http/tests/history/redirect-js-location-assign-2-seconds.html
http/tests/history/redirect-js-location-replace-0-seconds.html
http/tests/history/redirect-meta-refresh-0-seconds.html
http/tests/history/redirect-301.pl
http/tests/history/redirect-303.pl
http/tests/history/redirect-200-refresh-2-seconds.pl
http/tests/history/redirect-307.pl
http/tests/history/redirect-302.pl
http/tests/history/redirect-200-refresh-0-seconds.pl
Comment 7 Kenneth Rohde Christiansen 2010-01-26 11:29:39 PST
Comment on attachment 47427 [details]
Proposed patch to pass history tests

Please don't mark your own patches as r+
Comment 8 Kenneth Rohde Christiansen 2010-01-26 11:31:39 PST
Comment on attachment 47427 [details]
Proposed patch to pass history tests


> +    // Subtract one here as out QWebHistory::count() includes the actual page,
> +    // which is not considered in the DRT tests.

our, not out

> +    return m_webHistory->count() - 1;
> +}
> +
>  void LayoutTestController::keepWebHistory()
>  {
> -    // FIXME: implement
> +    m_webHistory = m_drt->webPage()->history();
>  }
Comment 9 Diego Gonzalez 2010-01-26 11:35:14 PST
Created attachment 47428 [details]
Proposed patch v0.2

Fixed typo
Comment 10 Diego Gonzalez 2010-01-26 11:44:40 PST
Created attachment 47429 [details]
Proposed patch v0.3

return -1 if web history doesn't exists
Comment 11 WebKit Commit Bot 2010-01-26 21:21:17 PST
Comment on attachment 47429 [details]
Proposed patch v0.3

Clearing flags on attachment: 47429

Committed r53895: <http://trac.webkit.org/changeset/53895>
Comment 12 WebKit Commit Bot 2010-01-26 21:21:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Csaba Osztrogonác 2010-01-26 23:56:00 PST
There are 4 failing tests on buildbot after http://trac.webkit.org/changeset/53895
http/tests/history/redirect-301.pl
http/tests/history/redirect-302.pl
http/tests/history/redirect-303.pl
http/tests/history/redirect-307.pl

Skipped them by http://trac.webkit.org/changeset/53906
Comment 14 Csaba Osztrogonác 2010-01-26 23:57:20 PST
detailed results: http://build.webkit.org/results/Qt%20Linux%20Release/r53905%20%286565%29/results.html
Comment 15 Diego Gonzalez 2010-01-27 04:46:10 PST
(In reply to comment #14)
> detailed results:
> http://build.webkit.org/results/Qt%20Linux%20Release/r53905%20%286565%29/results.html

I'm taking a look
Comment 16 Diego Gonzalez 2010-01-27 05:28:11 PST
For some reason LayoutTestController is not being instatiated in these tests. I'm checking it now!
Comment 17 Diego Gonzalez 2010-01-27 05:43:18 PST
(In reply to comment #16)
> For some reason LayoutTestController is not being instatiated in these tests.
> I'm checking it now!

Actually the LayoutTestController is being instantiated. The keepWebHistory() is not being called because the check:
 
if (window.layoutTestController) {
    layoutTestController.keepWebHistory();
    layoutTestController.waitUntilDone();
}

is not passing.
Comment 18 Diego Gonzalez 2010-01-27 05:51:22 PST
(In reply to comment #17)
> (In reply to comment #16)
> > For some reason LayoutTestController is not being instatiated in these tests.
> > I'm checking it now!
> 
> Actually the LayoutTestController is being instantiated. The keepWebHistory()
> is not being called because the check:
> 
> if (window.layoutTestController) {
>     layoutTestController.keepWebHistory();
>     layoutTestController.waitUntilDone();
> }
> 
> is not passing.

Created another bug: https://bugs.webkit.org/show_bug.cgi?id=34212 to investigate it. So, closing it