[webkit-reviews] review denied: [Bug 50380] Implement "required" attribute for select tags : [Attachment 75494] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Dai Mikurube
<dmikurube at google.com>'s request for review:
Bug 50380: Implement "required" attribute for select tags
https://bugs.webkit.org/show_bug.cgi?id=50380

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

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

> LayoutTests/fast/forms/ValidityState-valueMissing-001-expected.txt:2
> -There are two form control elements below, both required and blank:
validity.valueMissing should be true in both cases.
> +This test checks validity.valueMissing with blank values, blank options
selected, or nothing selected.
>  

The purpose of ValidityState-valueMissing-001.html was test cases for
validity.valueMissing==true.
If you follow it, you should remove cases with valueMissing==false.

> LayoutTests/fast/forms/ValidityState-valueMissing-001.html:71
> +    if (window.layoutTestController)
> +	   layoutTestController.waitUntilDone();

We don't need to call waitUntilDone() and notifyDone().  The document text is
dumped automatically when the document loading is completed.

> LayoutTests/fast/forms/ValidityState-valueMissing-001.html:91
> +    if (window.layoutTestController)
> +	   layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/ValidityState-valueMissing-002.html:53
> +    if (window.layoutTestController)
> +	   layoutTestController.waitUntilDone();

ditto

> LayoutTests/fast/forms/ValidityState-valueMissing-002.html:69
> +    if (window.layoutTestController)
> +	   layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/ValidityState-valueMissing-003-expected.txt:15
> +FAIL valueMissingFor("select-none-selected-multiple") should be false. Was
true.
> +PASS valueMissingFor("select-fake-placeholder-selected-multiple") is false
> +PASS valueMissingFor("select-without-fake-placeholder-multiple") is false
> +FAIL valueMissingFor("select-none-selected-size2-multiple") should be false.
Was true.

The result contains FAIL lines.

> LayoutTests/fast/forms/ValidityState-valueMissing-003.html:61
> +    if (window.layoutTestController)
> +	   layoutTestController.waitUntilDone();

waitUntilDone() and notifyDone() are not needed.

> LayoutTests/fast/forms/ValidityState-valueMissing-003.html:79
> +    if (window.layoutTestController)
> +	   layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/checkValidity-002-expected.txt:11
> +PASS checkValidityFor("select-non-placeholder") is true

checkValidity-002.html was for cases with checkValidity()==false.  You
shouldn't include checkValidity()==true case if you follow it.

> LayoutTests/fast/forms/required-attribute-001.html:25
> +    if (window.layoutTestController)
> +	   layoutTestController.waitUntilDone();

waitUntilDone() and notifyDone() are not needed.

> LayoutTests/fast/forms/required-attribute-001.html:34
> +    if (window.layoutTestController)
> +	   layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/required-attribute-002.html:25
> +    if (window.layoutTestController)
> +	   layoutTestController.waitUntilDone();

ditto.

> LayoutTests/fast/forms/required-attribute-002.html:46
> +    if (window.layoutTestController)
> +	   layoutTestController.notifyDone();

ditto.

> LayoutTests/fast/forms/resources/select-live-pseudo-selectors.js:76
> +// --------------------------------
> +//	  value change
> +// --------------------------------

We need more test cases for this.
You added some setNeedsValidityCheck() calls.  The test should cover all of
code paths with them.

> WebCore/dom/SelectElement.cpp:217
> -void SelectElement::scrollToSelection(SelectElementData& data, Element*
element)
> +void SelectElement::scrollToSelection(const SelectElementData& data,
Element* element)

Adding "const" to existing code is not related to the bug.  Please make a
separated bug and patch.

> WebCore/html/HTMLSelectElement.cpp:119
> +    // The number of the first option selected.
> +    int index = selectedIndex();

The variable name should be "firstSelectionIndex" and the comment should be
removed.