[webkit-reviews] review denied: [Bug 49308] Handle Page up/down and home/end keys in list box controls : [Attachment 73880] Updated patch now including tests

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 49308: Handle Page up/down and home/end keys in list box controls
https://bugs.webkit.org/show_bug.cgi?id=49308

Attachment 73880: Updated patch now including tests
https://bugs.webkit.org/attachment.cgi?id=73880&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=73880&action=review

Thanks for tackling this. Looks pretty good. There are a few improvements I’d
like to see before you land.

> WebCore/ChangeLog:6
> +	   Handle Page up/down and home/end keys in list box controls
> +	   https://bugs.webkit.org/show_bug.cgi?id=49308

This patch says its adding new keys, but that misrepresents it. It also changes
the behavior of existing keys; you need to explain why the new algorithm is
better. It's possible you are now doing the right thing for some platforms and
the wrong thing for others.

> WebCore/dom/SelectElement.cpp:762
> +	       endIndex = nextSelectableListIndex(data, element,
> +						 
(data.activeSelectionEndIndex() < 0) ?
> +						  lastSelectedListIndex(data,
element) :
> +						 
data.activeSelectionEndIndex());

Multiple-line function calls should not be indented so the arguments line up
with the parenthesis opening the argument list in WebKit code.

Multiple line if statement bodies need braces in WebKit coding style. It’s
based on lines of source code, not statements, so braces are needed here.

I suggest making helper functions so you don’t have to put these long ? :
expressions here.

> WebCore/dom/SelectElement.cpp:776
> +	       int widowSize =
toRenderListBox(element->renderer())->numVisibleItems() - 1;
> +	       int nextIndex = (data.activeSelectionEndIndex() < 0) ?
lastSelectedListIndex(data, element) : data.activeSelectionEndIndex();
> +
> +	       endIndex = nextSelectableListIndex(data, element, min((int)
listItems.size() - 1, nextIndex + widowSize - 1));

This code needs some “why” comments. The term “widow size” is also not all that
clear. Is it the size of a “widow”? I don’t think so, so the name is not great.


I suggest making some helper functions so the code is easier to read.

We do not use C-style casts, so you’ll want to use static_cast or find some way
to avoid the typecasting.

> WebCore/dom/SelectElement.cpp:789
> -	   if (keyIdentifier == "Down" || keyIdentifier == "Up") {
> +	   if (endIndex != -1) {

Using a special value of -1 is not a great way to handle this. A separate
boolean is probably a better idea. Or if you do want a special value, then
please use a name for it rather than just -1.