Bug 97521 – [Forms] Move multiple fields related functions to BaseDateAndTimeInputType from TimeInputType

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

To share code among date time related input types, date, datetime, month, time and week, we should move multiple fields related functions to BaseDateAndTimeInputType class from TimeInputType class.

Created attachment 165736 [details]
Patch 1

Comment on attachment 165736 [details]
Patch 1

Could you review this patch?
Thanks in advance.

Bug 97299 has an implementation of multiple fields "month" input UI.

[Kent Tamura]

Comment on attachment 165736 [details]
Patch 1

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

The direction of the change looks good.

> Source/WebCore/html/BaseDateAndTimeInputType.h:74
> +class BaseMultipleFieldsDateAndTimeInputType : public BaseDateAndTimeInputType {

This class should have its own source files because it is large.

> Source/WebCore/html/BaseDateAndTimeInputType.h:82
> +    class DateTimeEditControlOwnerImpl : public DateTimeEditElement::EditControlOwner {

nit:
We may add DateTimeEditElement::EditControlOwner interface to BaseMultipleFieldsDateAndTimeInputType class directly.
You can change so in another patch.

> Source/WebCore/html/TimeInputType.h:43
> +typedef BaseMultipleFieldsDateAndTimeInputType BaseTimeInputType;
> +#else
> +typedef BaseDateAndTimeInputType BaseTimeInputType;

This should be in BaseMultipleFieldsDateAndTimeInputType.h because this pattern will be used in other date/time input types.
Of course we should rename BaseTimeInputType so that it doesn't have "Time".

Created attachment 165916 [details]
Patch 2

Comment on attachment 165916 [details]
Patch 2

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Move BaseMultipleFiledsDateAndTimeInputType class to own file.

[WebKit Review Bot]

Comment on attachment 165916 [details]
Patch 2

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

[Kent Tamura]

Comment on attachment 165916 [details]
Patch 2

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

> Source/WebCore/ChangeLog:9
> +        This patch introduces new class BaseMultipleFieldsDateAndTimeInputType
> +         for sharing code among multiple fields date/time input UI.

nit: extra space before 'for'

Created attachment 165922 [details]
Patch 3

Comment on attachment 165922 [details]
Patch 3

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Make BaseMultipleFieldsDateAndTimeInputType::formatDateTimeFieldsState() and setupLayoutParameters() pure virtual
** Fix compilation error and no need to have default functions
* Update ChangeLog

[Kent Tamura]

Comment on attachment 165922 [details]
Patch 3

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

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:234
> +        TextFieldInputType::updateInnerTextValue();

Why did you add the TextFieldInputType::updateInnerTextValue() call?  It looks unnecessary and confusing.

Created attachment 165931 [details]
Patch 4

Comment on attachment 165931 [details]
Patch 4

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Remove call of TextFieldInputType::updateInnerTextValue() in BaseMultipleFieldsDateAndTimeInputType::updateInnerTextValue()

[Kent Tamura]

Comment on attachment 165931 [details]
Patch 4

ok

Comment on attachment 165931 [details]
Patch 4

Clearing flags on attachment: 165931

Committed r129729: <http://trac.webkit.org/changeset/129729>

All reviewed patches have been landed.  Closing bug.