[webkit-reviews] review denied: [Bug 98480] Add behavior tests for input[type=date] with multiple fields : [Attachment 167937] Patch 2

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Kent Tamura
<tkent at chromium.org>'s request for review:
Bug 98480: Add behavior tests for input[type=date] with multiple fields
https://bugs.webkit.org/show_bug.cgi?id=98480

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=167937&action=review


>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-aria-attrib
utes.html:11
> +function enableAccessibility()

Nit: This function just checks window.accessibilityController. You can remove
this and write the check directly in the global scope.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-aria-attrib
utes.html:22
> +    if (!window.accessibilityController)
> +	   return;

Nit: Remove this, as you've already checked window.accessibilityController.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-aria-attrib
utes.html:23
> +    var element = accessibilityController.focusedElement

Nit: ';' is missing.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-aria-attrib
utes.html:58
> +debug('');
> +testInput.parentNode.removeChild(testInput);

What is it for?

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-value-chang
ed-notification.html:11
> +function enableAccessibility()

Nit: initializeAccessibility() might be a better name.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-ax-value-chang
ed-notification.html:44
> +    debug('');
> +    testInput.parentNode.removeChild(testInput);

Do you need to remove it? If it is needed, debug('Remove the input element')
would be better so that we can observe the removal result in the expected file.


>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.h
tml:7
> +<p id="description"></p>

Nit: This is not needed. This will be added automatically.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.h
tml:33
> +    eventSender.mouseMoveTo(x + input.offsetLeft, y + input.offsetTop);

Nit: Maybe you can change this to eventSender.mouseMoveTo(x + input.offsetLeft,
input.offsetHeight / 2 + input.offsetTop); Then you can remove 'y' from the
arguments of this function.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.h
tml:39
> +input.blur();

Nit: Is this needed?

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-mouse-events.h
tml:91
> +
> +debug('');

Nit: Remove this.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-reset-value-af
ter-reloads.html:14
> +var testInput1;
> +var testInput2;

Nit: These can be local variables.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-reset-value-af
ter-reloads.html:19
> +    iframe.contentDocument.getElementById('test1').value = '2012-10-01';
> +    iframe.contentDocument.getElementById('test2').value = '2012-11-01';

Just in case, I want to check the values of these inputs not only in
runOnIFrameLoad() but also here.

>
LayoutTests/fast/forms/date-multiple-fields/date-multiple-fields-reset-value-af
ter-reloads.html:30
> +    iframe.parentNode.removeChild(iframe);

Nit: Is this needed?

> LayoutTests/fast/forms/date/date-reset-value.html:3
> +<!-- Confirm appearance after reset() is same as the initial appearance. -->


I would prefer writing this test using js-test-pre.js.

> LayoutTests/fast/forms/date/date-reset-value.html:11
> +if (testRunner)
> +    testRunner.waitUntilDone();
> +window.onload = function() {

Why does this test need to be async?

> LayoutTests/fast/forms/resources/multiple-fields-blur-and-focus-events.js:1
> +var testInput;

Nit: This can be a local variable.

> LayoutTests/fast/forms/resources/multiple-fields-blur-and-focus-events.js:39
> +    if (window.eventSender) {

Nit: This is not needed, as it is checked in keyDown().

> LayoutTests/fast/forms/resources/multiple-fields-blur-and-focus-events.js:50
> +    document.body.removeChild(document.getElementById("container"));

Nit: Is it needed?