[webkit-reviews] review requested: [Bug 9179] Implement select.options.add() method : [Attachment 9339] Patch v4

[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 9339: Patch v4
http://bugzilla.opendarwin.org/attachment.cgi?id=9339&action=edit

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at kilzer.net>
Changes since Patch v3:

(In reply to comment #27)
> (From update of attachment 9325 [edit])
> I'm concerned about adding another level of function call for every caller of

> toInt32, which might be hot. Maybe just repeat the code twice?

I hate duplicating code.  I made the one-argument method inline in value.h.  Is
that okay?

> I think the bool& parameter's purpose is not self-explanatory and so I think
it
> should have a name in the header.

Added.

> I don't see the code to set $maybeOkParam!

This value is set from the return value of TypeCanFailConversion(), which is
the line of code in question below.

> +    return 1; # 0; # or can it?
> 
> The above line of code is confusing, even more than it was before.

Sorry...I left the old code in when first attempting to change the value.  I
removed "# 0; # or can it?" in this patch.