Bug 97992 - [Forms] Adding DateTimeWeekFieldElement for multiple fields "week" input UI
Summary: [Forms] Adding DateTimeWeekFieldElement for multiple fields "week" input UI
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: 97877
  Show dependency tree / graph
 
Reported: 2012-09-30 22:21 PDT by yosin
Modified: 2012-10-01 02:18 PDT (History)
1 user (show)

See Also:

Attachments
Patch 1 (4.69 KB, patch)
2012-10-01 00:02 PDT, yosin 		no flags Details | Formatted Diff | Diff
Patch 2 (6.46 KB, patch)
2012-10-01 01:50 PDT, yosin 		no flags Details | Formatted Diff | Diff
Patch 3 (7.47 KB, patch)
2012-10-01 02:12 PDT, yosin 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-09-30 22:21:30 PDT 
To implement multiple fields "week" input UI, we would like to have DateTimeWeekFieldElement.
Comment 1 yosin 2012-10-01 00:02:58 PDT 
Created attachment 166414 [details]
Patch 1
Comment 2 yosin 2012-10-01 00:03:31 PDT 
Comment on attachment 166414 [details]
Patch 1

Could you review this patch?
Thanks in advance.
Comment 3 Kent Tamura 2012-10-01 00:38:56 PDT 
Comment on attachment 166414 [details]
Patch 1

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

> Source/WebCore/ChangeLog:8
> +        THis patch is a preparation of implementing multiple fields date/time

THis -> This
BTW, this paragraph looks unnecessary.  The next paragraph has enough information.

> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:344
> +    : DateTimeNumericFieldElement(document, fieldOwner, 1, 53, "--")

We had better define a static const variable for "53", or add a static const member to DateComponents.
Comment 4 yosin 2012-10-01 01:50:32 PDT 
Created attachment 166427 [details]
Patch 2
Comment 5 yosin 2012-10-01 01:51:34 PDT 
Comment on attachment 166427 [details]
Patch 2

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Add constant DateComponents::maximumWeekOfYearNumber and miniumWeekOfYear.
Comment 6 Kent Tamura 2012-10-01 02:01:32 PDT 
Comment on attachment 166427 [details]
Patch 2

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

> Source/WebCore/platform/DateComponents.cpp:48
> +const int DateComponents::minimumWeekOfYearNumber = 1;
> +
> +// HTML5 specification defines maximum week of year is 53.
> +const int DateComponents::maximumWeekOfYearNumber = 53;

IMO, minimumWeekNumber and maximumWeekNumber are better.
DateComponents::maxWeekNumberInYear() and DateComponents::parseWeek() should use them.
Comment 7 yosin 2012-10-01 02:12:37 PDT 
Created attachment 166430 [details]
Patch 3
Comment 8 yosin 2012-10-01 02:14:16 PDT 
Comment on attachment 166430 [details]
Patch 3

Could you review this patch?
Thanks in advance.

= Changes since the last review =
* Rename maximumWeekOfYearNumber to maximumWeekNumber.
* Rename minimumWeekOfYearNumber to minimumWeekNumber.
* Use maximumWeekNumber in DateComponents::maxWeekNumberInYear()
* Use minimumWeekNumber in parseWeek()
Comment 9 Kent Tamura 2012-10-01 02:16:18 PDT 
Comment on attachment 166430 [details]
Patch 3

ok
Comment 10 yosin 2012-10-01 02:18:25 PDT 
Comment on attachment 166430 [details]
Patch 3

Clearing flags on attachment: 166430

Committed r130018: <http://trac.webkit.org/changeset/130018>
Comment 11 yosin 2012-10-01 02:18:29 PDT 
All reviewed patches have been landed.  Closing bug.