Summary: | Use Localizer::monthFormat to construct input[type=month] UI | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||
Component: | Forms | Assignee: | 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
Kent Tamura
2012-10-19 00:07:45 PDT
Created attachment 169568 [details]
Patch
Created attachment 169574 [details]
Patch 2
more rebaseline
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. Created attachment 169583 [details]
Patch 3
follow reviewer's comments
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. Comment on attachment 169583 [details]
Patch 3
Looks OK
Comment on attachment 169583 [details]
Patch 3
Thanks!
Comment on attachment 169583 [details] Patch 3 Clearing flags on attachment: 169583 Committed r131898: <http://trac.webkit.org/changeset/131898> All reviewed patches have been landed. Closing bug. |