[webkit-reviews] review denied: [Bug 27658] Webkit ignores PgUp/PgDown/Home/End in SELECT tag objects : [Attachment 87998] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Naoki Takano
<takano.naoki at gmail.com>'s request for review:
Bug 27658: Webkit ignores PgUp/PgDown/Home/End in SELECT tag objects
https://bugs.webkit.org/show_bug.cgi?id=27658

Attachment 87998: Patch
https://bugs.webkit.org/attachment.cgi?id=87998&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=87998&action=review

Nicely done!

We're in the final stretch and the next patch may very well be the last.

Thanks for getting this into shape and addressing all feedback well!

> LayoutTests/fast/events/select-element.html:165
> +function testPageDownWithDisabledElements() {

Why is there no test in this function start starts at 0 or 7?

> LayoutTests/fast/events/select-element.html:173
> +function testPageUpWithDisabledElements() {

Why is there no test in this function start starts at 0 or 7?

> Source/WebCore/dom/SelectElement.cpp:134
> +    int edgeIndex = (direction== SkipForwards) ? 0 : (items.size() - 1);

Need a space after direction.

> Source/WebCore/dom/SelectElement.cpp:789
>		   endIndex = nextSelectableListIndex(data, element,
lastSelectedListIndex(data, element));

For down, the start index is lastSelectedListIndex(data, element)
For up, the start index is optionToListIndex(data, element, selectedIndex(data,
element))

Consider pulling this out of the clauses:

if (keyIdentifier == "Down" || keyIdentifier == "PageDown") {
    int startIndex = lastSelectedListIndex(data, element)
    handled = true;
    if (keyIdentifier == "Down")
	endIndex = nextSelectableListIndex(data, element, startIndex);
    else
	endIndex = nextSelectableListIndexPageAway(data, element, startIndex,
SkipForwards);	  
} else if (keyIdentifier == "Up" || keyIdentifier == "PageUp")	{
    int startIndex = optionToListIndex(data, element, selectedIndex(data,
element))
    handled = true;
    ... similar to above...
}