[webkit-reviews] review denied: [Bug 63709] Disabled input type=search still allows clicking [x] to clear contents : [Attachment 99452] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Kentaro Hara <haraken at google.com>'s
request for review:
Bug 63709: Disabled input type=search still allows clicking [x] to clear
contents
https://bugs.webkit.org/show_bug.cgi?id=63709

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

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

> LayoutTests/fast/forms/disabled-search-input-expected.txt:11
> +PASS Value is "".
> +PASS Value is "b".
> +PASS Value is "foo".
> +PASS Value is "foo".
> +PASS Value is "foo".
> +PASS Value is "foo".
> +PASS Value is "foo".
> +PASS Value is "foo".

Test result readability is not good.
Please show each of test conditions by debug();

> LayoutTests/fast/forms/disabled-search-input.html:8
> +<input id="search_input" type="search" />

The purpose of the existing test is to check behavior in a case that a disabled
search field gets enabled.  You must add 'disabled' here.

> LayoutTests/fast/forms/disabled-search-input.html:15
> +function click(button) {

The argument name 'button' is confusing.  It's a position of something.

> LayoutTests/fast/forms/disabled-search-input.html:37
> +function checkResult(input, expected) {
> +    if (input.value == expected)
> +	   testPassed('Value is "' + input.value + '".');
> +    else
> +	   testFailed('Value is "' + input.value + '", but it should be "' +
expected + '".');
> +}

This function is not needed.
You can write it as:
    shouldBe('input.value', '"foo"');

> LayoutTests/fast/forms/disabled-search-input.html:39
> +function setInputAttribute(input, value, disabled, readonly) {

Attribute -> Attributes?

> LayoutTests/fast/forms/disabled-search-input.html:45
> +    if (readonly)
> +	   input.setAttribute("readonly", readonly);
> +    else if (input.hasAttribute("readonly"))
> +	   input.removeAttribute("readonly");

Why don't you use input.readOnly = readonly ?

> LayoutTests/fast/forms/disabled-search-input.html:50
> +    var cancelButton = searchCancelButtonPosition(input);

'cancelButton' is not a good name.  It's a position, not button.