[webkit-reviews] review denied: [Bug 80574] [Forms] Re-factor label.for tests for extending test coverage : [Attachment 131016] Patch 3

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied yosin at chromium.org's request for
review:
Bug 80574: [Forms] Re-factor label.for tests for extending test coverage
https://bugs.webkit.org/show_bug.cgi?id=80574

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

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


> LayoutTests/fast/forms/label/labels-add-htmlFor-label-expected.txt:22
> +Not labelable: label [object HTMLLabelElement]
> +FAIL element.labels should be undefined. Was [object NodeList]
> +Not labelable: legend [object HTMLLegendElement]
> +FAIL element.labels should be undefined. Was [object NodeList]

Some tests are failing.

> LayoutTests/fast/forms/label/labels-add-htmlFor-label.html:14
> +const tester = setupLabelsTest();

I don't recommend to use 'const'.
We sometimes run our tests on non-WebKit browsers to see compatibility, and IE
doesn't support const.

> LayoutTests/fast/forms/label/labels-add-parent-label.html:16
> +    var data = tester.checkLabels(name);

The tester function name is not descriptive.  It should be
getDescriptionDataIfLabelsIsSuported() or something.

> LayoutTests/fast/forms/label/labels-add-parent-label.html:17
> +    if (data == null)

if (!data)

if we follow C++ style rule.

> LayoutTests/fast/forms/label/labels-add-parent-label.html:24
> +    document.getElementById("div1").appendChild(label);

"div1" is implicit. It doesn't appear in this file.