Bug 78873 - [Forms] Integrate InputType::dispatchChangeEventInResponseToSetValue into InputType::setValue
Summary: [Forms] Integrate InputType::dispatchChangeEventInResponseToSetValue into Inp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on:
Blocks: 75067
  Show dependency treegraph
 
Reported: 2012-02-16 21:28 PST by yosin
Modified: 2012-02-17 01:23 PST (History)
2 users (show)

See Also:


Attachments
Patch 1 (5.31 KB, patch)
2012-02-16 22:52 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 1 (6.30 KB, patch)
2012-02-16 23:45 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 3 (7.36 KB, patch)
2012-02-17 00:02 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 4 (7.95 KB, patch)
2012-02-17 00:27 PST, yosin
no flags Details | Formatted Diff | Diff
Patch 5 (7.94 KB, patch)
2012-02-17 00:33 PST, yosin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-02-16 21:28:12 PST
A virtual method InputType::dispatchChangeEventInResponseToSetValue is
* there is only one call site in HTMLInputElement::setValue
* there are two implementations in InputType and TextFiledInputType.

Merging this method into HTMLInputElement::setValue makes our code base simpler and helps to implement input/change event dispatch in Spin button (https://bugs.webkit.org/show_bug.cgi?id=75067)
Comment 1 yosin 2012-02-16 22:52:00 PST
Created attachment 127526 [details]
Patch 1
Comment 2 Kent Tamura 2012-02-16 23:21:42 PST
Comment on attachment 127526 [details]
Patch 1

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

> Source/WebCore/ChangeLog:9
> +        This patch simplifies event dispatch in HTML input element for ease of chaning
> +        event dispatching behavior.

chaning -> changing

I'm not sure why this change will help 'changing event dispatching behavior'.  Could you explain it please?

> Source/WebCore/html/HTMLInputElement.cpp:1049
> +        if (isTextField() && focused())

Using isTextField() in HTMLInputElement.cpp is not recommended.  Will you remove it?
Comment 3 yosin 2012-02-16 23:45:52 PST
Created attachment 127533 [details]
Patch 1
Comment 4 Kent Tamura 2012-02-16 23:51:10 PST
Comment on attachment 127533 [details]
Patch 1

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

Looks ok.

> Source/WebCore/html/TextFieldInputType.cpp:94
> +    if (valueChanged) {

nit: We prefer early exit:

if (!valueChanged)
    return;
Comment 5 Kent Tamura 2012-02-16 23:51:41 PST
Comment on attachment 127533 [details]
Patch 1

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

> Source/WebCore/ChangeLog:3
> +        [Forms] Merge InputType::dispatchChangeEventInResponseToSetValue into call site

Ah, the summary is incorrect.
Comment 6 yosin 2012-02-17 00:02:40 PST
Created attachment 127539 [details]
Patch 3
Comment 7 Kent Tamura 2012-02-17 00:05:00 PST
Comment on attachment 127539 [details]
Patch 3

Looks ok.
Comment 8 yosin 2012-02-17 00:27:11 PST
Created attachment 127543 [details]
Patch 4
Comment 9 Kent Tamura 2012-02-17 00:30:15 PST
Comment on attachment 127543 [details]
Patch 4

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

> Source/WebCore/ChangeLog:9
> +        and merge dispatch dispatchChangeEventInResponseToSetValue to setValue.

merge dispatch -> merge?

> Source/WebCore/html/TextFieldInputType.cpp:83
> +    // We don't ask InputType::setValue to dispatch events because of

because of -> because
Comment 10 yosin 2012-02-17 00:33:12 PST
Created attachment 127544 [details]
Patch 5
Comment 11 Kent Tamura 2012-02-17 00:35:11 PST
Comment on attachment 127544 [details]
Patch 5

ok
Comment 12 WebKit Review Bot 2012-02-17 01:22:57 PST
Comment on attachment 127544 [details]
Patch 5

Clearing flags on attachment: 127544

Committed r108051: <http://trac.webkit.org/changeset/108051>
Comment 13 WebKit Review Bot 2012-02-17 01:23:02 PST
All reviewed patches have been landed.  Closing bug.