[webkit-reviews] review denied: [Bug 113005] INPUT_MULTIPLE_FIELDS_UI: Incomplete datetime format should fallback to default : [Attachment 194451] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Kunihiko Sakamoto
<ksakamoto at chromium.org>'s request for review:
Bug 113005: INPUT_MULTIPLE_FIELDS_UI: Incomplete datetime format should
fallback to default
https://bugs.webkit.org/show_bug.cgi?id=113005

Attachment 194451: Patch
https://bugs.webkit.org/attachment.cgi?id=194451&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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