[webkit-reviews] review denied: [Bug 22784] [Chromium] Home and End buttons do not do anything in drop down lists : [Attachment 32577] Fixes bug 22784

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied Jens Alfke <snej at google.com>'s
request for review:
Bug 22784: [Chromium] Home and End buttons do not do anything in drop down
lists
https://bugs.webkit.org/show_bug.cgi?id=22784

Attachment 32577: Fixes bug 22784
https://bugs.webkit.org/attachment.cgi?id=32577&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
> -#if PLATFORM(MAC)
> +// On Mac (Safari or Chrome), pop up the menu if the user presses the arrow
keys
> +// while a Select element has focus. On other platforms, just change the
selection.
> +#if PLATFORM(DARWIN) && !PLATFORM(IPHONE)
>  #define ARROW_KEYS_POP_MENU 1
>  #else
>  #define ARROW_KEYS_POP_MENU 0

Another minor comment: Mac WebKit is more than just Safari, so it doesn't makes
too much sense to mention Safari here.

> +#if !ARROW_KEYS_POP_MENU
> +// Returns the index of the next valid list item in direction |dir| past
|listIndex|.
> +static int nextValidIndex(int listIndex, int dir, const Vector<Element*>&
listItems) {
> +    ASSERT(dir!=0);
> +    int size = listItems.size();
> +    for (listIndex += dir; listIndex >= 0 && listIndex < size; listIndex +=
dir) {
> +	   if (!listItems[listIndex]->disabled() &&
isOptionElement(listItems[listIndex]))
> +	       return listIndex;
> +    }
> +    return -1;
> +}
> +#endif

There are some minor style issues here (brace on same line as function
definition, lack of whitespace around operators).  We also prefer not to
unnecessarily abbreviate variable names such as "dir".	It may be clearer to
use an enum rather than magic values such as -1 and 1 to represent up and down
as the directions.