[webkit-reviews] review denied: [Bug 109555] INPUT_MULTIPLE_FIELDS_UI: Step-up/-down of hour field should respect min/max attributes : [Attachment 187813] 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 109555: INPUT_MULTIPLE_FIELDS_UI: Step-up/-down of hour field should
respect min/max attributes
https://bugs.webkit.org/show_bug.cgi?id=109555

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

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


> Source/WebCore/html/shadow/DateTimeEditElement.cpp:161
>	   DateTimeNumericFieldElement::Parameters parameters =
createNumericFieldParameters(static_cast<int>(msPerHour),
static_cast<int>(msPerHour * 12));
> -	   RefPtr<DateTimeFieldElement> field =
DateTimeHourFieldElement::create(document, m_editElement, 0, 11, parameters);
> +	   int minimumHour23, maximumHour23;
> +	   getRangeOfHourField(minimumHour23, maximumHour23);
> +	   RefPtr<DateTimeFieldElement> field =
DateTimeHour11FieldElement::create(document, m_editElement, minimumHour23,
maximumHour23, parameters);

How about introducing a subclass of DateTimeNumericFieldElement::Parameters to
add minimum/maximum members?
Repeating the same code four times looks not good.

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:163
>	   if (shouldHourFieldDisabled()) {

This patch doesn't disable the field in a case of minimumHour23 ==
maximumHour23.
It's ok if you think it's out of scope.

>
LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdow
n-from-renderer.html:175
> +shouldBeEqualToString('stepDown("15:00", 1, "13:00", "13:00")', '13:00');
> +shouldBeEqualToString('stepUp("12:00", 1, "12:00", "15:00")', '13:00');
> +shouldBeEqualToString('stepDown("12:00", 1, "12:00", "15:00")', '23:00');
> +shouldBeEqualToString('stepUp("15:00", 1, "12:00", "15:00")', '16:00');

We should have tests with
 - max="12:00" 
 - min="00:00"
 - min is in the morning, and max is in the afternoon.	e.g. min="10:00"
max="15:00"

>
LayoutTests/fast/forms/time-multiple-fields/time-multiple-fields-stepup-stepdow
n-from-renderer.html:182
>  debug('Hours, 0-23');

I have a concern about test coverage. The test doesn't cover hour11 and hour24
cases.

It seems there are no locales with these hour formats in ICU and Windows.
However we need support them because users can set these formats in OSX.
Ideally, we had better introduce test-only function to set date/time format. 
It would be something like window.internals.setDateTimeFormat(input, "kk:mm").