WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch 2
(56.44 KB, patch)
2011-10-08 18:58 PDT
,
Kent Tamura
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Kent Tamura
Comment 1
2011-10-08 07:18:19 PDT
Created
attachment 110270
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug