Bug 108263 – formMethod to have empty string as default value and 'get' as invalid.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Vineet Chaudhary (vineetc)]

Specification: http://www.whatwg.org/specs/web-apps/current-work/#form-submission-0
             : http://www.w3.org/html/wg/drafts/html/master/forms.html#attr-fs-formmethod
Mozilla bug  : https://bugzilla.mozilla.org/show_bug.cgi?id=787095

The spec says formmethod should only have an invalid value default, not a missing value default.
Tests affected : fast/forms/submit-form-attributes.html

[Vineet Chaudhary (vineetc)]

Created attachment 185354 [details]
proposed_patch

[Vineet Chaudhary (vineetc)]

Setting review flag to get initial comments.
If this change makes sense then similar is also applies to formenctype.

[Build Bot]

Comment on attachment 185354 [details]
proposed_patch

Attachment 185354 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16151470

New failing tests:
fast/forms/formmethod-attribute-test.html

[WebKit Review Bot]

Comment on attachment 185354 [details]
proposed_patch

Attachment 185354 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16196389

New failing tests:
fast/forms/formmethod-attribute-test.html

[Build Bot]

Comment on attachment 185354 [details]
proposed_patch

Attachment 185354 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16115516

New failing tests:
fast/forms/formmethod-attribute-test.html

[Build Bot]

Comment on attachment 185354 [details]
proposed_patch

Attachment 185354 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16218284

New failing tests:
fast/forms/formmethod-attribute-test.html

[Darin Adler]

Comment on attachment 185354 [details]
proposed_patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185354&action=review

Looks OK, but needs a little improvement.

> Source/WebCore/html/HTMLFormControlElement.cpp:90
> +    String formMethod = fastGetAttribute(formmethodAttr);

The result of fastGetAttribute is const AtomicString&, not String. Putting it in a String creates unnecessary reference count churn.

> Source/WebCore/html/HTMLFormControlElement.cpp:92
> +        return "";

The efficient way to return an empty string is emptyString(). Returning "" will result in extra code including a call to strlen.

[Vineet Chaudhary (vineetc)]

Created attachment 185532 [details]
updated_patch

Thanks darin for review comments.

[Kent Tamura]

Comment on attachment 185532 [details]
updated_patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185532&action=review

> Source/WebCore/html/HTMLFormControlElement.cpp:92
> +        return String();

Why did you change this line to String(), not emptyString() Darin recommended?

> LayoutTests/fast/forms/formmethod-attribute-test.html:21
> +shouldBe('ip1.formMethod', '""');

Please use shouldBeEqualToString to avoid ugly nested quotes.

> LayoutTests/fast/forms/formmethod-attribute-test.html:28
> +shouldBe('ip2.formMethod', '"get"');
> +shouldBe('ip2.getAttribute("formmethod")', '""');

ditto.

> LayoutTests/fast/forms/formmethod-attribute-test.html:34
> +shouldBe('ip3.formMethod', '"post"');
> +shouldBe('ip3.getAttribute("formmethod")', '"post"');

ditto.

> LayoutTests/fast/forms/formmethod-attribute-test.html:40
> +shouldBe('ip4.formMethod', '"get"');
> +shouldBe('ip4.getAttribute("formmethod")', '"get"');

ditto.

> LayoutTests/fast/forms/formmethod-attribute-test.html:46
> +shouldBe('ip5.formMethod', '"get"');
> +shouldBe('ip5.getAttribute("formmethod")', '"foo"');

ditto.

[Kent Tamura]

Comment on attachment 185532 [details]
updated_patch

View in context: https://bugs.webkit.org/attachment.cgi?id=185532&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=108263
> +        formMethod to have empty string as default value and 'get' as invalid.

Please put the description line first.

> LayoutTests/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=108263
> +        formMethod to have empty string as default value and 'get' as invalid.

ditto.

[Vineet Chaudhary (vineetc)]

Created attachment 185658 [details]
updated_patch

(In reply to comment #9)
> > Source/WebCore/html/HTMLFormControlElement.cpp:92
> > +        return String();
> 
> Why did you change this line to String(), not emptyString() Darin recommended?

I interpreted it wrong. Done!!
 
> > LayoutTests/fast/forms/formmethod-attribute-test.html:21
> > +shouldBe('ip1.formMethod', '""');
> 
> Please use shouldBeEqualToString to avoid ugly nested quotes.

Done. There are still calls shouldBe() instead of shouldBeEqualToString()
in another file ../forms/submit-form-attributes.html in this patch but I wasn't sure to change those
but I can do that in followup patch for formenctype if required.

> > Source/WebCore/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=108263
> > +        formMethod to have empty string as default value and 'get' as invalid.
> Please put the description line first.

Done. Thank you for comments.

[Kent Tamura]

Comment on attachment 185658 [details]
updated_patch

ok

[WebKit Review Bot]

Comment on attachment 185658 [details]
updated_patch

Clearing flags on attachment: 185658

Committed r141405: <http://trac.webkit.org/changeset/141405>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.