RESOLVED FIXED 69701
Remove redundant code of HTMLSelectElement.
https://bugs.webkit.org/show_bug.cgi?id=69701
Summary Remove redundant code of HTMLSelectElement.
Kent Tamura
Reported 2011-10-08 07:16:41 PDT
Remove redundant code of HTMLSelectElement.
Attachments
Patch (56.70 KB, patch)
2011-10-08 07:18 PDT, Kent Tamura
no flags
Patch 2 (56.44 KB, patch)
2011-10-08 18:58 PDT, Kent Tamura
no flags
Kent Tamura
Comment 1 2011-10-08 07:18:19 PDT
Ryosuke Niwa
Comment 2 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.
Kent Tamura
Comment 3 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.
Kent Tamura
Comment 4 2011-10-08 18:58:16 PDT
Created attachment 110287 [details] Patch 2 Addressed the deselectItems() issue.
WebKit Review Bot
Comment 5 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>
WebKit Review Bot
Comment 6 2011-10-09 20:24:48 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.