[webkit-reviews] review denied: [Bug 22784] [Chromium] Home and End buttons do not do anything in drop down lists : [Attachment 32594] Proposed fix for bug 22784

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Neil Rhodes <nrhodes at google.com>'s
request for review:
Bug 22784: [Chromium] Home and End buttons do not do anything in drop down
lists
https://bugs.webkit.org/show_bug.cgi?id=22784

Attachment 32594: Proposed fix for bug 22784
https://bugs.webkit.org/attachment.cgi?id=32594&action=review

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

1. Please create a separate bug for your patch (or else this is going to get
confusing fast).
2. This needs a changelog.  Run prepare-ChangeLog --bug=YourNewBugNumber.
3. It would be great if you used the functions that are being created in the
other patch on this bug rather than have basically the same code duplicated
here.
4. Please make comments look like sentences.  Start with a capital and end with
a period.
5. It looks like you added several static functions to the class. It doesn't
look like they need to be exposed on the class and could instead be static
functions in the file. 

> Index: WebCore/dom/SelectElement.cpp

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

Avoid bool parameters. Use an enum instead. See my comment for the other patch
on this bug.

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

1 space before end of line comments.

> +    int indexOnePageAway = optionToListIndex(data, element, max(0,
min(listSize -1, startIndex +  (pageDown ? pageSize : -pageSize))));

There is no space after "listSize -"
There are two spaces after "startIndex + "

> +    while (index >= 0 && index < listSize && (!isOptionElement(items[index])
|| items[index]->disabled()))
> +	   pageDown ? ++index : --index;

Ideally this would use the function from the other patch -- perhaps wait until
after that patch is checked in.


> +    if (index < 0 || (unsigned) index == items.size()) {

Use c++ style casts.

> +	   while (index >= 0 && index < listSize &&
(!isOptionElement(items[index]) || items[index]->disabled()))
> +	       pageDown ? --index : ++index;

Ideally this would use the function from the other patch.


> +int SelectElement::firstSelectableListIndex(SelectElementData& data,
Element* element)

Use the function from the other patch.

> +int SelectElement::lastSelectableListIndex(SelectElementData& data, Element*
element)

Use the function from the other patch.

>	       else if (keyIdentifier == "Up")
>		   endIndex = previousSelectableListIndex(data, element,
optionToListIndex(data, element, selectedIndex(data, element)));
...
> +	       else if (keyIdentifier == "PageUp")
> +		   endIndex = selectableListIndexPageFrom(data, element,
selectedIndex(data, element), false);

Why is this using selectedIndex(data, element) instead of what "Up" does:
optionToListIndex(data, element, selectedIndex(data, element)) ?

> +	   if (endIndex >= 0 && 
Boolean expressions at the same nesting level that span multiple lines should
have their operators on the left side of the line instead of the right side.

So please move the && to the next line.

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

You have some TABs here which is why the indentation looks so bad.