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

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov 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 73354: Patch
https://bugs.webkit.org/attachment.cgi?id=73354&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=73354&action=review

Just  a few comments.

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

This definitely should be a dumpAsText test.

> WebCore/html/HTMLFormElement.cpp:319
> +    if (event && event->target() && event->target()->toNode())
> +	   button =
static_cast<HTMLFormControlElement*>(event->target()->toNode());

This seems like a different method from obtaining the button from above (see
firstSuccessfulSubmitButton logic). What's the difference?

> WebCore/html/HTMLFormElement.cpp:330
> +    if (button) {
> +	   String attribute;
> +	   if (!(attribute = button->formAction()).isNull())
> +	       copiedAttributes.parseAction(attribute);
> +	   if (!(attribute = button->formEnctype()).isNull())
> +	       copiedAttributes.parseEncodingType(attribute);
> +	   if (!(attribute = button->formMethod()).isNull())
> +	       copiedAttributes.parseMethodType(attribute);
> +	   if (!(attribute = button->formTarget()).isNull())
> +	       copiedAttributes.setTarget(attribute);
> +    }

This whole thing looks like it should just live in FormSubmission::create.

> WebCore/loader/FormSubmission.cpp:121
> +void FormSubmission::Attributes::copyFrom(const Attributes& other)
> +{
> +    m_method = other.m_method;
> +    m_action = other.m_action;
> +    m_target = other.m_target;
> +    m_encodingType = other.m_encodingType;
> +    m_acceptCharset = other.m_acceptCharset;
> +}
> +

And this becomes unnecessary.