Bug 23262

Summary:

Dual lines in css2.1 layout tests do not match:

Product:

WebKit

Reporter:

Pierre-Olivier Latour <pol>

Component:

Tools / Tests

Assignee:

Shinichiro Hamaji <hamaji>

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

adele, eric, hamaji, mitz

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

Attachments:

Description	Flags
Patch for first two tests	none
Change for CSS2.1 test suites	none
Patch for first two tests v2	none
Patch for first two tests v2	none
Patch for first two tests v3	none
Patch for first two tests v4	commit-queue: commit-queue-
Gtk+ layout tests fix	none
Qt layout tests fix	none
Layout tests with Ahem	none
Layout tests with Ahem v2	none

Pierre-Olivier Latour

Reported 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

Attachments
Patch for first two tests (12.05 KB, patch) 2009-08-19 01:51 PDT, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Change for CSS2.1 test suites (5.20 KB, patch) 2009-08-19 02:19 PDT, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Patch for first two tests v2 (2.00 KB, patch) 2009-09-07 22:27 PDT, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Patch for first two tests v2 (11.35 KB, patch) 2009-09-07 22:31 PDT, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Patch for first two tests v3 (13.38 KB, patch) 2009-09-10 02:08 PDT, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Patch for first two tests v4 (14.01 KB, patch) 2009-09-10 02:18 PDT, Shinichiro Hamaji	commit-queue: commit-queue-	Details Formatted Diff Diff
Gtk+ layout tests fix (4.46 KB, patch) 2009-10-06 23:38 PDT, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Qt layout tests fix (4.47 KB, patch) 2009-10-07 10:13 PDT, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Layout tests with Ahem (12.96 KB, patch) 2009-10-08 01:39 PDT, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Layout tests with Ahem v2 (13.00 KB, patch) 2009-10-15 07:56 PDT, Shinichiro Hamaji	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Shinichiro Hamaji

Comment 1 2009-08-19 01:51:51 PDT

Created attachment 35109 [details] Patch for first two tests --- 8 files changed, 219 insertions(+), 8 deletions(-)

Shinichiro Hamaji

Comment 2 2009-08-19 02:14:50 PDT

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.

Shinichiro Hamaji

Comment 3 2009-08-19 02:19:26 PDT

Created attachment 35110 [details] Change for CSS2.1 test suites

Eric Seidel (no email)

Comment 4 2009-09-03 00:29:42 PDT

I can't remember who it was who added Counter support. Might have been adele.

Eric Seidel (no email)

Comment 5 2009-09-03 00:32:54 PDT

Comment on attachment 35109 [details] Patch for first two tests 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.

Shinichiro Hamaji

Comment 6 2009-09-07 22:27:01 PDT

Created attachment 39170 [details] Patch for first two tests v2 --- 2 files changed, 2 insertions(+), 16 deletions(-)

Shinichiro Hamaji

Comment 7 2009-09-07 22:31:31 PDT

Created attachment 39171 [details] Patch for first two tests v2 --- 8 files changed, 205 insertions(+), 8 deletions(-)

Shinichiro Hamaji

Comment 8 2009-09-07 23:20:58 PDT

Thanks for the review! (In reply to comment #5) > (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"> 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.

Eric Seidel (no email)

Comment 9 2009-09-09 11:11:13 PDT

Comment on attachment 39171 [details] Patch for first two tests v2 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.

Shinichiro Hamaji

Comment 10 2009-09-10 02:08:42 PDT

Created attachment 39335 [details] Patch for first two tests v3 --- 12 files changed, 217 insertions(+), 8 deletions(-)

Shinichiro Hamaji

Comment 11 2009-09-10 02:18:09 PDT

Created attachment 39337 [details] Patch for first two tests v4 --- 12 files changed, 225 insertions(+), 8 deletions(-)

Shinichiro Hamaji

Comment 12 2009-09-10 02:20:42 PDT

> 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.

Shinichiro Hamaji

Comment 13 2009-09-30 10:28:21 PDT

Ping? I think I can add pixel test results for chromium port, gtk port (and maybe qt port) after this patch is submitted.

Eric Seidel (no email)

Comment 14 2009-10-06 08:51:48 PDT

Comment on attachment 39337 [details] Patch for first two tests v4 OK. No, sadly WebKit has no such rebaselining tool.

WebKit Commit Bot

Comment 15 2009-10-06 22:55:30 PDT

Comment on attachment 39337 [details] Patch for first two tests v4 Rejecting patch 39337 from commit-queue. Patch https://bugs.webkit.org/attachment.cgi?id=39337 from bug 23262 failed to download and apply.

Shinichiro Hamaji

Comment 16 2009-10-06 23:20:33 PDT

Committed r49225: <http://trac.webkit.org/changeset/49225>

Shinichiro Hamaji

Comment 17 2009-10-06 23:22:24 PDT

Still 4 test cases are failing.

Shinichiro Hamaji

Comment 18 2009-10-06 23:38:59 PDT

Created attachment 40765 [details] Gtk+ layout tests fix

Shinichiro Hamaji

Comment 19 2009-10-07 10:13:53 PDT

Created attachment 40803 [details] Qt layout tests fix

Eric Seidel (no email)

Comment 20 2009-10-07 10:16:29 PDT

Why are these results needed? Why can't all platforms share results? Can we use a font that all platforms have?

Shinichiro Hamaji

Comment 21 2009-10-07 10:26:47 PDT

(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?

Eric Seidel (no email)

Comment 22 2009-10-07 11:23:37 PDT

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.

Shinichiro Hamaji

Comment 23 2009-10-08 01:39:56 PDT

Created attachment 40860 [details] Layout tests with Ahem

Shinichiro Hamaji

Comment 24 2009-10-08 01:41:23 PDT

I see. I updated the layout tests with Ahem so that its render tree is platform independent. Thanks!

Shinichiro Hamaji

Comment 25 2009-10-15 07:56:56 PDT

Created attachment 41226 [details] Layout tests with Ahem v2

Shinichiro Hamaji

Comment 26 2009-10-15 07:57:29 PDT

(In reply to comment #25) > Created an attachment (id=41226) [details] > Layout tests with Ahem v2 I found comments for CSS should be /* and */, not // .

Shinichiro Hamaji

Comment 27 2009-10-19 01:37:35 PDT

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.

Eric Seidel (no email)

Comment 28 2009-10-19 15:11:50 PDT

Comment on attachment 39337 [details] Patch for first two tests v4 Clearing r+ on this obsolete patch.

Shinichiro Hamaji

Comment 29 2009-10-20 01:00:00 PDT

Comment on attachment 41226 [details] Layout tests with Ahem v2 Remove review? bit for now. Bug 30555 fixes this issue in another (and probably better) way.

Shinichiro Hamaji

Comment 30 2009-11-18 21:45:24 PST

Carol is working for this issue. *** This bug has been marked as a duplicate of bug 11031 ***

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