[webkit-reviews] review granted: [Bug 6814] Implement selection methods for RenderTextField : [Attachment 6948] patch to address Darin's comments

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has granted 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 6948: patch to address Darin's comments
http://bugzilla.opendarwin.org/attachment.cgi?id=6948&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
a)

+    int indexForVisiblePosition(const VisiblePosition& pos);

No need for the name pos here; I would leave it out.

b)

+    setSelectionRange(selectionStart(), end);

If you set an end that is < the start with setSelectionEnd, should it move the
start back too?

c)

+    end = kMax(end, start);

More use of kMax is deprecated. Instead you should use the standard max
function template from <algorithm>. Just make sure something includes
<algorithm> and something does "using namespace std" and then call max instead
of kMax.

d)

If m_div is 0, then RenderTextField::setSelectionRange is going to fail its
assertions. The actual function looks like it will just change the selection to
nothing.

e)

RenderTextField::setSelectionRange always calls shouldChangeSelection even if
there's no selection change; that doesn't seem good. I think there is a good
argument for adding a function to frame that first checks if the new selection
is any different from the old, then calls shouldChangeSelection and then
setSelection.

f)

It's kind of annoying that CharacterIterator won't let you call advance if
you're already at the end, because it does let you try to advance past the end
and handles that correctly by pinging you at the end. At some point, we should
fix that so you can remove the atEnd() check here.

g)

+    return VisiblePosition(it.range()->endContainer(ec),
it.range()->endOffset(ec), DOWNSTREAM);

Maybe that should be UPSTREAM? I'm not sure.

These are all minor, so I'm marking this review+.

The most important issue is (d), and even that is an issue in debug versions
only. I'd like you to add some tests about how setSelectionRange behaves when
m_div is 0, but I don't understand under what circumstances it can be 0.