[webkit-reviews] review denied: [Bug 22784] [Chromium] Home and End buttons do not do anything in drop down lists : [Attachment 32582] updated patch

[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 32582: updated patch
https://bugs.webkit.org/attachment.cgi?id=32582&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 45720)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,24 @@
> +2009-07-10  Jens Alfke  <snej at google.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Bug 22784: Home/End and PageUp/PageDn buttons do not do anything in
drop down lists,
> +		on non-Mac platforms (nor in the Mac build of Chromium.)
> +		https://bugs.webkit.org/show_bug.cgi?id=22784
> +		http://code.google.com/p/chromium/issues/detail?id=4576
> +
> +
> +	   No new tests; affects only user input handling, not layout or
rendering.
> +
> +	   * dom/SelectElement.cpp:
> +		Changed definition of ARROW_KEYS_POP_MENU to make it true in
Mac Chromium,
> +		so it will behave compatibly with Mac HI guidelines on pop-up
menus.
> +		It's not possible to have PLATFORM(MAC) be true in the Mac
build of Chromium.
> +	   (WebCore::nextValidIndex): 
> +		New utility fn for traversing items of a select's list.
> +	   (WebCore::SelectElement::menuListDefaultEventHandler):
> +		Added code to handle Home/End and PageUp/PageDn when
ARROW_KEYS_POP_MENU is false.

There are lots of tabs in this ChangeLog entry.  Our SVN pre-commit hook will
be unhappy with these, so it would be best to remove them.

> +#if !ARROW_KEYS_POP_MENU
> +// Returns the index of the next valid list item before or after
|listIndex|.
> +static int nextValidIndex(int listIndex, bool forward, const
Vector<Element*>& listItems)
> +{
> +    int skip = forward ? 1 : -1;

Is there a reason that you switched to a boolean argument rather than the enum
that I suggested?  At the call site you don't have the context of the argument
name to indicate what the true or false represent. An enum value is
understandable at a glance, even at the call site.

The rest of the patch looks good.  It should be landable when these two minor
issues are addressed.