[webkit-reviews] review denied: [Bug 3401] WebKit does not support setting selection range on form controls : [Attachment 2539] the latest iteration of the 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 2539: the latest iteration of the patch
http://bugzilla.opendarwin.org/attachment.cgi?id=2539&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looking good! Here are some more comments:

The code in HTMLInputElementImpl::setValue that casts m_render to
RenderLineEdit* doesn't seem to check m_type. I don't think it's good to have
this code assume that storesValueSeparateFromAttribute() implies it's a
RenderLineEdit * even if it's true, especially without a comment. But it's easy
to fix, because updateFromElement() is a function that exists in RenderObject
and there's no need to cast m_render to call updateFromElement() on it.

The new code in this patch omits spaces beteen class names and *, using syntax
like RenderTextArea*. We use a space. This is mentioned in our coding style
guidelines under "Names" as rule #9.

I know that you investigated using the real Qt API for QLineEdit, but I'm still
not sure why you ended up adding KWQ-specific methods. It seems clear that
RenderLineEdit::selectionStart, for example, could just call selectionStart()
and then if that returned -1, call cursorPosition(). This patch errs too far on
the "don't worry about Qt, just do something KWQ-specific" side, I think, and
now that it's working, I think you should try a little harder to use the Qt API
except where there's a reason you can't. To give another example, you should
name the function setSelection as in Qt rather than setSelectionRange and have
the parameter types be int rather than long. The sole exception here might be
selectionEnd, since we might not want to require getting the selection text
just to compute the end point.

I still see some use of "unsigned int" where we customarily use "unsigned".

I don't see the value in your "just in case" check for NULL in the
getCursorPositionAsIndex:inParagraph: code. Either we should allow NULL for the
parameters (in case someone is interested in only one or the other), or we
should assert if they are NULL. We shouldn't add dead code "just in case". And
this change seems unrelated to the rest of the patch. Why not just leave it
out?

Do we know what the performance impact is of the new CRLF code? We might need
to test that.

Can we refactor setSelectionStart, setSelectionEnd, and setSelectionRange so
that we don't need so much duplicate code? Even better, lets just have
setSelectionRange (and name it setSelection to match Qt) and have the callers
do a tiny bit more of the work themselves.