Bug 75217 – [Forms] Use enum instead of bool for HTMLInputElement::setValue

• Component: Forms
• Status: RESOLVED FIXED

There are two reasons for introducing enum rather than bool.
1. HTMLINputElement::setValue should fire input and change events.
2. Avoid bool constant in code.

HTMLINputElement::setValue should fire event in following way:
1. No event firing
2. Change event
3. Input event and Change event

We can avoid following pattern:
  bool sendChangeEvent = false;
  applyStep(-n, RejectAny, sendChangeEvent, ec);

Created attachment 120519 [details]
Patch 1

[Kent Tamura]

Comment on attachment 120519 [details]
Patch 1

View in context: https://bugs.webkit.org/attachment.cgi?id=120519&action=review

> Source/WebCore/html/HTMLInputElement.cpp:1098
> +    // FIXME: Why do we do this when !eventBehavior?

"!eventBehavior" looks incorrect.

> Source/WebCore/html/HTMLTextFormControlElement.h:37
> +enum TextFieldEventBehavior { DispatchNoEvent, DispatchChangeEvent, DispatchInputEvent, DispatchInputAndChangeEvent };

Please add an explanation why we have four different values to ChangeLog though we replace a bool value.

Created attachment 121207 [details]
Patch 2

[Kent Tamura]

Comment on attachment 121207 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=121207&action=review

> Source/WebCore/ChangeLog:15
> +        Use TextFieldEventBehavior enum instead of sendChangeEvent bool
> +        parameter for HTMLInputElement::setValue method. This new enum
> +        parameter allows us to dispatch input and change events when
> +        user agent populates input field as specified in 
> +        "Common events behavior" of HTML5 standard.
> +
> +        This patch is required for fixing bug 75067 "Forms] Spin buttons
> +        of number input type should fire both input and change event."

"Forms]" -> "[Forms]" 

I think these paragraphs don't explain a concrete reason that TextFieldEventBehavior has four values.
A patch should be minimal in general.  I prefer introducing TextFieldEventBehavior with two values in this patch, and extending it to four values in another patch.

[Hajime Morrita]

Comment on attachment 121207 [details]
Patch 2

Sweeping the review queue. r- due to tkent's comment.

Created attachment 126733 [details]
Patch

Please ignore the patch posted at 2012-02-13 00:56 PST. It contains patch of LayoutTests/ChangeLog. It should not be included.

Created attachment 126734 [details]
Patch

Created attachment 126735 [details]
Patch 3

[Kent Tamura]

Comment on attachment 126735 [details]
Patch 3

View in context: https://bugs.webkit.org/attachment.cgi?id=126735&action=review

> Source/WebCore/html/RadioInputType.cpp:160
> -#if PLATFORM(IOS)
> -    state->indeterminate = element()->indeterminate();
> -
>      if (element()->indeterminate())
>          element()->setIndeterminate(false);
> -#endif
> -
> -    element()->setChecked(true, true);
> +    element()->setChecked(true, DispatchChangeEvent);
>  

please do not remove unrelated code.

> Source/WebCore/html/RadioInputType.cpp:-185
> -
> -#if PLATFORM(IOS)        
>          element()->setIndeterminate(state.indeterminate);
> -#endif
> -

ditto.

> Source/WebCore/html/RadioInputType.cpp:-203
> -#if PLATFORM(IOS)
>      return true;
> -#else
> -    return false;
> -#endif

ditto.

Created attachment 126736 [details]
Patch 4

[Kent Tamura]

Comment on attachment 126736 [details]
Patch 4

View in context: https://bugs.webkit.org/attachment.cgi?id=126736&action=review

ok

> Source/WebCore/html/RangeInputType.cpp:224
> -        bool sendChangeEvent = true;
> -        setValueAsNumber(newValue, sendChangeEvent, ec);
> +        TextFieldEventBehavior eventBehavior = DispatchChangeEvent;
> +        setValueAsNumber(newValue, eventBehavior, ec);

nit: We can remove the 'eventBehavior' variable.

[WebKit Review Bot]

Comment on attachment 126736 [details]
Patch 4

Clearing flags on attachment: 126736

Committed r107555: <http://trac.webkit.org/changeset/107555>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.