Bug 23262 - Dual lines in css2.1 layout tests do not match:
: Dual lines in css2.1 layout tests do not match:
Status: RESOLVED DUPLICATE of bug 11031
: WebKit
Tools / Tests
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-01-12 13:00 PST by
Modified: 2009-11-18 21:45 PST (History)


Attachments
Patch for first two tests (12.05 KB, patch)
2009-08-19 01:51 PST, Shinichiro Hamaji
no flags Review Patch | Details | Formatted Diff | Diff
Change for CSS2.1 test suites (5.20 KB, patch)
2009-08-19 02:19 PST, Shinichiro Hamaji
no flags Review Patch | Details | Formatted Diff | Diff
Patch for first two tests v2 (2.00 KB, patch)
2009-09-07 22:27 PST, Shinichiro Hamaji
no flags Review Patch | Details | Formatted Diff | Diff
Patch for first two tests v2 (11.35 KB, patch)
2009-09-07 22:31 PST, Shinichiro Hamaji
no flags Review Patch | Details | Formatted Diff | Diff
Patch for first two tests v3 (13.38 KB, patch)
2009-09-10 02:08 PST, Shinichiro Hamaji
no flags Review Patch | Details | Formatted Diff | Diff
Patch for first two tests v4 (14.01 KB, patch)
2009-09-10 02:18 PST, Shinichiro Hamaji
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Gtk+ layout tests fix (4.46 KB, patch)
2009-10-06 23:38 PST, Shinichiro Hamaji
no flags Review Patch | Details | Formatted Diff | Diff
Qt layout tests fix (4.47 KB, patch)
2009-10-07 10:13 PST, Shinichiro Hamaji
no flags Review Patch | Details | Formatted Diff | Diff
Layout tests with Ahem (12.96 KB, patch)
2009-10-08 01:39 PST, Shinichiro Hamaji
no flags Review Patch | Details | Formatted Diff | Diff
Layout tests with Ahem v2 (13.00 KB, patch)
2009-10-15 07:56 PST, Shinichiro Hamaji
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 2009-01-12 13:00:12 PST
Dual lines in css2.1 layout tests do not match:
css2.1/t1204-increment-00-c-o.html
css2.1/t1204-increment-01-c-o.html
css2.1/t1204-increment-02-c-o.html
css2.1/t1204-reset-00-c-o.html
css2.1/t1204-reset-01-c-o.html
css2.1/t1204-reset-02-c-o.html
------- Comment #1 From 2009-08-19 01:51:51 PST -------
Created an attachment (id=35109) [details]
Patch for first two tests


---
 8 files changed, 219 insertions(+), 8 deletions(-)
------- Comment #2 From 2009-08-19 02:14:50 PST -------
Let me explain this patch.

This should fix the first two failures. As I've looked the discussion of Bug 17557, I copied the original CSS2.1 test suites from css2.1/* to fast/css/counters/* . I don't think this patch can be handled properly by commit-queue. Please wait until I'll eventually get my committer bit or use svn cp for these test files before you commit this patch on behalf of me.

As the diff of tests aren't clear, I'll upload a diff which shows the difference of the tests.

This patch contains two fixes actually. However, splitting them into two patches causes tentative layout test failures.

The fix for CounterNode.cpp, which fixes the original issue, makes css2.1/t120401-scope-02-c.html and css2.1/t120401-scope-03-c.html fail. They update the value of counter to wrong values, but they passed luckily because setNeedsLayoutAndPrefWidthsRecalc() is never called. I guess the re-layout function is not called due to this change:

http://trac.webkit.org/changeset/21606

The changes the meaning of the m_renderer. As m_renderer is the parent() of corresponding render counter, we should check the children of m_renderer to re-layout them.

The fix for RenderCounter.cpp fixes issues in css2.1/t120401-scope-02-c.html and css2.1/t120401-scope-03-c.html. Suppose the counter is showing the following numbers:

1.
 1.1.
2.

For this case, the previous code creates the tree of counter node like:

reset
 increment
 increment
 reset
  increment

But I think this should be

reset
 increment
 reset
  increment
 increment

