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

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

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


The code change looks good.  Some nits.

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:221
> +    case DateTimeFormat::FieldTypeMonthStandAlone: {
> +	   int minMonth = 0, maxMonth = 11;
> +	   if (m_parameters.minimum.type() != DateComponents::Invalid
> +	       && m_parameters.maximum.type() != DateComponents::Invalid
> +	       && m_parameters.minimum.fullYear() ==
m_parameters.maximum.fullYear()
> +	       && m_parameters.minimum.month() <= m_parameters.maximum.month())
{
> +	       minMonth = m_parameters.minimum.month();
> +	       maxMonth = m_parameters.maximum.month();
> +	   }
> +	   RefPtr<DateTimeFieldElement> field;

There are a lot of duplicated code in the FieldTypeMonth case and the
FieldTypeMonthStandAlone case.	We had better merge them.
e.g.  ...::create(document, m_editElement, fieldType ==
DateTimeFormat::FieldTypeMonth ? m_parameters.locale.shortMonthLabels() :
m_parameters.locale.shortStandAloneMonthLabels(), minMonth, maxMonth);

> Source/WebCore/html/shadow/DateTimeFieldElements.h:64
> +    // DateTimeNumericFieldElement functions.

functions -> function

> Source/WebCore/html/shadow/DateTimeFieldElements.h:137
> +    // DateTimeNumericFieldElement functions.

Ditto.

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.h:77
> +    const int m_minIndex;
> +    const int m_maxIndex;

should not be abbreviations.
  m_minIndex -> m_minimumIndex
  m_maxIndex -> m_maximumIndex

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-readonly-subfi
eld.html:49
> +shouldBeTrue('isReadOnlyField(createDateInput("2012-12-01", "2012-12-31",
""), pseudoMonth)');
> +shouldBeFalse('isReadOnlyField(createDateInput("2012-11-01", "2013-12-31",
""), pseudoMonth)');
> +shouldBeFalse('isReadOnlyField(createDateInput("2012-12-01", "2013-12-31",
""), pseudoMonth)');

should add out-of-range value case such as createDateInput("2012-12-01",
"2012-12-31", "2012-11-30").

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-readonly-subfi
eld.html:59
> +shouldBeTrue('isReadOnlyField(createDateInput("2012-12-17", "2012-12-17",
""), pseudoDay)');
> +shouldBeFalse('isReadOnlyField(createDateInput("2012-12-17", "2013-12-18",
""), pseudoDay)');
> +shouldBeFalse('isReadOnlyField(createDateInput("2012-11-17", "2012-12-17",
""), pseudoDay)');
> +shouldBeFalse('isReadOnlyField(createDateInput("2012-12-17", "2013-12-17",
""), pseudoDay)');

Ditto.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdow
n-from-renderer.html:11
> +if (window.internals)
> +    internals.settings.setLangAttributeAwareFormControlUIEnabled(true);

Looks unnecessary.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdow
n-from-renderer.html:20
> +function keyDown(key, modifiers)
> +{

Style is inconsistent.	You put "{" on the same line as "function" in other
functions.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdow
n-from-renderer.html:22
> +    if (!window.eventSender)
> +	   return;

You had better warn the lack of eventSender at the beginning of the test rather
than ignoring eventSender.keyDown.
We sometimes load a layout test with a real browser, and if the test doesn't
work on the browser, we need to investigate the reason.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdow
n-from-renderer.html:27
> +    input.value = input.min = input.max = null;

Is this needed?

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdow
n-from-renderer.html:58
> +shouldBeEqualToString('stepUp("2000-05-01", null, null)', '2000-06-01');
> +shouldBeEqualToString('stepDown("2000-05-01", null, null)', '2000-04-01');
> +shouldBeEqualToString('stepUp("2000-12-01", null, null)', '2000-01-01');
> +shouldBeEqualToString('stepDown("2000-01-01", null, null)', '2000-12-01');
> +shouldBeEqualToString('test("2000-05-01", null, null, ["upArrow", "upArrow",
"upArrow"])', '2000-08-01');
> +shouldBeEqualToString('test("2000-05-01", null, null, ["downArrow",
"downArrow", "downArrow"])', '2000-02-01');
> +shouldBeEqualToString('test("2000-05-01", null, null, ["delete",
"upArrow"])', '2000-01-01');
> +shouldBeEqualToString('test("2000-05-01", null, null, ["delete",
"downArrow"])', '2000-12-01');

They look duplications of date-multiple-fields-keyboard-events.html.
We should remove them from this test, or merge everything to
date-multiple-fields-keyboard-events.html.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-stepup-stepdow
n-from-renderer.html:116
> +document.body.removeChild(input);

input.remove() is enough.

>
LayoutTests/fast/forms/month-multiple-fields/month-multiple-fields-readonly-sub
field-expected.txt:24
> +PASS isReadOnlyField(createMonthInput("2012-12", "2012-12", ""),
pseudoMonth) is true
> +PASS isReadOnlyField(createMonthInput("2012-11", "2013-12", ""),
pseudoMonth) is false
> +PASS isReadOnlyField(createMonthInput("2012-12", "2013-12", ""),
pseudoMonth) is false

should add out-of-range month value cases.