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
Created attachment 35109 [details] Patch for first two tests --- 8 files changed, 219 insertions(+), 8 deletions(-)
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.
Created attachment 35110 [details] Change for CSS2.1 test suites
I can't remember who it was who added Counter support. Might have been adele.
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.
Created attachment 39170 [details] Patch for first two tests v2 --- 2 files changed, 2 insertions(+), 16 deletions(-)
Created attachment 39171 [details] Patch for first two tests v2 --- 8 files changed, 205 insertions(+), 8 deletions(-)
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.
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.
Created attachment 39335 [details] Patch for first two tests v3 --- 12 files changed, 217 insertions(+), 8 deletions(-)
Created attachment 39337 [details] Patch for first two tests v4 --- 12 files changed, 225 insertions(+), 8 deletions(-)
> 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.
Ping? I think I can add pixel test results for chromium port, gtk port (and maybe qt port) after this patch is submitted.
Comment on attachment 39337 [details] Patch for first two tests v4 OK. No, sadly WebKit has no such rebaselining tool.
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.
Committed r49225: <http://trac.webkit.org/changeset/49225>
Still 4 test cases are failing.
Created attachment 40765 [details] Gtk+ layout tests fix
Created attachment 40803 [details] Qt layout tests fix
Why are these results needed? Why can't all platforms share results? Can we use a font that all platforms have?
(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?
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.
Created attachment 40860 [details] Layout tests with Ahem
I see. I updated the layout tests with Ahem so that its render tree is platform independent. Thanks!
Created attachment 41226 [details] Layout tests with Ahem v2
(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 // .
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 on attachment 39337 [details] Patch for first two tests v4 Clearing r+ on this obsolete patch.
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.
Carol is working for this issue. *** This bug has been marked as a duplicate of bug 11031 ***