[Webkit-unassigned] [Bug 49308] Handle Page up/down and home/end keys in list box controls

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49308


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #73880|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #7 from Darin Adler <darin at apple.com>  2010-12-29 17:04:05 PST ---
(From update of attachment 73880)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.