[webkit-reviews] review granted: [Bug 69518] Remove SelectElement class : [Attachment 109948] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Kent Tamura <tkent at chromium.org>'s
request for review:
Bug 69518: Remove SelectElement class
https://bugs.webkit.org/show_bug.cgi?id=69518

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109948&action=review


r=me provided builds are fixed before you land.

> Source/WebCore/ChangeLog:8
> +	   SelectElement used to be an abstraction class for

Nit: abstract class.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1149
> -	   SelectElement* selectNode =
toSelectElement(static_cast<Element*>(m_renderer->node()));
> +	   HTMLSelectElement* selectNode =
static_cast<HTMLSelectElement*>(m_renderer->node());

I'd like to see a type check here just to be safe. You can add
toHTMLSelectElement.

> Source/WebCore/html/HTMLSelectElement.cpp:675
> +	   OptionElement* optionElement = toOptionElement(items[i]);
> +	   cachedStateForActiveSelection.append(optionElement &&
optionElement->selected());

Seems like these two lines can be merged into one line?

> Source/WebCore/html/HTMLSelectElement.cpp:729
> +	   bool selected = optionElement &&  optionElement->selected();

Nit: 2 spaces after &&.

> Source/WebCore/html/HTMLSelectElement.cpp:912
> +    int optionIndex2 = -1;
> +    for (int listIndex = 0; listIndex < listSize; ++listIndex) {
> +	   if (isOptionElement(items[listIndex])) {
> +	       ++optionIndex2;
> +	       if (optionIndex2 == optionIndex)
> +		   return listIndex;
> +	   }
> +    }

Huh... this loop is almost identical to the one in listToOptionIndex except
it's written in such a convoluted way.

> Source/WebCore/html/HTMLSelectElement.cpp:945
> +    // made. This matches other browsers' behavior.

Nit: please put "made." on the previous line to avoid the awkward wrapping.

> Source/WebCore/html/HTMLSelectElement.cpp:1106
> +	   bool handled = false;

Can we initialize this to true?

> Source/WebCore/html/HTMLSelectElement.cpp:1131
> +	   }

And then add an else clause here and set it to false? I'm also surprised that
key identifier isn't given as an integer.

> Source/WebCore/html/HTMLSelectElement.cpp:1231
> +					   bool multi, bool shift)

Nit: wrong indentation.

> Source/WebCore/html/HTMLSelectElement.cpp:1282
> +void HTMLSelectElement::listBoxDefaultEventHandler(SelectElementData& data,
Element* element, Event* event, HTMLFormElement* htmlForm)

This function needs a massive refactoring :(

> Source/WebCore/html/HTMLSelectElement.cpp:1440
> +    // return the number of the last option selected

Nit: s/number/index/

> Source/WebCore/html/HTMLSelectElement.cpp:1449
> +    for (size_t i = 0; i < items.size(); ++i) {
> +	   if (OptionElement* optionElement = toOptionElement(items[i])) {
> +	       if (optionElement->selected()) {
> +		   index = i;
> +		   found = true;
> +	       }

In a follow up, we should iterate through options backwards so that we wouldn't
need these two variables.

> Source/WebCore/rendering/RenderListBox.cpp:168
> -    SelectElement* select = toSelectElement(static_cast<Element*>(node()));
> +    HTMLSelectElement* select =
toSelectElement(static_cast<Element*>(node()));

It looks silly to cast node to Element* and then call toSelectElement. Can we
make toSelectElement take Node* instead?