Bug 99818

Summary: Use Localizer::monthFormat to construct input[type=month] UI
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: haraken, mifenton, morrita, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 99787    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch 2
none
Patch 3 none

Kent Tamura
Reported 2012-10-19 00:07:45 PDT
Use Localizer::monthFormat to construct input[type=month] UI
Attachments
Patch (50.30 KB, patch)
2012-10-19 00:31 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Patch 2 (216.78 KB, patch)
2012-10-19 01:30 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Patch 3 (216.89 KB, patch)
2012-10-19 02:41 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kent Tamura
Comment 1 2012-10-19 00:31:13 PDT
Created attachment 169568 [details] Patch
Kent Tamura
Comment 2 2012-10-19 01:30:58 PDT
Created attachment 169574 [details] Patch 2 more rebaseline
Kentaro Hara
Comment 3 2012-10-19 02:29:07 PDT
Comment on attachment 169568 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169568&action=review > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:399 > + dateTimeFieldsState.setMonth(hasValue() ? valueAsInteger() + 1 : DateTimeFieldsState::emptyValue); We might want an ASSERT that checks that the value is less than symbolsSize(). > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:420 > + const unsigned value = dateTimeFieldsState.month() - 1; > + if (value < symbolsSize()) { > + setValueAsInteger(value); > + return; > + } > + > + setEmptyValue(dateForReadOnlyField); Nit: I would slightly prefer: if (value >= symbolsSize()) { setEmptyValue(dateForReadOnlyField); return; } setValueAsInteger(value); Just like the above 'if (!dateTimeFieldsState.hasMonth())' check, we can put an error case first.
Kent Tamura
Comment 4 2012-10-19 02:41:40 PDT
Created attachment 169583 [details] Patch 3 follow reviewer's comments
Kent Tamura
Comment 5 2012-10-19 02:42:13 PDT
Comment on attachment 169568 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169568&action=review >> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:399 >> + dateTimeFieldsState.setMonth(hasValue() ? valueAsInteger() + 1 : DateTimeFieldsState::emptyValue); > > We might want an ASSERT that checks that the value is less than symbolsSize(). Done. >> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:420 >> + setEmptyValue(dateForReadOnlyField); > > Nit: I would slightly prefer: > > if (value >= symbolsSize()) { > setEmptyValue(dateForReadOnlyField); > return; > } > setValueAsInteger(value); > > Just like the above 'if (!dateTimeFieldsState.hasMonth())' check, we can put an error case first. Done.
Kentaro Hara
Comment 6 2012-10-19 06:38:09 PDT
Comment on attachment 169583 [details] Patch 3 Looks OK
Kent Tamura
Comment 7 2012-10-19 06:51:31 PDT
Comment on attachment 169583 [details] Patch 3 Thanks!
WebKit Review Bot
Comment 8 2012-10-19 07:20:53 PDT
Comment on attachment 169583 [details] Patch 3 Clearing flags on attachment: 169583 Committed r131898: <http://trac.webkit.org/changeset/131898>
WebKit Review Bot
Comment 9 2012-10-19 07:20:56 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.