Bug 17557

Created attachment 19384 [details] remove setTimeout and update expected results I tried to update the pixel test results too, but I couldn't get any of the css2.1 tests to pass on my machine (Tiger).

I think the proposed change makes the tests invalid. The purpose of the timeout is to defer the change until after the document has been laid out and rendered once.

Ok, then we can use waitUntilDone/notifyDone to ensure that the timeout gets run.

(In reply to comment #3) > Ok, then we can use waitUntilDone/notifyDone to ensure that the timeout gets > run. That might work, but I think that as a rule the tests in /css2.1 are not to be modified. If these scenarios are not covered by other tests, then it might be best to make DumpRenderTree-compatible copies of these tests and put them, e.g. in fast/css/counters.

Comment on attachment 19384 [details] remove setTimeout and update expected results Seems good. r=me

Assigning to myself so that nobody lands the patch before I discuss my comments with Darin :-)

Comment on attachment 19384 [details] remove setTimeout and update expected results Setting to r- now. This bug should be addressed by creating copies of the tests and modifying them to work with DumpRenderTree using waitUntilDone and notifyDone. The updated patch should ideally include expected pixel results as well.

Created attachment 19505 [details] fork files and use waitUntilDone/notifyDone This patch forks the test files (svn cp) and uses waitUntilDone/notifyDone. However, I don't have expected results because it looks like this produces the wrong output. The setTimeout causes the wrong output to be displayed. The same is true of the existing tests in LayoutTests/css2.1, I just didn't notice before. It looks like this is a bug in WebKit.

(In reply to comment #7) > (From update of attachment 19384 [details] [edit]) > Setting to r- now. > > This bug should be addressed by creating copies of the tests and modifying them > to work with DumpRenderTree using waitUntilDone and notifyDone. > > The updated patch should ideally include expected pixel results as well. Odd. We've modified W3C tests in the pass to do things like this. I don't remember which set of tests though. I don't have any problems with modifying their tests. It makes updating to the latest versions of the tests more tricky, but it's silly to have broken versions of the tests checked into LayoutTests.

Created attachment 21270 [details] Patch for the 2 affected tests After going through all the test I found that only these 2 tests used settimout in the onload event and have altered them accordingly, unfortunately saving the files in windows changed the newlines hence the duplication.

Comment on attachment 21270 [details] Patch for the 2 affected tests I am guessing this needs to be reviewed, but it would really help if you could make a diff that showed only what's changed.

Comment on attachment 21270 [details] Patch for the 2 affected tests r=me

Comment on attachment 21270 [details] Patch for the 2 affected tests Once I applied the patch and put these diffs into a form I could read, I could tell they were incorrect. I was wrong to say r=me before! The meta-refresh-vs-open.html change was only adding a call to notifyDone inside the onload handler. That's not going to fix anything in the success case, since the onload handler only runs if the test fails. The http/tests/navigation/onload-navigation-iframe-timeout.html was no change at all, only a whitespace tweak.

I believe that there is a deeper problem in the CSS counters code than the waiting for the tests to finish. Currently there is a big fixme in CounterNode.cpp that acknowledges that the counters are not set up to handle changes to the DOM tree correctly. A fix for this is being discussed as part of bug 11031. Also I have been previously instructed by Darin to have modified CSS 2.1 tests (just as those for this bug put in fast/css/counters instead of css2.1)

I also think it's OK to modify css2.1 tests as long as only the harness is affected, and not anything in what the tests check for. It's just confusing to have tests that fail for wrong reasons.

Carol offered to take over this bug.

I will be working slowly on fixing these tests. Those that should work now, but I am doing this in my dead time while working on other tasks so it will take a while. If anyone wants to do this quickly feel free to pick up the task.

See also: bug 31311. It sounds like a waste to fix the obsolete version of test suite - on the other hand, I do not know if the "new" version is mature enough to replace it.

Created attachment 47168 [details] Create copy in fast/css/counters

It seems the "current" version of the six tests in question have almost no differences from the copies we have (only some meta tags changed). So, I think we can modify these tests if we want. We've already had 5 copies of these tests in fast/css/counters and we only need to add one more test. So, this patch does - Add README.txt into LayoutTests/css2.1 . This was suggested by Darin in Bug 33942. - Disable the 6 tests in LayoutTests/css2.1 as they are invalid with DumpRenderTree. - Add counter-reset-001.html to LayoutTests/fast/css . This is modified version of t1204-reset-01-c-o.html . - Rename t1204-* in LayoutTests/fast/css to counter-* . It seems the recent version of W3C's CSS2.1 test suite uses this naming. - Remove unnecessary expectation files. I hope this fix makes sense.

Comment on attachment 47168 [details] Create copy in fast/css/counters Seems fine. rs=me

Comment on attachment 47168 [details] Create copy in fast/css/counters Clearing flags on attachment: 47168 Committed r53701: <http://trac.webkit.org/changeset/53701>

All reviewed patches have been landed. Closing bug.