Bug 93941 – [Forms] Set SpinButtonElement free from HTMLInputElement

• Component: Forms
• Status: RESOLVED FIXED

I would like to set SpinButtonElement free from HTMLInputELement, because:
  1. Concept and functionality of spin button is independent from the "input" element.
  2. ease of mountainous: HTMLInputElement.{cpp,h} changes don't affect to SpinButtonElement
  3. faster build time when HTMLInputElement.h changed. Frequency of HTMLInputElement.h changes is more often than SpinButtonElement.h, because HTMLInputElement has lot of methods for various input types.

I also move code of SpinButtonElement to separate file. Since multiple fields time input UI, SpinButtonElement is not only text field inner element but also inner element of multiple fields time input UI.

Created attachment 158749 [details]
Patch 1

[WebKit Review Bot]

Comment on attachment 158749 [details]
Patch 1

Attachment 158749 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13517286

New failing tests:
fast/forms/number/number-change-type-on-focus.html

[WebKit Review Bot]

Created attachment 158757 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 158768 [details]
Patch 2

[Kent Tamura]

Comment on attachment 158768 [details]
Patch 2

The patch doesn't have r? flag, but I have a comment.

I recommend to split the patch into three pieces.
1. Add SpinButtonElement.{cpp.h} to build files, and remove unnecessary code
2. Rename StepActionHandler to SpinButtonOwner
3. Remaining

Reviewers can set r+ for 1 and 2 easily because they are mechanical changes.
Reviewers need to review 3 and it should be smaller in order to concentrate on it easily. Actually, your first patch had use-after-free.

[Kent Tamura]

(In reply to comment #6)
> Reviewers need to review 3

need to review 3 carefully

Comment on attachment 158768 [details]
Patch 2

Could you review this patch?
Thanks in advance.

[Kent Tamura]

Comment on attachment 158768 [details]
Patch 2

r- because of comment #6

Created attachment 159064 [details]
Patch 3

Comment on attachment 159064 [details]
Patch 3

Could you review this patch?
Thanks in advance.

[Kent Tamura]

Comment on attachment 159064 [details]
Patch 3

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

> Source/WebCore/ChangeLog:40
> +        * html/TextFieldInputType.cpp:
> +        (WebCore::TextFieldInputType::focusAndSelectSpinButtonOwner):
> +        (WebCore::TextFieldInputType::shouldSpinButtonRespondToMouseEvents):
> +        (WebCore::TextFieldInputType::shouldSpinButtonRespondToWheelEvents):
> +        * html/TextFieldInputType.h:
> +        (TextFieldInputType):
> +        * html/TimeInputType.cpp:
> +        (WebCore::TimeInputType::DateTimeEditControlOwnerImpl::focusAndSelectEditControlOwner):
> +        (WebCore::TimeInputType::DateTimeEditControlOwnerImpl::isEditControlOwnerFocused):
> +        * html/TimeInputType.h:
> +        (DateTimeEditControlOwnerImpl):
> +        * html/shadow/DateTimeEditElement.cpp:
> +        (WebCore::DateTimeEditElement::focusAndSelectSpinButtonOwner):
> +        (WebCore::DateTimeEditElement::shouldSpinButtonRespondToMouseEvents):
> +        (WebCore::DateTimeEditElement::shouldSpinButtonRespondToWheelEvents):
> +        * html/shadow/DateTimeEditElement.h:
> +        (EditControlOwner):
> +        (DateTimeEditElement):
> +        * html/shadow/SpinButtonElement.cpp:
> +        (WebCore::SpinButtonElement::defaultEventHandler):
> +        (WebCore::SpinButtonElement::forwardEvent):
> +        (WebCore::SpinButtonElement::willRespondToMouseMoveEvents):
> +        (WebCore::SpinButtonElement::willRespondToMouseClickEvents):
> +        (WebCore::SpinButtonElement::step):
> +        (WebCore::SpinButtonElement::shouldRespondToMouseEvents):
> +        * html/shadow/SpinButtonElement.h:
> +        (SpinButtonOwner):
> +        (SpinButtonElement):

You had better add per-function comments, especially to existing functions.

Created attachment 159330 [details]
Patch 4

Comment on attachment 159330 [details]
Patch 4

Clearing flags on attachment: 159330

Committed r125997: <http://trac.webkit.org/changeset/125997>

All reviewed patches have been landed.  Closing bug.