Bug 97521 - [Forms] Move multiple fields related functions to BaseDateAndTimeInputType from TimeInputType
Summary: [Forms] Move multiple fields related functions to BaseDateAndTimeInputType fr...
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: 97649
Blocks: 97299 97756
  Show dependency treegraph
 
Reported: 2012-09-24 21:43 PDT by yosin
Modified: 2012-09-26 23:43 PDT (History)
4 users (show)

See Also:


Attachments
Patch 1 (28.23 KB, patch)
2012-09-25 23:11 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 2 (35.64 KB, patch)
2012-09-26 19:21 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 3 (35.22 KB, patch)
2012-09-26 21:06 PDT, yosin
no flags Details | Formatted Diff | Diff
Patch 4 (34.84 KB, patch)
2012-09-26 22:48 PDT, 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-09-24 21:43:20 PDT
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.
Comment 1 yosin 2012-09-25 23:11:27 PDT
Created attachment 165736 [details]
Patch 1
Comment 2 yosin 2012-09-25 23:12:55 PDT
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.
Comment 3 Kent Tamura 2012-09-26 02:17:21 PDT
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".
Comment 4 yosin 2012-09-26 19:21:05 PDT
Created attachment 165916 [details]
Patch 2
Comment 5 yosin 2012-09-26 19:22:02 PDT
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.
Comment 6 WebKit Review Bot 2012-09-26 19:44:38 PDT
Comment on attachment 165916 [details]
Patch 2

Attachment 165916 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14036636
Comment 7 Kent Tamura 2012-09-26 20:45:09 PDT
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'
Comment 8 yosin 2012-09-26 21:06:42 PDT
Created attachment 165922 [details]
Patch 3
Comment 9 yosin 2012-09-26 21:08:16 PDT
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
Comment 10 Kent Tamura 2012-09-26 22:15:24 PDT
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.
Comment 11 yosin 2012-09-26 22:48:52 PDT
Created attachment 165931 [details]
Patch 4
Comment 12 yosin 2012-09-26 22:50:25 PDT
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()
Comment 13 Kent Tamura 2012-09-26 23:41:19 PDT
Comment on attachment 165931 [details]
Patch 4

ok
Comment 14 yosin 2012-09-26 23:42:57 PDT
Comment on attachment 165931 [details]
Patch 4

Clearing flags on attachment: 165931

Committed r129729: <http://trac.webkit.org/changeset/129729>
Comment 15 yosin 2012-09-26 23:43:02 PDT
All reviewed patches have been landed.  Closing bug.