Bug 69701 - Remove redundant code of HTMLSelectElement.
Summary: Remove redundant code of HTMLSelectElement.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Minor
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 69621
Blocks: 69755
  Show dependency treegraph
 
Reported: 2011-10-08 07:16 PDT by Kent Tamura
Modified: 2011-10-10 06:20 PDT (History)
3 users (show)

See Also:


Attachments
Patch (56.70 KB, patch)
2011-10-08 07:18 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (56.44 KB, patch)
2011-10-08 18:58 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2011-10-08 07:16:41 PDT
Remove redundant code of HTMLSelectElement.
Comment 1 Kent Tamura 2011-10-08 07:18:19 PDT
Created attachment 110270 [details]
Patch
Comment 2 Ryosuke Niwa 2011-10-08 10:38:07 PDT
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.
Comment 3 Kent Tamura 2011-10-08 18:36:22 PDT
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.
Comment 4 Kent Tamura 2011-10-08 18:58:16 PDT
Created attachment 110287 [details]
Patch 2

Addressed the deselectItems() issue.
Comment 5 WebKit Review Bot 2011-10-09 20:24:44 PDT
Comment on attachment 110287 [details]
Patch 2

Clearing flags on attachment: 110287

Committed r97038: <http://trac.webkit.org/changeset/97038>
Comment 6 WebKit Review Bot 2011-10-09 20:24:48 PDT
All reviewed patches have been landed.  Closing bug.