[webkit-reviews] review denied: [Bug 23262] Dual lines in css2.1 layout tests do not match: : [Attachment 35109] Patch for first two tests

Thu Sep 3 00:32:53 PDT 2009

Eric Seidel <eric at webkit.org> has denied Shinichiro Hamaji
<hamaji at chromium.org>'s request for review:
Bug 23262: Dual lines in css2.1 layout tests do not match:
https://bugs.webkit.org/show_bug.cgi?id=23262

Attachment 35109: Patch for first two tests
https://bugs.webkit.org/attachment.cgi?id=35109&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.