[webkit-reviews] review denied: [Bug 3852] typeahead doesn't work in multiple row select boxes. : [Attachment 4036] Patch to enable type select on List Boxes

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Dave Hyatt <hyatt at apple.com>'s request
for review:
Bug 3852: typeahead doesn't work in multiple row select boxes.
http://bugzilla.opendarwin.org/show_bug.cgi?id=3852

Attachment 4036: Patch to enable type select on List Boxes
http://bugzilla.opendarwin.org/attachment.cgi?id=4036&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This patch is great, but I think it still needs a little work:

I don't see any reason for the check for a refcon that's nil -- can't happen.
The check for a box of nil is required of course. But the coding style is not
quite right. The * is not supposed to be next to the type name (see the coding
style document on our website). Also I don't think the extra parentheses around
box->count() make things clearer.

Instead of calling getNSString() and casting to CFStringRef this should be
calling getCFString().

Also, if outString is 0, then we should save code by not calling itemAtIndex at
all (put that code inside the if).

The brace should be on the same line as the if. We use 0 in our code, not NULL
(see coding style document).

There's no need to use "UPP" in the code at all. This code is only used in
Mach-O and so both NewIndexToUCStringUPP and DisposeIndexToUCStringUPP are
no-ops and should be left out entirely. The code can be used as is.

There's no need to cast self to (void*) -- the cast should be left out.

The extra space before "byExtendingSelection" should be omitted.

Even though these are small things, I'm changing the review+ to review- so we
can get them fixed before landing the patch.