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

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jens Alfke <snej at chromium.org>'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 32590: new patch
https://bugs.webkit.org/attachment.cgi?id=32590&action=review

------- Additional Comments from David Levin <levin at chromium.org>
It is best not to assign your patch to be reviewed by a particular person. 
You'll get more attention usually if your patch is in the general queue.

There were a few things that Mark suggested which would allow you to get closer
to getting this checked in faster:

1. If you had a patch and a bug just for the define change for
ARROW_KEYS_POP_MENU.  I'd r+ it right now and it would be a small focused patch
that didn't attempt to do more than one thing.	This is a good practice in
general. As it makes reviews go quicker and keeping patches focused also helps
with future maintainence when people look at the history and try to understand
what patches were trying to accomplish, etc. so while it may be ten minutes
more work for you. It is a good thing.

2. WebKit is adopting a new style guide (that isn't yet in the written
guidelines) to avoid bool parameters to functions. This is a really good thing
for several reasons: 
a. It keeps the code more readable (What does "true/false" mean when you see it
at a calling site?)
b. It helps with future changes to avoid breakages.  (Once when we picked up a
new revision of WebKit code in chromium things broke because a new bool
parameter was inserted in the middle of other bool parameters.	Now clearly
this doesn't seem like the best thing to do but if they had been enums the
problem would have been quickly detected and corrected instead of taking hours
to track down.)

While your function seems small and you may feel that it isn't worth it to add
an enum for this function, it isn't hard, and it could help with future
maintenance.



> +static int nextValidIndex(const Vector<Element*>& listItems, int listIndex,
bool forward = true)

Change bool forward to an enum as noted above.

> +	   if (keyIdentifier == "Down" || keyIdentifier == "Right" ||
keyIdentifier == "PageDown") {
> +	       listIndex = nextValidIndex(listItems, listIndex);

Making "PageDown" go down only one item seems incorrect.

> +	   } else if (keyIdentifier == "Up" || keyIdentifier == "Left" ||
keyIdentifier == "PageUp") {
> +	       listIndex = previousValidIndex(listItems, listIndex);
> +	       handled = true;

Making "PageUp" go up only one item seems incorrect.