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

Description Kent Tamura 2012-10-19 00:07:45 PDT
Use Localizer::monthFormat to construct input[type=month] UI
Comment 1 Kent Tamura 2012-10-19 00:31:13 PDT
Created attachment 169568 [details]
Patch
Comment 2 Kent Tamura 2012-10-19 01:30:58 PDT
Created attachment 169574 [details]
Patch 2

more rebaseline
Comment 3 Kentaro Hara 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.
Comment 4 Kent Tamura 2012-10-19 02:41:40 PDT
Created attachment 169583 [details]
Patch 3

follow reviewer's comments
Comment 5 Kent Tamura 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.
Comment 6 Kentaro Hara 2012-10-19 06:38:09 PDT
Comment on attachment 169583 [details]
Patch 3

Looks OK
Comment 7 Kent Tamura 2012-10-19 06:51:31 PDT
Comment on attachment 169583 [details]
Patch 3

Thanks!
Comment 8 WebKit Review Bot 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>
Comment 9 WebKit Review Bot 2012-10-19 07:20:56 PDT
All reviewed patches have been landed.  Closing bug.