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

[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 130983: Patch 2
https://bugs.webkit.org/attachment.cgi?id=130983&action=review

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


> LayoutTests/fast/forms/label/labels-add-htmlFor-label.html:18
> +    var id = name + '1';

This rule is very implicit.
I recommend passing formControlDesc to createAllFromControls(), and it fills id
properties of formControlDesc.

> LayoutTests/fast/forms/resources/forms-test-util.js:1
> +// A list of labelable elements resides in
http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#category
-label

We already have forms/resources/common.js.  The content of this file should be
moved to common.js.

> LayoutTests/fast/forms/resources/forms-test-util.js:3
> +const formControlClassNames = [

Polluting the global namespace in a utility file is not good. Please make a
function like createFormControlDescription(), and avoid to make global
variables.

> LayoutTests/fast/forms/resources/forms-test-util.js:20
> +var formControlDesc = {};

'Desc' is not good for variable names.	Use a full word, 'Description'

> LayoutTests/fast/forms/resources/forms-test-util.js:27
> +	   labelable: true,

Let's follow C++ style rule.
lebelable -> isLabelable

> LayoutTests/fast/forms/resources/forms-test-util.js:28
> +	   listable: true,

listable is not used in this patch.  Please do not add in this patch.

> LayoutTests/fast/forms/resources/forms-test-util.js:30
> +	   supported: element + '' == '[object ' + className + ']',

+ '' looks ugly.  Please use .toString().

> LayoutTests/fast/forms/resources/forms-test-util.js:92
> +	   // FIXME: How do we know supported input types?
> +	   supported: true,

You can detect by
  inputElement.type = typeName;  if (inputElement.type == typeName) ...

> LayoutTests/fast/forms/resources/forms-test-util.js:97
> +formControlDesc.hiddentype.labelable = false;
> +formControlDesc.imagetype.listable = false;

identifier should be camelCase.  hiddentype -> hiddenType

> LayoutTests/fast/forms/resources/label-test-util.js:6
> +const WITH_NO_LABEL = 0;
> +const WITH_PARENT_LABEL = 1;
> +const WITH_SIBLING_LABEL = 2;
> +const WITH_SIBLING2_LABEL = 4;

Constant value names also should be camelCase.

> LayoutTests/fast/forms/resources/label-test-util.js:9
> +function createAllFormControls(labelRelation, preHtml, postHtml)

This name isn't represent the content.
appendAllFormControlsToBody(...) ?