[webkit-reviews] review denied: [Bug 97299] [Forms] Multiple fields month input UI : [Attachment 166167] Patch 3

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 97299: [Forms] Multiple fields month input UI
https://bugs.webkit.org/show_bug.cgi?id=97299

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166167&action=review


> Source/WebCore/ChangeLog:11
> +	   Note: This patch affects ports which enable both
ENABLE_INPUT_TYPE_TIME

Looks wrong.  ENABLE_INPUT_TYPE_TIME is unrelated.

> Source/WebCore/html/MonthInputType.cpp:152
> +    return String::format("%u-%02u", dateTimeFieldsState.year(),
dateTimeFieldsState.month());

Don't you need %04u-%02u?

> Source/WebCore/html/MonthInputType.h:39
> +#if ENABLE(INPUT_MULTIPLE_FIELDS_UI)
> +#include "BaseMultipleFieldsDateAndTimeInputType.h"
> +#else
> +#include "BaseDateAndTimeInputType.h"
> +#endif

You may just include BaseMultipleFieldsDateAndTimeInputType.h.

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:71
> +    const String& m_placeholderForMonth;
> +    const String& m_placeholderForYear;

See Comment #7:
> Let's use "const String" instead of "const String&".	I understand "const
String&" works in this case, but it is not robust for the future changes.
Copying String objects is not expensive.

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:127
> +    case DateTimeFormat::FieldTypeMonth:
> +	   m_editElement.addField(DateTimeMonthFieldElement::create(document,
m_editElement, m_placeholderForMonth));

Need a comment about MMM, MMMM, MMMMM.