This order fix is necessary because CounterNode::recount() updates the nodes' value using the values of the previous siblings.
------- Comment #3 From 2009-08-19 02:19:26 PST -------
Created an attachment (id=35110) [details]
Change for CSS2.1 test suites
------- Comment #4 From 2009-09-03 00:29:42 PST -------
I can't remember who it was who added Counter support.  Might have been adele.
------- Comment #5 From 2009-09-03 00:32:54 PST -------
(From update of attachment 35109 [details])
The experts have been silent about your patch for over 2 weeks.  I'm not an expert here, but your patch looks non-harmful.

Some nits:
 1 <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">

Why the BOM?

Why all the extra junk in the tests?
 5   <link rel="help" href="http://www.w3.org/TR/CSS21/generate.html#counters">
 6   <link rel="help" href="http://www.w3.org/TR/CSS21/generate.html#propdef-content">
 7   <link rel="help" href="http://www.w3.org/TR/CSS21/syndata.html#counter">

No need to run this off a timeout:
 37  <body onload="setTimeout('run()', 0)">

Better to just run it from inline script by placing the script tag low enough in the body...  Then you don't need "notifyDone()" either.

Do these tests really need to dump the render tree?  Ideally tests should be js-only test first, if they can't be js-only, at least dump as text, and render tree dumping tests should be used as a last-resort.
------- Comment #6 From 2009-09-07 22:27:01 PST -------
Created an attachment (id=39170) [details]
Patch for first two tests v2


---
 2 files changed, 2 insertions(+), 16 deletions(-)
------- Comment #7 From 2009-09-07 22:31:31 PST -------
Created an attachment (id=39171) [details]
Patch for first two tests v2


---
 8 files changed, 205 insertions(+), 8 deletions(-)
------- Comment #8 From 2009-09-07 23:20:58 PST -------
Thanks for the review!

