Bug 69518 – Remove SelectElement class

• Component: DOM
• Status: RESOLVED FIXED

[Kent Tamura]

SelectElement abstraction for HTMLSelectElement and WMLSelectElement is not needed anymore because WMLSelectElement was removed.

[Kent Tamura]

Created attachment 109948 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 109948 [details]
Patch

Attachment 109948 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9977245

[Ryosuke Niwa]

Comment on attachment 109948 [details]
Patch

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?

[Kent Tamura]

Comment on attachment 109948 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109948&action=review

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

done.

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1149
>> +        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.

done.

>> Source/WebCore/html/HTMLSelectElement.cpp:675
>> +        cachedStateForActiveSelection.append(optionElement && optionElement->selected());
> 
> Seems like these two lines can be merged into one line?

I don't think so.

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

Fixed.

>> Source/WebCore/html/HTMLSelectElement.cpp:912
>> +    }
> 
> Huh... this loop is almost identical to the one in listToOptionIndex except it's written in such a convoluted way.

I don't think so.

>> 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.

done.

>> Source/WebCore/html/HTMLSelectElement.cpp:1106
>> +        bool handled = false;
> 
> Can we initialize this to true?

Done.

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

Done.

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

I agree.  I'll handle later.

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

I removed this comment.

>> Source/WebCore/html/HTMLSelectElement.cpp:1449
>> +            }
> 
> In a follow up, we should iterate through options backwards so that we wouldn't need these two variables.

I added a FIXME comment.

>> Source/WebCore/rendering/RenderListBox.cpp:168
>> +    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?

I'll handle it later.
IMO, RenderListBox and RenderMenuList should have selectElement() member function.

[Kent Tamura]

Created attachment 110083 [details]
Patch for landing

Windows and GTK build fixes, and so on.

[Gustavo Noronha (kov)]

Comment on attachment 110083 [details]
Patch for landing

Attachment 110083 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10010097

[Kent Tamura]

Committed r96907: <http://trac.webkit.org/changeset/96907>