Summary: | [Forms] Use enum instead of bool for HTMLInputElement::setValue | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | yosin | ||||||||||||||
Component: | Forms | Assignee: | yosin | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | tkent, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 75067 | ||||||||||||||||
Attachments: |
|
Description
yosin
2011-12-25 19:18:30 PST
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. |