[webkit-reviews] review denied: [Bug 3401] WebKit does not support setting selection range on form controls : [Attachment 2532] A revised patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Kevin Ballard <kevin at sb.org>'s request
for review:
Bug 3401: WebKit does not support setting selection range on form controls
http://bugzilla.opendarwin.org/show_bug.cgi?id=3401

Attachment 2532: A revised patch
http://bugzilla.opendarwin.org/attachment.cgi?id=2532&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
It's not great to have kjs_html.cpp be checking the type enum of the HTML input
element. Instead HTMLInputElementImpl needs a DOM API that indicates whether
things like selectionStart and selectionEnd are appropriate. A separate
"canHaveSelection" function that returns a boolean would be one appropriate way
to do it.

As far as layout tests are concerned, you can make a test that uses these new
methods easily, and even though it doesn't change layout, it would be good.

In fact, we should change DumpRenderTree so it can dump the selection of form
elements just as it dumps a selection within HTML today, and then you could
make some even-better tests.

There's no need to have a break after a return in a case statement and it's
generally considered bad style.

It looks like some of the indenting is done with tabs. All the indenting should
be using spaces. In particular, the code in RenderLineEdit looks like it's
indented with a tab, as does the declaration of setSelectionRange in
HTMLTextAreaElementImpl in the header.

We usually say "unsigned" rather than "unsigned int"
(QLineEdit::setSelectionStart).

The biggest lack here is tests, though. There are many different problems you
are fixing here and edge cases you are checking for. All of them should have
individual tests.

Looks good! Keep at it.