Bug 69701 – Remove redundant code of HTMLSelectElement.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Kent Tamura]

Remove redundant code of HTMLSelectElement.

[Kent Tamura]

Created attachment 110270 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 110270 [details]
Patch

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

r- because of two behavioral changes. Otherwise the patch looks good.

> Source/WebCore/html/HTMLSelectElement.cpp:724
> +void HTMLSelectElement::setSelectedIndexInternal(int optionIndex, bool deselect, bool fireOnChangeNow, bool userDrivenChange)

I think we can just keep the function name and overload the other one.

> Source/WebCore/html/HTMLSelectElement.cpp:836
> +    setNeedsValidityCheck();

Why are we adding this function call here? It'll be best to separate this behavioral change.

> Source/WebCore/html/HTMLSelectElement.cpp:1301
> +    HTMLFormControlElementWithState::defaultEventHandler(event);

Again, we should really separate this behavioral change.

[Kent Tamura]

Comment on attachment 110270 [details]
Patch

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

> Source/WebCore/html/HTMLSelectElement.cpp:-369
> -    HTMLFormControlElementWithState::defaultEventHandler(event);

This line was moved to another defaultEventHandler().

>> Source/WebCore/html/HTMLSelectElement.cpp:724
>> +void HTMLSelectElement::setSelectedIndexInternal(int optionIndex, bool deselect, bool fireOnChangeNow, bool userDrivenChange)
> 
> I think we can just keep the function name and overload the other one.

I don't think so.
We had two setSelectedIndex():
    void setSelectedIndex(int index, bool deselect = true);
    static void setSelectedIndex(SelectElementData&, Element*, int optionIndex, bool deselect = true, bool fireOnChangeNow = false, bool userDrivenChange = true);
If we simply made the later non-static, it would be:
    void setSelectedIndex(int optionIndex, bool deselect = true, bool fireOnChangeNow = false, bool userDrivenChange = true);
Compilers can't determine which setSelectedIndex() should be used if a callsite is setSelectedIndex(foo) or setSelectedIndex(foo, true/false).

The former setSelectedIndex() is a simple wrapper of the later setSelectedIndex(). But I don't like to remove the former setSelectedIndex() and make the later setSelectedIndex() public because the later setselectedIndex() has arguments to control low-level behaviors.

>> Source/WebCore/html/HTMLSelectElement.cpp:836
>> +    setNeedsValidityCheck();
> 
> Why are we adding this function call here? It'll be best to separate this behavioral change.

This setNeedsValidityCheck() call is a result of merger with another deselectItems(), which had setNeedsValidityCheck().
However you're right.  We have deselectItems() callers other than the another deselectItems(). This change brings extra setNeedsValidityCheck() calls.

>> Source/WebCore/html/HTMLSelectElement.cpp:1301
>> +    HTMLFormControlElementWithState::defaultEventHandler(event);
> 
> Again, we should really separate this behavioral change.

This is a result of merger with another defaultEventHandler().  It's not a behavior change.

[Kent Tamura]

Created attachment 110287 [details]
Patch 2

Addressed the deselectItems() issue.

[WebKit Review Bot]

Comment on attachment 110287 [details]
Patch 2

Clearing flags on attachment: 110287

Committed r97038: <http://trac.webkit.org/changeset/97038>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.