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

[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 165963: Patch 2
https://bugs.webkit.org/attachment.cgi?id=165963&action=review

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


Let's split.
1. Adding placeholder feature to DateTimeNumericElement, and update its
existing subclasses.
2. Adding DateTimeMonthFieldElement and DateTimeYearFieldElement
3. Remaining C++/CSS changes
4. Tests

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

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));

This code seems to support only M and MM.  Don't you support MMM, MMMM, MMMMM ?


> Source/WebCore/html/shadow/DateTimeFieldElements.cpp:351
> +static const int minimumYear = 1;
> +// Date in ECMAScript can't represent dates later than 275760-09-13T00:00Z.
> +// So, we have the same upper limit in HTML5 dates.
> +static const int maximumYear = 275760;
> +
> +DateTimeYearFieldElement::DateTimeYearFieldElement(Document* document,
FieldOwner& fieldOwner, const String& placeholder)
> +    : DateTimeNumericFieldElement(document, fieldOwner, minimumYear,
maximumYear, placeholder)

We had better apply actual min/max years of the host <input>.

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:194
>  String DateTimeNumericFieldElement::visibleValue() const
>  {
> -    if (m_hasValue)
> -	   return value();
> -
> -    StringBuilder builder;
> -    for (int numberOfDashs = std::max(displaySizeOfNumber(m_range.maximum),
displaySizeOfNumber(m_range.minimum)); numberOfDashs; --numberOfDashs)
> -	   builder.append('-');
> -    return builder.toString();
> +    return m_hasValue ? value() : m_placeholder;

You assume the length of m_placeholder is longer than or equal to the length of
the maximum value?