Remove redundant code of HTMLSelectElement.
Created attachment 110270 [details] Patch
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 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.
Created attachment 110287 [details] Patch 2 Addressed the deselectItems() issue.
Comment on attachment 110287 [details] Patch 2 Clearing flags on attachment: 110287 Committed r97038: <http://trac.webkit.org/changeset/97038>
All reviewed patches have been landed. Closing bug.