[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 bugzilla-daemon at webkit.org
Tue Feb 12 18:32:41 PST 2013

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

Attachment 187813: Patch

------- 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 =
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 ==
It's ok if you think it's out of scope.

> +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"

>  debug('Hours, 0-23');

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

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").

More information about the webkit-reviews mailing list