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
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
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.
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
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
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.
Comment on attachment 126736 [details] Patch 4 Clearing flags on attachment: 126736 Committed r107555: <http://trac.webkit.org/changeset/107555>
All reviewed patches have been landed. Closing bug.