[webkit-reviews] review denied: [Bug 27658] Webkit ignores PgUp/PgDown/Home/End in SELECT tag objects : [Attachment 87671] 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 87671: Patch
https://bugs.webkit.org/attachment.cgi?id=87671&action=review

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

There are a few issues I mentioned above which would be nice to fix up.

Also if you fix any bugs, please consider increasing the test coverage to have
caught them. (For example, I think for the PageDown case, the value passed into
nextSelectableListIndexPageAway is incorrect. Also it feels like PageDown
resulting in selecting an item above the currently selected item is wrong.
Ditto for PageUp selecting an item above the current item.)

> Source/WebCore/dom/SelectElement.cpp:78
> +// Returns the closest valid index far |skip| items from |listIndex| in
direction |direction|.

How about:

// Returns the 1st valid item |skip| items from |listIndex| in direction
|direction| if there is one.
// Otherwise, it returns the valid item closest to that boundary which is past
|listIndex| if there is one.
// Otherwise, it returns |listIndex|.
// Valid means that it is enabled and an option element.

> Source/WebCore/dom/SelectElement.cpp:81
> +{

ASSERT(direction == -1 || direction == 1);

> Source/WebCore/dom/SelectElement.cpp:96
> +static int nextSelectableListIndexPageAway(SelectElementData& data, Element*
element, int startIndex, SkipDirection direction)

I think startIndex should be startOptionIndex to clarify that this startIndex
isn't the same as the other start indexes.

> Source/WebCore/dom/SelectElement.cpp:97
> +{

ASSERT(direction == -1 || direction == 1);

> Source/WebCore/dom/SelectElement.cpp:100
> +    int pageSize = static_cast<RenderListBox*>(element->renderer())->size()
- 1; // -1 so we still show context

Use toRenderListBox instead of the cast.

How do you know that the renderer is a RenderListBox?

> Source/WebCore/dom/SelectElement.cpp:104
> +    int indexOnePageAway = SelectElement::optionToListIndex(data, element,
max(0, min(listSize - 1, startIndex + direction * pageSize)));

Why is this offset going through this function as opposed to converting the
startIndex to a list index and then just doing this:
   return nextValidIndex(items, startListIndex, direction, pageSize);

> Source/WebCore/dom/SelectElement.cpp:107
> +    // It means we try to find the index in (startIndex + page * direction,
direction == SkipForward ? MAXINT : 0].

s/page/pageSize/

> Source/WebCore/dom/SelectElement.cpp:108
> +    int index = nextValidIndex(items, indexOnePageAway + (direction ==
SkipForwards ? -1 : 1), direction, 1);

It seems simpler to not do this "(direction == SkipForwards ? -1 : 1)"

> Source/WebCore/dom/SelectElement.cpp:112
> +	   index = nextValidIndex(items, indexOnePageAway, direction ==
SkipForwards ? SkipBackwards : SkipForwards, 1);

This could actually end up being before startIndex which seems weird.

> Source/WebCore/dom/SelectElement.cpp:161
> +    const Vector<Element*>& items = data.listItems(element);

Consider:

int index = nextValidIndex(items, items.size(), SkipBackwards, MAXINT);
if (static_cast<unsigned>(index) == items.size())
  return -1;
return index;

seems simpler and doesn't repeat logic. (slightly less efficient but this won't
be used that often and probably not for a list long enough where the efficiency
will matter).

> Source/WebCore/dom/SelectElement.cpp:172
> +    const Vector<Element*>& items = data.listItems(element);

return nextValidIndex(items, -1, SkipForwards, MAXINT);

as before.

> Source/WebCore/dom/SelectElement.cpp:174
> +	while (index >= 0 && static_cast<unsigned>(index) < items.size() &&
(!isOptionElement(items[index]) || items[index]->disabled()))

indenting is messed up here.

> Source/WebCore/dom/SelectElement.cpp:183
>	   startIndex = items.size();

Consider rewriting in terms of nextValidIndex

> Source/WebCore/dom/SelectElement.cpp:811
> +		   endIndex = nextSelectableListIndexPageAway(data, element,
lastSelectedListIndex(data, element), SkipForwards);

Is it correct to pass in lastSelectedListIndex for startOptionIndex given that
the function is expect an option index?

> Source/WebCore/dom/SelectElement.cpp:835
> +	   if (endIndex >= 0 && (keyIdentifier == "Down" || keyIdentifier ==
"Up" || keyIdentifier == "Home" ||  keyIdentifier == "End" || keyIdentifier ==
"PageDown" || keyIdentifier == "PageUp")) {

Hmmm this if got a bit ugly and seems fragile.

Consider (I'm not sure if these are better or not or how easy they are to do so
use your judgement):
1. use a bool handled to indicate that the endIndex should be set. I think
there is another function in this file that does that. OR
2. can endIndex have a value of -1 and only get set to >= 0 if it should be
used in here.