[webkit-reviews] review denied: [Bug 10934] REGRESSION: prototype.js logs error (HTMLFormElement) on webkit builds : [Attachment 12579] proposed fix

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 10934: REGRESSION: prototype.js logs error (HTMLFormElement) on webkit
builds
http://bugs.webkit.org/show_bug.cgi?id=10934

Attachment 12579: proposed fix
http://bugs.webkit.org/attachment.cgi?id=12579&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+	 select.remove(((HTMLOptionElement*)element)->index());
+    else
+	 select.remove(int(args[0]->toNumber(exec)));

I suggest static_cast syntax on these two lines.

+JSValue* toJS(ExecState* exec, HTMLOptionsCollection* c)

This seems to belong in kjs_html.h/cpp not kjs_dom.h/cpp.

+    for (unsigned i = 0; i < items.size(); ++i) {

If you're rewriting this function anyway, then please consider putting the
items.size() call outside the loop. The compiler might optimize it anyway, but
it might not.

Why do you need to make this change? Won't the bindings handle int for the
length? It's not like we can have lengths that are greater than 31 bits.

+	 [Custom] void	    remove(/* 1 */);

What does the commented out 1 mean here? I find that comment confusing.

+//	   attribute HTMLFrameElementConstructor HTMLFrameElement;
+//	   attribute HTMLFrameSetElementConstructor HTMLFrameSetElement;
+//	   attribute HTMLIFrameElementConstructor HTMLIFrameElement;
+//	   attribute HTMLObjectElementConstructor HTMLObjectElement;
+//	   attribute HTMLTableCaptionElementConstructor
HTMLTableCaptionElement;
+//	   attribute HTMLTableCellElementConstructor HTMLTableCellElement;
+//	   attribute HTMLTableColElementConstructor HTMLTableColElement;
+//	   attribute HTMLTableElementConstructor HTMLTableElement;
+//	   attribute HTMLTableRowElementConstructor HTMLTableRowElement;
+//	   attribute HTMLTableSectionElementConstructor
HTMLTableSectionElement;

These appear in the list, commented out without any indicate of why they're
there or why they're commented out. Please either omit them or make it clearer
what these commented-out lines mean.

The patch generally looks great to me, but I made enough comments about that
I'm going to say review- this round.