[webkit-reviews] review denied: [Bug 27032] document.title does not replace or remove space characters : [Attachment 42702] Patch and test case for collapsing whitespace when using document.title

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Christian Sejersen
<christian.webkit at gmail.com>'s request for review:
Bug 27032: document.title does not replace or remove space characters
https://bugs.webkit.org/show_bug.cgi?id=27032

Attachment 42702: Patch and test case for collapsing whitespace when using
document.title
https://bugs.webkit.org/attachment.cgi?id=42702&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for taking this on!

This new work seems to be a subset of the work already done in the
DocumentLoader function canonicalizedTitle. I don't think Document::title
should have its own separate copy of the logic. Some of what canonicalizedTitle
does may be specific to display -- I'm thinking specifically of
displayBufferModifiedByEncoding -- but most of it should be shared in common
with Document::title.

I think that rather than adding new code for this, instead you should be moving
some of the code from DocumentLoader to Document.

A single title in the test case is really light coverage. I'd like to see a
test case that sets all sorts of title strings to test the various edge cases
and lots of characters other than just letters and the space character. The
simplest way to write such a test would be to set and then get the document's
title. I recommend a script-tests style test rather than writing your own mini
test harness. See an example such as
fast/Element/script-tests/element-traversal.js. The HTML driver file for the
script test is made by typing make-script-test-wrappers.