[webkit-reviews] review denied: [Bug 6814] Implement selection methods for RenderTextField : [Attachment 6926] patch

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 6814: Implement selection methods for RenderTextField
http://bugzilla.opendarwin.org/show_bug.cgi?id=6814

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

------- Additional Comments from Darin Adler <darin at apple.com>
Please use private instead of protected.

No need to check for a document() of 0 in RenderTextField::selectionStart(). A
renderer can't outlast its document.

No need to explicitly convert to VisiblePosition in
RenderTextField::selectionStart().

+    int start = kMax(s, 0);

Don't need that line of code, since getVisiblePositionForIndex already does the
right thing.

The setSelection helper does nothing if either position is null. I don't think
that's helpful behavior. I suggest asserting that neither is null, and further,
I suggest asserting that each is within the text field's generated content. I
don't think you need a separate function for this. It could be folded into
setSelectionRange.

Frame needs a function that does setSelection after doing shouldChangeSelection
and you should be calling that.

getVisiblePositionForIndex is willing to advance past the end of the div.
That's not good -- if the caller of setSelectionRange passes a large value for
end you'll end up selecting "off the end". If you change it to instead return
the end for those values, then it's guaranteed to never return null, which
makes it easier to use.

Normally we leave out the word "get" from function names like these.

getIndexForVisiblePosition should take a const VisiblePosition& rather than a
VisiblePosition.

You can make a faster version of getVisiblePositionForIndex by using
CharacterIterator.

You can make a much faster version of getIndexForVisiblePosition by composing a
range and calling TextIterator::rangeLength.

I noticed that clearSelectionIfNeeded needs to be a private function in
DocumentImpl.

As we discussed, we need to document what setFocusNode does clearly to figure
out when it should and should not clear the selection. Once we understand that
we can figure out whether a boolean parameter is an appropriate way to prevent
it from doing so.

The change to the logic about clearing selections in dispatchMouseEvent is not
clearly correct. We need more research to decide what rule we're trying to
implement.