[webkit-reviews] review denied: [Bug 3401] WebKit does not support setting selection range on form controls : [Attachment 2520] A patch to fix this issue

[bugzilla-request-daemon at opendarwin.org]

Maciej Stachowiak <mjs 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 2520: A patch to fix this issue
http://bugzilla.opendarwin.org/attachment.cgi?id=2520&action=edit

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
Comments on the patch:

This does not match the coding style guidelines, the brace should be on a
separate line:

+static Value getInputSelectionStart(HTMLInputElementImpl &input) {

This code looks wrong, the toULong call attempts to turn a property name into a
number, so it won't match "selectionStart", rather it will match some arbitrary
integer. What you want to do to convert an identifier to a property int tag is
to use Lookup::findEntry. The Window object has an example of how to do it.
Right now, probably every input element appears to have the selectionStart and
selectionEnd properties, since the fallback goes to the special property
hashtable.

+      uint u = propertyName.toULong(&ok);
+      if (u == InputSelectionStart || u == InputSelectionEnd) {

For the following, maybe a #error directive or assert of some kind would work
better:

+    // FIXME: I can't test Qt, so I'm not going to bother
+    // trying to write the code for it

Wouldn't it be better to make no change at all for out of range values? What do
other browsers do when you give a negative selection start or end?

+    // The second MAX is a workaround for the unsigned aspect
+    // if range.location is larger, this will end up 0, instead of a huge
number

Besides these issues, the patch looks really good. Looking forward to the next
take.