[webkit-reviews] review denied: [Bug 99168] Add tests for suggestion picker that test step/min/max attributes : [Attachment 168406] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 99168: Add tests for suggestion picker that test step/min/max attributes
https://bugs.webkit.org/show_bug.cgi?id=99168

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

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


> LayoutTests/fast/forms/resources/suggestion-picker-common.js:28
> +    for (var i = 0; i < elements.length; ++i) {
> +	   values.push(valueForEntry(elements[i]));
> +    }

nit: { and } are not needed

> LayoutTests/platform/chromium-android/TestExpectations:92
>
+platform/chromium/fast/forms/date/date-suggestion-picker-min-max-attribute.htm
l [ WontFix ]
> +platform/chromium/fast/forms/date/date-suggestion-picker-step-attribute.html
[ WontFix ]
>
+platform/chromium/fast/forms/time/time-suggestion-picker-min-max-attribute.htm
l [ WontFix ]
> +platform/chromium/fast/forms/time/time-suggestion-picker-step-attribute.html
[ WontFix ]

We had better put *-suggestion-picker-* tests into their own directory. 
platform/chromium/fast/forms/suggestion-picker/ ?

>
LayoutTests/platform/chromium/fast/forms/date/date-suggestion-picker-min-max-at
tribute.html:26
> +description("Tests that key bindings work as expected.");
> +
> +debug('Check that page popup doesn\'t exist at first.');

Usage of ' and " is inconsistent.

>
LayoutTests/platform/chromium/fast/forms/date/date-suggestion-picker-min-max-at
tribute.html:31
> +    openPicker(document.getElementById('date'));
> +    popupWindow.addEventListener("resize", test, false);

nit:
We had better introduce openPicker() with a callback argument. like:

function openPicker(element, callback) {
 ....
  popupOpwnCallback = callback;
  popupWindow.addEventListener("resize", pickerCallbackWrapper, false);
}

function pickerCallbackWrapper() {
    popupWindow.removeEventListener("resize", pickerCallbackWrapper);
    popupOpenCallback();
}

>
LayoutTests/platform/chromium/fast/forms/date/date-suggestion-picker-min-max-at
tribute.html:37
> +var timeoutTimer = setTimeout(function() {
> +    testFailed("Test timed out.");
> +    finishJSTest();
> +}, 10000);

Do not add time out detection in tests.  It will be a source of flakiness.

>
LayoutTests/platform/chromium/fast/forms/date/date-suggestion-picker-step-attri
bute.html:39
> +var timeoutTimer = setTimeout(function() {
> +    testFailed("Test timed out.");
> +    finishJSTest();
> +}, 10000);

ditto.

>
LayoutTests/platform/chromium/fast/forms/time/time-suggestion-picker-min-max-at
tribute.html:34
> +var timeoutTimer = setTimeout(function() {
> +    testFailed("Test timed out.");
> +    finishJSTest();
> +}, 10000);

ditto.

>
LayoutTests/platform/chromium/fast/forms/time/time-suggestion-picker-step-attri
bute.html:36
> +var timeoutTimer = setTimeout(function() {
> +    testFailed("Test timed out.");
> +    finishJSTest();
> +}, 10000);

ditto.