Bug 97521

Summary:

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

Product:

WebKit

Reporter:

yosin

Component:

Forms

Assignee:

yosin

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, mifenton, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

97649

Bug Blocks:

97299, 97756

Attachments:

Description	Flags
Patch 1	none
Patch 2	none
Patch 3	none
Patch 4	none

yosin

Reported 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.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

yosin

Comment 1 2012-09-25 23:11:27 PDT

Created attachment 165736 [details] Patch 1

yosin

Comment 2 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.

Kent Tamura

Comment 3 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".

yosin

Comment 4 2012-09-26 19:21:05 PDT

Created attachment 165916 [details] Patch 2

yosin

Comment 5 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.

WebKit Review Bot

Comment 6 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

Kent Tamura

Comment 7 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'

yosin

Comment 8 2012-09-26 21:06:42 PDT

Created attachment 165922 [details] Patch 3

yosin

Comment 9 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

Kent Tamura

Comment 10 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.

yosin

Comment 11 2012-09-26 22:48:52 PDT

Created attachment 165931 [details] Patch 4

yosin

Comment 12 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()

Kent Tamura

Comment 13 2012-09-26 23:41:19 PDT

Comment on attachment 165931 [details] Patch 4 ok

yosin

Comment 14 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>

yosin

Comment 15 2012-09-26 23:43:02 PDT

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.