(In reply to comment #5)
> (From update of attachment 35109 [details] [details])
> The experts have been silent about your patch for over 2 weeks.  I'm not an
> expert here, but your patch looks non-harmful.
> 
> Some nits:
>  1 <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN">
> 
> Why the BOM?
> 
> Why all the extra junk in the tests?
>  5   <link rel="help" href="http://www.w3.org/TR/CSS21/generate.html#counters">
>  6   <link rel="help"
> href="http://www.w3.org/TR/CSS21/generate.html#propdef-content">
>  7   <link rel="help" href="http://www.w3.org/TR/CSS21/syndata.html#counter">

Removed them.

> No need to run this off a timeout:
>  37  <body onload="setTimeout('run()', 0)">
> 
> Better to just run it from inline script by placing the script tag low enough
> in the body...  Then you don't need "notifyDone()" either.

This test checks if the counters work properly when some elements are added/removed *after* the first rendering finishes. So, if we put the script in <body> this only checks the first rendering. Counters are working properly for the first rendering even before this change. That's why we need to use the timeout for this test.

> Do these tests really need to dump the render tree?  Ideally tests should be
> js-only test first, if they can't be js-only, at least dump as text, and render
> tree dumping tests should be used as a last-resort.

I think we need to use dump render tree unfortunately. Because counter is a CSS element, it isn't dumped in dumpAsText output and it cannot be obtained from javascripts.
------- Comment #9 From 2009-09-09 11:11:13 PST -------
(From update of attachment 39171 [details])
OK.  Since this is so hard to test, I should ask:  you tested to make sure your tests failed before your change?

Also this will cause failures on the bots because results for other platforms are missing.  Someone will have to commit this manually and be around to commit the results for the other platforms too.

The tests need some comments to explain the tricky bits.  Like the fact that they use a timeout and that they are not dump as text because the bug doesn't trigger in those cases.
------- Comment #10 From 2009-09-10 02:08:42 PST -------
Created an attachment (id=39335) [details]
Patch for first two tests v3


---
 12 files changed, 217 insertions(+), 8 deletions(-)
------- Comment #11 From 2009-09-10 02:18:09 PST -------
Created an attachment (id=39337) [details]
Patch for first two tests v4


---
 12 files changed, 225 insertions(+), 8 deletions(-)
------- Comment #12 From 2009-09-10 02:20:42 PST -------
> OK.  Since this is so hard to test, I should ask:  you tested to make sure your
> tests failed before your change?

Yes. If you want, please check the following URLs:

http://shinh.skr.jp/t/t1204-increment-00-c-o.html
http://shinh.skr.jp/t/t1204-increment-01-c-o.html

You should see two lines and these texts are different.

> Also this will cause failures on the bots because results for other platforms
> are missing.  Someone will have to commit this manually and be around to commit
> the results for the other platforms too.

Yeah, I'm sorry about this... Are there any tools which are similar to the Chromium's rebaseline tool? I don't know how it is doing the trick actually. But, I think it sends some layout tests to the buildbot server, the server runs the layout tests with recent builds, and the server returns the results. We can check the result images and if it looks OK, we can commit the results. This is quite handy because we don't need to have many platforms we are supporting.

> The tests need some comments to explain the tricky bits.  Like the fact that
> they use a timeout and that they are not dump as text because the bug doesn't
> trigger in those cases.

I see. I added some comments.
------- Comment #13 From 2009-09-30 10:28:21 PST -------
Ping? I think I can add pixel test results for chromium port, gtk port (and maybe qt port) after this patch is submitted.
------- Comment #14 From 2009-10-06 08:51:48 PST -------
(From update of attachment 39337 [details])
OK.  No, sadly WebKit has no such rebaselining tool.
------- Comment #15 From 2009-10-06 22:55:30 PST -------
(From update of attachment 39337 [details])
Rejecting patch 39337 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=39337 from bug 23262 failed to download and apply.
------- Comment #16 From 2009-10-06 23:20:33 PST -------
Committed r49225: <http://trac.webkit.org/changeset/49225>
------- Comment #17 From 2009-10-06 23:22:24 PST -------
Still 4 test cases are failing.
------- Comment #18 From 2009-10-06 23:38:59 PST -------
Created an attachment (id=40765) [details]
Gtk+ layout tests fix
------- Comment #19 From 2009-10-07 10:13:53 PST -------
Created an attachment (id=40803) [details]
Qt layout tests fix
------- Comment #20 From 2009-10-07 10:16:29 PST -------
Why are these results needed?  Why can't all platforms share results?  Can we use a font that all platforms have?
------- Comment #21 From 2009-10-07 10:26:47 PST -------
(In reply to comment #20)
> Why are these results needed?  Why can't all platforms share results?  Can we
> use a font that all platforms have?

Ah, I didn't know we can share dump render tree results using the same font. Thanks for this advice! So, could you tell me what font can be used in all platforms?
------- Comment #22 From 2009-10-07 11:23:37 PST -------
Well, at least Ahem is required on all platforms for testing.

You can check in render tree results right next to the test and those will be used for all platforms.
------- Comment #23 From 2009-10-08 01:39:56 PST -------
Created an attachment (id=40860) [details]
Layout tests with Ahem
------- Comment #24 From 2009-10-08 01:41:23 PST -------
I see. I updated the layout tests with Ahem so that its render tree is platform independent. Thanks!
------- Comment #25 From 2009-10-15 07:56:56 PST -------
Created an attachment (id=41226) [details]
Layout tests with Ahem v2
------- Comment #26 From 2009-10-15 07:57:29 PST -------
(In reply to comment #25)
> Created an attachment (id=41226) [details] [details]
> Layout tests with Ahem v2

I found comments for CSS should be /* and */, not // .
------- Comment #27 From 2009-10-19 01:37:35 PST -------
As we need several steps to handle the rest three test suites correctly, I'd like to open other bugs for them. The first step is Bug 30505.
------- Comment #28 From 2009-10-19 15:11:50 PST -------
(From update of attachment 39337 [details])
Clearing r+ on this obsolete patch.
------- Comment #29 From 2009-10-20 01:00:00 PST -------
(From update of attachment 41226 [details])
Remove review? bit for now. Bug 30555 fixes this issue in another (and probably better) way.
------- Comment #30 From 2009-11-18 21:45:24 PST -------
Carol is working for this issue.

*** This bug has been marked as a duplicate of bug 11031 ***