Bug 70184 – Change HTMLSelectElement::setSelectedIndex to use enums instead of bools

• Component: Forms
• Status: RESOLVED FIXED

[Darin Adler]

Many arguments to setSelectedIndex take boolean constants. Change them to use enums instead of bools.

[Darin Adler]

Created attachment 111459 [details]
Patch

[Ryosuke Niwa]

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.

[Darin Adler]

Good suggestions. I’ll deal with this when I get back to the computer that happens to already have this patch on it.

[Darin Adler]

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.

[Darin Adler]

Created attachment 112152 [details]
Patch

[Ryosuke Niwa]

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.

[Darin Adler]

(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.

[Ryosuke Niwa]

Are you intending to upload a new patch for review? Or is the attachment 112152 [details] supposed to have r? ?

[Darin Adler]

(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.

[Darin Adler]

I think I should use a single set of flags instead of three separate arguments.

[Darin Adler]

Created attachment 112441 [details]
Patch

[Darin Adler]

Still not up for review, still no change log.

[Kent Tamura]

(In reply to comment #10)
> I think I should use a single set of flags instead of three separate arguments.

This looks good!

[Darin Adler]

Created attachment 113244 [details]
Patch

[Kent Tamura]

Comment on attachment 113244 [details]
Patch

The patch improves the code readability very much!

[WebKit Review Bot]

Comment on attachment 113244 [details]
Patch

Clearing flags on attachment: 113244

Committed r99035: <http://trac.webkit.org/changeset/99035>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.