[webkit-reviews] review requested: [Bug 9179] Implement select.options.add() method : [Attachment 8941] Patch v1 (for review only!)

[bugzilla-request-daemon at opendarwin.org]

David Kilzer (ddkilzer) <ddkilzer at kilzer.net> has asked  for review:
Bug 9179: Implement select.options.add() method
http://bugzilla.opendarwin.org/show_bug.cgi?id=9179

Attachment 8941: Patch v1 (for review only!)
http://bugzilla.opendarwin.org/attachment.cgi?id=8941&action=edit

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at kilzer.net>
This is a first-pass patch at the options.add() implementation.  I'm only
looking for feedback now, and I expect this patch to get an r-.  This patch
should NOT be applied to SVN.

Changes:

- Removed poorly-named HTMLSelectCollection class from kjs_html.cpp.  Added IDL
for HTMLOptionsCollection.
- Fixed names of "HTMLCollectionProto*" items in kjs_html.cpp to add "JS"
prefix to match naming conventions.
- Added new "HasCustomIndexSetter" to CodeGenereratorJS.pm which expects an
indexSetter() method to be available on the JS object.  (I don't really like
the name "HasCustomIndexSetter" because there is no "HasIndexSetter" default
implementation, but I wasn't sure if that would happen when HTMLCollection was
extracted from kjs_html.cpp.)
- Added new lookupPutWithIndex() template in lookup.h (blatantly copied from
lookupPut() template).  I don't like the code duplication, but wasn't sure of a
better way to do this off the top of my head.
- All current layout tests pass with this patch; only the new options.add()
feature is not fully tested (see below).

Known issues:

- Layout tests are not complete and are still failing in some cases.  I will
flesh them out per the MS spec and do more testing on Firefox and MSIE to make
sure I match their behavior.
- There seems to be some code duplication between the HTMLSelect and
HTMLCollection regarding the management of the Option element list.  I'm
wondering if I should clean that up while I'm digging around in this part of
the code?