WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch 2
(34.12 KB, patch)
2012-01-04 19:19 PST
,
yosin
no flags
Details
Formatted Diff
Diff
Patch
(49.05 KB, patch)
2012-02-13 00:56 PST
,
yosin
no flags
Details
Formatted Diff
Diff
Patch
(35.93 KB, patch)
2012-02-13 00:59 PST
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 3
(34.80 KB, patch)
2012-02-13 01:02 PST
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 4
(33.93 KB, patch)
2012-02-13 01:20 PST
,
yosin
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
yosin
Comment 1
2011-12-25 19:53:52 PST
Created
attachment 120519
[details]
Patch 1
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
Created
attachment 121207
[details]
Patch 2
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
Created
attachment 126733
[details]
Patch
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
Created
attachment 126734
[details]
Patch
yosin
Comment 9
2012-02-13 01:02:39 PST
Created
attachment 126735
[details]
Patch 3
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
Created
attachment 126736
[details]
Patch 4
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.
Top of Page
Format For Printing
XML
Clone This Bug