Bug 113005 – INPUT_MULTIPLE_FIELDS_UI: Incomplete datetime format should fallback to default

• Component: Forms
• Status: RESOLVED FIXED

[Kunihiko Sakamoto]

Imported from https://code.google.com/p/chromium/issues/detail?id=182123

When system locale's datetime format is not complete (e.g. 12-hour without AM/PM field), user cannot enter valid value to datetime inputs.
In such cases, fallback format should be used.

[Kunihiko Sakamoto]

Created attachment 194451 [details]
Patch

[Kent Tamura]

Comment on attachment 194451 [details]
Patch

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

r- because of some style errors.

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:57
> +class FormatValidator : public DateTimeFormat::TokenHandler {

This class belongs to the large WebCore namespace. We had better name more specific class name; e.g. WebCore::DateTimeFormatValidator.

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:73
> +    bool validateFormat(const String& format, const BaseMultipleFieldsDateAndTimeInputType*);
> +private:

Need a blank line before a scope label.

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:123
> +bool FormatValidator::validateFormat(const String& format, const BaseMultipleFieldsDateAndTimeInputType* inputType)

The second argument type should be "const BaseMultipleFieldDateAndTimeInputType&".

> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.h:54
>      , protected ClearButtonElement::ClearButtonOwner {
> +public:
> +    virtual bool isValidFormat(bool hasYear, bool hasMonth, bool hasWeek, bool hasDay, bool hasAMPM, bool hasHour, bool hasMinute, bool hasSecond) const = 0;
>  protected:

Need a blank line before each of scope labels

[Kunihiko Sakamoto]

Created attachment 194459 [details]
Patch 2

[Kunihiko Sakamoto]

Created attachment 194460 [details]
Patch 3, EWS build fix

[Kunihiko Sakamoto]

Comment on attachment 194451 [details]
Patch

Please take another look.


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

>> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:57
>> +class FormatValidator : public DateTimeFormat::TokenHandler {
> 
> This class belongs to the large WebCore namespace. We had better name more specific class name; e.g. WebCore::DateTimeFormatValidator.

Done.

>> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:73
>> +private:
> 
> Need a blank line before a scope label.

Done.

>> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.cpp:123
>> +bool FormatValidator::validateFormat(const String& format, const BaseMultipleFieldsDateAndTimeInputType* inputType)
> 
> The second argument type should be "const BaseMultipleFieldDateAndTimeInputType&".

Done.

>> Source/WebCore/html/BaseMultipleFieldsDateAndTimeInputType.h:54
>>  protected:
> 
> Need a blank line before each of scope labels

Done.

[Kent Tamura]

Comment on attachment 194460 [details]
Patch 3, EWS build fix

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

FYI. I thought additional format parsing was not needed if we implement the check in DateTimeEditBuilder. However your design is simpler and we can avoid additional complexity of DateTimeEditBuilder.

> Source/WebCore/ChangeLog:21
> +        (FormatValidator): A helper class that parses datetime format and tests existence of fields.
> +        (WebCore::FormatValidator::FormatValidator):
> +        (WebCore::FormatValidator::visitField):
> +        (WebCore::FormatValidator::validateFormat): Parses datetime format and validates by calling BaseMultipleFieldsDateAndTimeInputType::isValidFormat.

Need to update these lines

[Kunihiko Sakamoto]

Created attachment 194480 [details]
Patch 4

[Kunihiko Sakamoto]

(In reply to comment #6)
> (From update of attachment 194460 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194460&action=review
> 
> FYI. I thought additional format parsing was not needed if we implement the check in DateTimeEditBuilder. However your design is simpler and we can avoid additional complexity of DateTimeEditBuilder.

Yeah first I attempted to check it in DateTimeEditBuilder, but then realized that InputType knows best whether format is valid or not.

> 
> > Source/WebCore/ChangeLog:21
> > +        (FormatValidator): A helper class that parses datetime format and tests existence of fields.
> > +        (WebCore::FormatValidator::FormatValidator):
> > +        (WebCore::FormatValidator::visitField):
> > +        (WebCore::FormatValidator::validateFormat): Parses datetime format and validates by calling BaseMultipleFieldsDateAndTimeInputType::isValidFormat.
> 
> Need to update these lines

Oops, done.

[Kent Tamura]

Comment on attachment 194480 [details]
Patch 4

ok

[WebKit Review Bot]

Comment on attachment 194480 [details]
Patch 4

Clearing flags on attachment: 194480

Committed r146584: <http://trac.webkit.org/changeset/146584>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.