Bug 99818 – Use Localizer::monthFormat to construct input[type=month] UI

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Use Localizer::monthFormat to construct input[type=month] UI

[Kent Tamura]

Created attachment 169568 [details]
Patch

[Kent Tamura]

Created attachment 169574 [details]
Patch 2

more rebaseline

[Kentaro Hara]

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]

Created attachment 169583 [details]
Patch 3

follow reviewer's comments

[Kent Tamura]

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 on attachment 169583 [details]
Patch 3

Looks OK

[Kent Tamura]

Comment on attachment 169583 [details]
Patch 3

Thanks!

[WebKit Review Bot]

Comment on attachment 169583 [details]
Patch 3

Clearing flags on attachment: 169583

Committed r131898: <http://trac.webkit.org/changeset/131898>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.