Bug 9150 – DumpRenderTree should be able to keep URL history during runs

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

It would be very useful for DumpRenderTree to remember URLs that it has accessed during each run, with the history being empty at the beginning of each run.

This would allow for testing history where visited links are a different color than unvisited links.

Note that there will be a number of tests whose results will change with this update.

[David Kilzer (:ddkilzer)]

Created attachment 8583 [details]
Patch v1

Patch to implement URL history in DumpRenderTree.

I still need to review the changed test results to make sure the changes aren't regressions.

[David Kilzer (:ddkilzer)]

Here is a list of tests that will require text and image updates if this patch (Attachment 8583 [details]) lands:

fast/block/margin-collapse/010
fast/block/margin-collapse/011
fast/block/margin-collapse/012
fast/block/margin-collapse/015
fast/block/margin-collapse/016
fast/block/margin-collapse/020
fast/block/margin-collapse/056
fast/block/margin-collapse/059
fast/repaint/outline-repaint-glitch
tables/mozilla/marvin/backgr_layers-opacity
tables/mozilla/marvin/backgr_position-table
tables/mozilla/marvin/backgr_simple-table-cell
tables/mozilla/marvin/backgr_simple-table-column
tables/mozilla/marvin/backgr_simple-table-column-group
tables/mozilla/marvin/backgr_simple-table
tables/mozilla/marvin/backgr_simple-table-row
tables/mozilla/marvin/backgr_simple-table-row-group
tables/mozilla_expected_failures/marvin/backgr_border-table
tables/mozilla_expected_failures/marvin/backgr_border-table-column
tables/mozilla_expected_failures/marvin/backgr_border-table-row
tables/mozilla_expected_failures/marvin/backgr_border-table-row-group
tables/mozilla_expected_failures/marvin/backgr_layers-hide
tables/mozilla_expected_failures/marvin/backgr_layers-show
tables/mozilla_expected_failures/marvin/backgr_position-table-cell
tables/mozilla_expected_failures/marvin/backgr_position-table-column
tables/mozilla_expected_failures/marvin/backgr_position-table-row
tables/mozilla_expected_failures/marvin/backgr_position-table-row-group

[David Kilzer (:ddkilzer)]

Created attachment 8589 [details]
Patch v2

Same as Patch v1 with changelogs and *-expected.txt updates.

Note that the matching pixel tests will also need to be updated before checking this in.

Also note that the current ToT has table layout failures.  These should be fixed before applying this patch.

(In reply to comment #3)
> Created an attachment (id=8589) [edit]

> Also note that the current ToT has table layout failures.  These should be
> fixed before applying this patch.

The expected results for those tests need to be updated. I have attached a patch to bug 8848.

[Darin Adler]

Comment on attachment 8589 [details]
Patch v2

What does this mean for running these tests individually?

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> (From update of attachment 8589 [details] [edit])
> What does this mean for running these tests individually?

It would mean that an individual test would fail since the color would appear as not-visited instead of visited.

All of the current tests whose results would change if the DRT patch were applied are referencing other tests that ran before them when the whole suite is run.

There are a number of easy fixes for this:

- Change the links to text-only.
- Force the link color to stay the same whether visited or not.
- Remove the links.

Thoughts?

[Darin Adler]

(In reply to comment #6)
> (In reply to comment #5)
> > What does this mean for running these tests individually?
> 
> It would mean that an individual test would fail since the color would appear
> as not-visited instead of visited.

The original concept for layout tests was that each test can run separately and give the same results. That means that DumpRenderTree should isolate each test from the next one. A simple way to do that would be to create a new history object each time, as long as that doesn't make the tests too long.

But that doesn't allow testing history and in fact it directly contradicts the argument made at the top of this bug report.

It seems great for tests that intend to test history to behave this way, but I'd prefer to not have tests affect each other by default. So we should rack our brains and figure out a way to solve the problem.

[Anders Carlsson]

How about we add a method to the layout test controller which creates a WebHistory object and sets it as the global history. Then we run the test and destroy the WebHistory object afterwards.

This way we only create history objects when they are needed and we get isolated tests.

[Darin Adler]

(In reply to comment #8)
> How about we add a method to the layout test controller which creates a
> WebHistory object and sets it as the global history. Then we run the test and
> destroy the WebHistory object afterwards.
> 
> This way we only create history objects when they are needed and we get
> isolated tests.

Something along those lines sounds excellent; just what we should do.

But I still don't understand how tests would work. Are there pairs of tests that go together, or would they be self-contained?

I don't need someone to explain it; instead one could just make an example that demonstrates how it will work.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> But I still don't understand how tests would work. Are there pairs of tests
> that go together, or would they be self-contained?
> 
> I don't need someone to explain it; instead one could just make an example that
> demonstrates how it will work.

See example tests (which are patches that place the tests in the LayoutTests directory structure) in bug 9145, bug 9148 or bug 9166.

[Darin Adler]

Comment on attachment 8589 [details]
Patch v2

I'm going to mark this review-, because I'd like to see a version where tests "opt in" to this feature, so that tests can be run singly as well as in groups.

[David Kilzer (:ddkilzer)]

Reassigning this bug back to webkit-unassigned if anyone is interested in fixing this.  I plan to come back to it in the next week or two if no one else is working on it.

[David Kilzer (:ddkilzer)]

Created attachment 8892 [details]
Patch v3

This patch adds a layoutTestController.keepWebHistory() method to enable web history.  It also contains a layout test that demonstrates its usage.

The change to the WebCore manual test is to (a) make it work if the JavaScript is commented back in and (b) document the window.history hack it uses.  Note that this manual test doesn't exhibit the problem seen in Safari when run by DumpRenderTree (Bug 8079), hence it's a manual test.

[David Kilzer (:ddkilzer)]

Note to self:  Update the wiki page when this lands.

http://wiki.opendarwin.org/index.php/WebKit:Writing_Layout_Tests_for_DumpRenderTree

[David Kilzer (:ddkilzer)]

Created attachment 8893 [details]
Patch v4

Same as Patch v4, except I removed the text/html mime type from the HTML files (svn-apply doesn't handle the property information--yet), and I only call [WebHistory setOptionalSharedHistory:nil] if the field is not currently nil.

[Geoffrey Garen]

Comment on attachment 8893 [details]
Patch v4

Looks good, and seems to address Darin's comments. r=me

In the layout test, using a fake mouse event to click the link seems a little funny. I'm pretty sure that dispatching an event through the DOM would work just as well. Just for future reference.

Part of me also thinks that the repaint and history code should go into separate "controller" objects, not layoutTestController, which should limit itself to direct control of the test mechanism.

[David Kilzer (:ddkilzer)]

Committed revision 14905.

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> Note to self:  Update the wiki page when this lands.
> http://wiki.opendarwin.org/index.php/WebKit:Writing_Layout_Tests_for_DumpRenderTree

Updated.

[David Kilzer (:ddkilzer)]

Created attachment 8909 [details]
Fix tests to use DOM mouse events

[David Kilzer (:ddkilzer)]

(In reply to comment #19)
> Created an attachment (id=8909) [edit]
> Fix tests to use DOM mouse events

r=ggaren via IRC

Committed revision 14907.