[webkit-reviews] review denied: [Bug 49240] Implement formaction, formenctype, formmethod and formtarget attributes for the input tag : [Attachment 73579] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Dai Mikurube
<dmikurube at google.com>'s request for review:
Bug 49240: Implement formaction, formenctype, formmethod and formtarget
attributes for the input tag
https://bugs.webkit.org/show_bug.cgi?id=49240

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

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

> LayoutTests/fast/forms/formaction-attribute.html:12
> +if (window.layoutTestController)
> +    layoutTestController.dumpAsText();

You dont need to do this.  js-test-pre.js does it.

> LayoutTests/fast/forms/formaction-attribute.html:14
> +doneAction = false;

Prepend "var ".

> LayoutTests/fast/forms/formaction-attribute.html:29
> +<script>

Why does this HTML have two <script>s?
I think you can merge them into one.

> LayoutTests/fast/forms/formaction-attribute.html:32
> +var x = document.getElementsByTagName('input')[0];

"x" is not a good name.

> LayoutTests/fast/forms/formaction-attribute.html:36
> +successfullyParsed = true;

Prepend "var ".

> LayoutTests/fast/forms/formmethod-attribute-expected.txt:1
> +PASS The formmethod attribute was successfully used

Please add a test description.	You can use description() defined by
js-test-pre.js

> LayoutTests/fast/forms/formmethod-attribute.html:14
> +<script>

add description() call please.

> LayoutTests/fast/forms/formmethod-attribute.html:16
> +    layoutTestController.dumpAsText();

You don't need to call dumpAsText() because of js-test-pre.js.

> LayoutTests/fast/forms/formmethod-attribute.html:35
> +successfullyParsed = true;

add "var ".

> LayoutTests/fast/forms/formtarget-attribute.html:7
> +<script>

You can merge this <script> block into the <script> below.

> LayoutTests/fast/forms/formtarget-attribute.html:12
> +	   layoutTestController.dumpAsText();

You don't need to call dumpAsText() because of js-test-pre.js.

> WebCore/ChangeLog:19
> +	   (WebCore::HTMLFormControlElement::formAction):
> +	   (WebCore::HTMLFormControlElement::formEnctype):
> +	   (WebCore::HTMLFormControlElement::setFormEnctype):
> +	   (WebCore::HTMLFormControlElement::formMethod):
> +	   (WebCore::HTMLFormControlElement::formTarget):

Please update the ChangeLog.  This block refers to non-existent functions.

> WebCore/html/HTMLFormControlElement.cpp:87
> +String HTMLFormControlElement::formEnctype() const

Why didn't you remove formEnctype() and setFormEnctype()?