Bug 9150 - DumpRenderTree should be able to keep URL history during runs
: DumpRenderTree should be able to keep URL history during runs
Status: RESOLVED FIXED
: WebKit
Tools / Tests
: 420+
: Macintosh Mac OS X 10.4
: P2 Normal
Assigned To:
:
:
:
: 9144 9145 9148 9166
  Show dependency treegraph
 
Reported: 2006-05-28 11:44 PST by
Modified: 2006-06-18 15:16 PST (History)


Attachments
Patch v1 (965 bytes, patch)
2006-05-28 11:57 PST, David Kilzer (:ddkilzer)
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2 (29.70 KB, patch)
2006-05-29 06:47 PST, David Kilzer (:ddkilzer)
darin: review-
Review Patch | Details | Formatted Diff | Diff
Patch v3 (30.35 KB, patch)
2006-06-17 19:42 PST, David Kilzer (:ddkilzer)
no flags Review Patch | Details | Formatted Diff | Diff
Patch v4 (30.04 KB, patch)
2006-06-17 20:06 PST, David Kilzer (:ddkilzer)
ggaren: review+
Review Patch | Details | Formatted Diff | Diff
Fix tests to use DOM mouse events (3.55 KB, patch)
2006-06-18 15:10 PST, David Kilzer (:ddkilzer)
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2006-05-28 11:44:45 PST
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.
------- Comment #1 From 2006-05-28 11:57:38 PST -------
Created an attachment (id=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.
------- Comment #2 From 2006-05-28 15:37:53 PST -------
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
------- Comment #3 From 2006-05-29 06:47:29 PST -------
Created an attachment (id=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.
------- Comment #4 From 2006-05-29 08:18:03 PST -------
(In reply to comment #3)
> Created an attachment (id=8589) [edit] [details]

> 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.
------- Comment #5 From 2006-05-30 19:32:39 PST -------
(From update of attachment 8589 [details])
What does this mean for running these tests individually?
------- Comment #6 From 2006-05-30 20:01:29 PST -------
(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?
------- Comment #7 From 2006-05-31 10:34:00 PST -------
(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.
------- Comment #8 From 2006-06-01 06:08:08 PST -------
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.
------- Comment #9 From 2006-06-01 10:32:07 PST -------
(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.
------- Comment #10 From 2006-06-01 11:22:33 PST -------
(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.
------- Comment #11 From 2006-06-01 17:01:31 PST -------
(From update of attachment 8589 [details])
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.
------- Comment #12 From 2006-06-12 08:28:30 PST -------
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.
------- Comment #13 From 2006-06-17 19:42:55 PST -------
Created an attachment (id=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.
------- Comment #14 From 2006-06-17 19:44:13 PST -------
Note to self:  Update the wiki page when this lands.

http://wiki.opendarwin.org/index.php/WebKit:Writing_Layout_Tests_for_DumpRenderTree
------- Comment #15 From 2006-06-17 20:06:13 PST -------
Created an attachment (id=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.
------- Comment #16 From 2006-06-17 21:05:33 PST -------
(From update of attachment 8893 [details])
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.
------- Comment #17 From 2006-06-18 10:49:41 PST -------
Committed revision 14905.
------- Comment #18 From 2006-06-18 11:34:12 PST -------
(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.
------- Comment #19 From 2006-06-18 15:10:35 PST -------
Created an attachment (id=8909) [details]
Fix tests to use DOM mouse events
------- Comment #20 From 2006-06-18 15:16:44 PST -------
(In reply to comment #19)
> Created an attachment (id=8909) [edit] [details]
> Fix tests to use DOM mouse events

r=ggaren via IRC

Committed revision 14907.