[webkit-reviews] review granted: [Bug 70184] Change HTMLSelectElement::setSelectedIndex to use enums instead of bools : [Attachment 111459] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 70184: Change HTMLSelectElement::setSelectedIndex to use enums instead of
bools
https://bugs.webkit.org/show_bug.cgi?id=70184

Attachment 111459: Patch
https://bugs.webkit.org/attachment.cgi?id=111459&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.