Many arguments to setSelectedIndex take boolean constants. Change them to use enums instead of bools.
Created attachment 111459 [details] Patch
Comment on attachment 111459 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111459&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) Please remove this. cq- because of this line. > Source/WebCore/html/HTMLSelectElement.cpp:388 > + selectOption(index, LeaveOtherOptionsSelected, DoNotDispatchChangeEvent, NotUserDriven); We can call setSelectedIndex(index, LeaveOtherOptionsSelected); here. > Source/WebCore/html/HTMLSelectElement.cpp:766 > + m_isProcessingUserDrivenChange = userDrivenChange; I'd prefer explicitly comparing userDrivenChange to UserDriven rather than implicitly coercing the enum to bool. > Source/WebCore/html/HTMLSelectElement.cpp:1010 > + selectOption(listToOptionIndex(listIndex), DeselectOtherOptions, DoNotDispatchChangeEvent, UserDriven); Seems like you can just call setSelectedIndex(listToOptionIndex(listIndex), DeselectOtherOptions); > Source/WebCore/html/HTMLSelectElement.h:39 > +enum ShouldDeselectOtherOptions { LeaveOtherOptionsSelected, DeselectOtherOptions }; I'd prefer calling it KeepOtherOptionsSelectedIfAllowed given that selectOption can ignore this option when m_multiple is false.
Good suggestions. I’ll deal with this when I get back to the computer that happens to already have this patch on it.
Comment on attachment 111459 [details] Patch I’ve got a new better version of this that I’ll put up for review that goes a little bit further in renaming and refactoring as well as fixing everything Ryosuke mentioned.
Created attachment 112152 [details] Patch
Comment on attachment 112152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112152&action=review > Source/WebCore/ChangeLog:3 > + OBSOLETE CHANGE LOG NEEDS TO BE UPDATED (OOPS) Please update the change log. > Source/WebCore/html/HTMLSelectElement.cpp:749 > + selectOption(option->index(), DeselectOnlyToMaintainSingleOptionInvariant, DoNotDispatchChangeEvent, NotUserDriven); Hm... DeselectOnlyToMaintainSingleOptionInvariant seems too verbose to me. How about DeselectIfMultiSelectIsDisallowed or DeselectIfOnlySingleSelectionIsAllowed (including their variants)? > Source/WebCore/html/HTMLSelectElement.cpp:754 > +void HTMLSelectElement::selectOption(int optionIndex, ShouldDeselectOtherOptions deselectOptions, ShouldDispatchChangeEvent shouldDispatchChangeEvent, UserDrivenFlag userDrivenFlag) "flag" makes me think of bit flags. I'd prefer calling it EUserDriven. Also I just recalled that FrameSelection already defines EUserTriggered. Maybe we should move that to EUserTriggered.h and reuse it here? > Source/WebKit/chromium/ChangeLog:3 > + OBSOLETE CHANGE LOG NEEDS TO BE UPDATED (OOPS) Please update the change log.
(In reply to comment #6) > > + selectOption(option->index(), DeselectOnlyToMaintainSingleOptionInvariant, DoNotDispatchChangeEvent, NotUserDriven); > > Hm... DeselectOnlyToMaintainSingleOptionInvariant seems too verbose to me. How about DeselectIfMultiSelectIsDisallowed or DeselectIfOnlySingleSelectionIsAllowed (including their variants)? So far I don’t like any of these, including the one I used. Note that they are all about 6 or 7 words. I do think the “only” is better than “if”. The ones with if don’t indicate what will be deselected very well. > > +void HTMLSelectElement::selectOption(int optionIndex, ShouldDeselectOtherOptions deselectOptions, ShouldDispatchChangeEvent shouldDispatchChangeEvent, UserDrivenFlag userDrivenFlag) > > "flag" makes me think of bit flags OK. This is a bit flag; just a single one. But sure, I will stay away from flag. > I'd prefer calling it EUserDriven. I am not a fan of that “E”. > Also I just recalled that FrameSelection already defines EUserTriggered. Sure, would be good to share that. Although there may be some subtle semantic difference.
Are you intending to upload a new patch for review? Or is the attachment 112152 [details] supposed to have r? ?
(In reply to comment #8) > Are you intending to upload a new patch for review? Yes. Not for a while, though. > Or is the attachment 112152 [details] supposed to have r? ? No. I hadn’t written change log yet, so didn’t put it up for review yet.
I think I should use a single set of flags instead of three separate arguments.
Created attachment 112441 [details] Patch
Still not up for review, still no change log.
(In reply to comment #10) > I think I should use a single set of flags instead of three separate arguments. This looks good!
Created attachment 113244 [details] Patch
Comment on attachment 113244 [details] Patch The patch improves the code readability very much!
Comment on attachment 113244 [details] Patch Clearing flags on attachment: 113244 Committed r99035: <http://trac.webkit.org/changeset/99035>
All reviewed patches have been landed. Closing bug.