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

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request 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 Darin Adler <darin at apple.com>
Nice!

There's code here that says WebCore::Node inside namespace WebCore -- no need
for that. Also formatted wrong (* should be next to node).

Why is kjs_binding.h now including Node.h? I'd like to avoid that if at all
possible! Can the .cpp file that depends on Node.h include it instead, or does
the header now somehow depend on it?

+    // FIXME: Does this work without the if/else statement?

What does that comment mean?

+	 select->setOption(unsigned(index),
static_cast<HTMLOptionElement*>(node.get()), ec);

I'm not fond of the cast syntax here. Also, what does this function do if the
node is not an option element? Casting an object that's not of the right type
can end up creating memory-trashing bugs, so there needs to be some level of
code that checks (or somehow guarantees) the type before casting.

+    int selectedIndex();

Should be const.

+    void setSelectedIndex(int index);

Should omit the word "index" here.

I think it's a little ugly to have to add a new variant of lookupPut here.
Especially as it hard-wires the name indexSetter, which doesn't exist anywhere
in KJS. One possibility would be to make a version of lookupPut that just
returns a boolean, false, if it's not found in the lookup table. Then, the call
to indexSetter could be in the WebCore (generated) code. I think that would be
a little cleaner, although perhaps a tiny bit slower since there'd be an
additional if statement.

+    HTMLOptionsCollection* impl =
static_cast<HTMLOptionsCollection*>(JSHTMLOptionsCollection::impl());

How about using a different name for the local variable than the function so we
can avoid that big JSHTMLOptionsCollection::impl() mouthful? Maybe "imp"? And
maybe the generated code could get the type right so we wouldn't need casting
and local variables?

+	 newLength = unsigned(floor(lengthValue));

I know this was not new code, but the call to floor here is unnecessary, since
floor is what a cast to unsigned will do when the number is greater than or
equal to zero. It also would be good to have a check for a value too large to
fit in an unsigned and do something predictable in that case rather than just
truncating the number.

+    if (value->isUndefinedOrNull()) {
+	 base->remove(index);
+    } else {

We don't use braces around one-line if statements.

Why does getHTMLOptionsCollection still take an HTMLCollection parameter
instead of an HTMLOptionsCollection parameter? What if the caller passes an
HTMLCollection that's not an HTMLOptionsCollection? Again, casting in such
cases is dangerous.