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

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Neil Rhodes <nrhodes at google.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 33476: Fixes bug 27658
https://bugs.webkit.org/attachment.cgi?id=33476&action=review

------- Additional Comments from David Levin <levin at chromium.org>

> WebCore/dom/SelectElement.cpp (working copy)
> ===================================================================
> +// Returns the index of the next valid list item |skip| items past
|listIndex| in direction |direction|.
This isn't really correct as it could return listIndex or listIndex + 1, etc.
even if skip = 10.


> +// returns the index of the next valid item one page away from |startIndex|
in direction |direction|.

(Many instances) Make comments looks like sentences. (Start with a capital and
end with a period.)


> +int SelectElement::selectableListIndexPageFrom(SelectElementData& data,
Element* element, int startIndex, SkipDirection direction)

I can't make sense of this function name.  It seems like it should stat with
"next".  Perhaps "nextSelectableListIndexPageAway".

Also, after having read through this function it is unclear what it is trying
to do and if it is correct.  I think it attempts to find a "valid index" that
is in this range (startIndex, startIndex + pageSize * direction] but if that
fails, it tries to find a valid index (startIndex + pageSize * direction,
direction  == SkipForwards ? MAXINT : 0], so that's what I'll use as my guide.


> +    int pageSize = static_cast<RenderListBox*>(element->renderer())->size()
- 1;  // -1 so we still show context

1 space before end of line comments.

> + int index = nextValidIndex(items, indexOnePageAway + oppositeDirection,
direction, 1);

Relying on the enum value to be -1, +1 seems like a bad practice here. I know
that "nextValidIndex" does but ideally it is only relied on there.


> +    if (index < 0 || index >= listSize ||(!isOptionElement(items[index]) ||
items[index]->disabled())) {

Add a space after || here: "||(!isOptionElement".
Unnecessary () around the last two items.

At a higher level, this isn't correct because "index" may be valid but not in
this range (startIndex, startIndex + pageSize * direction].

Add a test which exposes this issue.


> +int SelectElement::firstSelectableListIndex(SelectElementData& data,
Element* element)
> +{
> +    const Vector<Element*>& items = data.listItems(element);
> +    int index = 0;
> +    while (index >= 0 && (unsigned) index < items.size() &&
(!isOptionElement(items[index]) || items[index]->disabled()))

(Multiple places) Use c++ style casts.


>  void SelectElement::menuListDefaultEventHandler(SelectElementData& data,
Element* element, Event* event, HTMLFormElement* htmlForm)
>  {
...
> +	   if (endIndex >= 0 && 

Please move the && to the next line.

> +			(keyIdentifier == "Down" || keyIdentifier == "Up" ||
keyIdentifier == "Home"
> +		   ||  keyIdentifier == "End" || keyIdentifier == "PageDown" ||
keyIdentifier == "PageUp")) {

The || should be aligned with the ( from the previous line.


> Index: WebCore/dom/SelectElement.h
> ===================================================================
> +    static int selectableListIndexPageFrom(SelectElementData& data, Element*
element, int startIndex, SkipDirection direction);

Can this be moved out of the class and solely be defined within the
SelectElement.cpp file?



> Index: LayoutTests/fast/events/select-element.html
> ===================================================================
> +    function log(s)

Ideally use whole words for variables: "s"


> +    function sendKeyAndExpectIndex(selectId, key, initialIndex,
expectedIndex) {
...
> +	   if (select.selectedIndex != initialIndex) {
> +	       log("can't set selectedIndex to " + initialIndex + ' (is '
> +	       + select.selectedIndex + ')');

Would be nice to align with (

> +	       return false;
> +	   }
> +	   if (window.layoutTestController)
> +	       eventSender.keyDown(key);
> +	   if (select.selectedIndex != expectedIndex) {
> +	       log('selectedIndex should be ' + expectedIndex + ' (is ' +

In keeping with the moving the &&, etc to the beginning of the next line, move
the + to the next line would be nice.