[webkit-reviews] review denied: [Bug 3503] form.elements[] not order-preserving when using javascript DOM interface : [Attachment 2356] Fix

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Anders Carlsson <andersca at mac.com>'s
request for review:
Bug 3503: form.elements[] not order-preserving when using javascript DOM
interface
http://bugzilla.opendarwin.org/show_bug.cgi?id=3503

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

------- Additional Comments from Darin Adler <darin at apple.com>
This patch looks good. I'm increasingly uncomfortable with the form element
class using vectors and inserting and removing elements from the middle of the
vectors. There's no obvious benefit of a vector over a list, and a list
supports high speed insertion and deletion. Except for the one use of this as a
vector in HTMLFormCollectionImpl::item. But I think that need not prevent us
from taking this patch; it's not even obvious what we should do.

But I'm concerned with another aspect of this patch.

I think that getFormElementByIndex might need to check the form() on the
element before counting it when computing the index. If for some reason the
form element is not associated with this form, we could end up with an index
that is past the end of the form element vector, with disastrous results. I'm
not sure if that case can arise, but I'm also not sure that it can't, and the
code change is very simple. Please fix that in the patch.

As far as the comments about linear vs. binary search, if you look at the
Mozilla code you'll see that inside that binary search they are doing an
operation that compares the relative positions of the DOM nodes. This operation
is going to be proportional to the tree distances between successive nodes. The
approach of iterating the entire DOM tree under the form element should be
relatively similar in complexity, and there's no real option to use a binary
search here.

On the other hand, there's a chance that the Mozilla code will handle cases
better where the form elements are not inside the <form> element. It's worth
making test cases of this sort to find that out, but we can file a new bug once
we do that if this fix has already been landed.