[webkit-reviews] review granted: [Bug 27632] Expose text segmentation through JavaScript : [Attachment 38625] patch w/ layout test

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 2 14:16:08 PDT 2009


Eric Seidel <eric at webkit.org> has granted Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 27632: Expose text segmentation through JavaScript
https://bugs.webkit.org/show_bug.cgi?id=27632

Attachment 38625: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=38625&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
For the future, it's much better when the test cases include their expected
result.

For example:
shouldBe("range.toString()", "Here");
is much clearer than:
+    log("word5: " + range.toString());

Because implementers don't have to compare with a golden file to know if
they've passed/failed the test.


Even better is to use functions to make the test expectations even more
readable, like:

shouldBe("expandRange(mydiv, 2, mydiv, 3, 'word')", "Here");

Where "expandRange" is some locally defined function which does the right
thing.

shouldBe will print the function call text and if it passed or failed.	

Use of the "shouldBe" asserts require the test to be a script test/DOM only
test, which is documented here:
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

I'll r+ the patch for now.  If you'd like to update the test more and upload a
new patch that's great.  If you're pressed on time for other things that's OK
too, and someone can mark this commit-queue+ tomorrow after you've had a chance
to see my comments.


More information about the webkit-reviews mailing list