[webkit-reviews] review denied: [Bug 35786] Improve portability of fast/forms/listbox-selection.html : [Attachment 63693] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Kenichi Ishibashi
<bashi at google.com>'s request for review:
Bug 35786: Improve portability of fast/forms/listbox-selection.html
https://bugs.webkit.org/show_bug.cgi?id=35786

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

------- Additional Comments from Kent Tamura <tkent at chromium.org>
The change is basically good.  Thank you for taking this bug.

Did you confirm the change worked on Mac and Windows?

> +	   Improve portability of fast/forms/listbox-selection.html
> +	   https://bugs.webkit.org/show_bug.cgi?id=35786
> +
> +	   * fast/forms/listbox-selection-expected.txt: Updated to match with
new test.
> +	   * fast/forms/listbox-selection.html: Modified to use script-tests
style.
> +	   * fast/forms/script-tests/listbox-selection.js: Added.
> +	   (mouseDownOnSelect):
> +	   (keyDownOnSelect):
> +	   (createSelect):
> +	   (selectionPattern):

You had better add how to resolve the portability issue to ChangeLog.

> --- a/LayoutTests/fast/forms/listbox-selection-expected.txt
> +++ b/LayoutTests/fast/forms/listbox-selection-expected.txt
> @@ -1,29 +1,23 @@
> - 1) Select one item with mouse (no previous selection)
> - 2) Select one item with mouse (with previous selection)
> - 3) Select one item with the keyboard (no previous selection)
> - 4) Select one item with the keyboard (with previous selection)
> - 5) Attempt to select an item cmd-clicking
> - 6) Attempt to select a range shift-clicking
> - 7) Attempt to select a range with the keyboard
> - 8) Select one item with mouse (no previous selection)
> - 9) Select one item with mouse (with previous selection)
> - 10) Select one item with the keyboard (no previous selection)
> - 11) Select one item with the keyboard (with previous selection)
> - 12) Select an item cmd-clicking
> - 13) Select a range shift-clicking
> - 14) Select a range with the keyboard

The old expectation had information about what was tested, and new expectation
has no such information.
An expectation files should have such information in order to detect what is
wrong easily when a test fails. So, for example,

> +// 1) Select one item with mouse (no previous selection)
> +mouseDownOnSelect("sl1", 0);
> +shouldBe('selectionPattern("sl1")', '"10000"');

The first line should be:
  debug("1) Select one item with mouse (no previous selection)")

> +if (window.layoutTestController)
> +    layoutTestController.waitUntilDone();

> +if (window.layoutTestController)
> +    layoutTestController.notifyDone();

You don't need these blocks because js-test-pre.js and js-test-post.js are
loaded.