[webkit-reviews] review denied: [Bug 74590] [Forms] Selection change by type-ahead doesn't fire 'change' event : [Attachment 119551] Patch 1

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 74590: [Forms] Selection change by type-ahead doesn't fire 'change' event
https://bugs.webkit.org/show_bug.cgi?id=74590

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

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


The code change looks ok.

> LayoutTests/fast/forms/select/menulist-type-ahead-find-expected.txt:1
> +WebKit Bug 74590

We should show what is tested.	The bug number is not useful.

> LayoutTests/fast/forms/select/menulist-type-ahead-find.html:16
> +function log(msg)
> +{
> +    var res = document.getElementById("log");
> +    res.innerHTML = res.innerHTML + msg + "<br>";
> +}
> +
> +function expectEq(expected, got)
> +{
> +    if (got == expected)
> +	   log("PASSED got=[" + got + "]");
> +    else
> +	   log("FAILED: expected[" + expected + "] got=[" + got + "]");
> +}

You don't need to make these function if you use
../../js/resources/js-test-pre.js.
or, they can be removed if we have <div id="res">FAIL</div> initially and
recordIt() changes the content to 'PASS'.

> LayoutTests/fast/forms/select/menulist-type-ahead-find.html:21
> +function recordIt(ctrl)
> +{
> +   document.getElementById("res").innerText = ctrl.value;
> +}

I think recording the control value is not necessary.  .innerText = 'PASS' is
enough.

> LayoutTests/fast/forms/select/menulist-type-ahead-find.html:37
> +    if (!window.layoutTestController)
> +	   return;

It would be nice if we show a manual test instruction.
This test shows 'Type "c" To see "cherry"' for now. We had better explain the
expected result too.