RESOLVED FIXED 75217
[Forms] Use enum instead of bool for HTMLInputElement::setValue
https://bugs.webkit.org/show_bug.cgi?id=75217
Summary [Forms] Use enum instead of bool for HTMLInputElement::setValue
yosin
Reported 2011-12-25 19:18:30 PST
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);
Attachments
Patch 1 (33.63 KB, patch)
2011-12-25 19:53 PST, yosin
no flags
Patch 2 (34.12 KB, patch)
2012-01-04 19:19 PST, yosin
no flags
Patch (49.05 KB, patch)
2012-02-13 00:56 PST, yosin
no flags
Patch (35.93 KB, patch)
2012-02-13 00:59 PST, yosin
no flags
Patch 3 (34.80 KB, patch)
2012-02-13 01:02 PST, yosin
no flags
Patch 4 (33.93 KB, patch)
2012-02-13 01:20 PST, yosin
no flags
yosin
Comment 1 2011-12-25 19:53:52 PST
Kent Tamura
Comment 2 2011-12-27 20:01:58 PST
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.
yosin
Comment 3 2012-01-04 19:19:47 PST
Kent Tamura
Comment 4 2012-01-04 20:40:34 PST
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 5 2012-02-02 21:59:52 PST
Comment on attachment 121207 [details] Patch 2 Sweeping the review queue. r- due to tkent's comment.
yosin
Comment 6 2012-02-13 00:56:01 PST
yosin
Comment 7 2012-02-13 00:57:45 PST
Please ignore the patch posted at 2012-02-13 00:56 PST. It contains patch of LayoutTests/ChangeLog. It should not be included.
yosin
Comment 8 2012-02-13 00:59:54 PST
yosin
Comment 9 2012-02-13 01:02:39 PST
Kent Tamura
Comment 10 2012-02-13 01:15:30 PST
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.
yosin
Comment 11 2012-02-13 01:20:22 PST
Kent Tamura
Comment 12 2012-02-13 01:26:31 PST
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 13 2012-02-13 03:01:32 PST
Comment on attachment 126736 [details] Patch 4 Clearing flags on attachment: 126736 Committed r107555: <http://trac.webkit.org/changeset/107555>
WebKit Review Bot
Comment 14 2012-02-13 03:01:37 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.