Bug 108263

Summary: formMethod to have empty string as default value and 'get' as invalid.
Product: WebKit Reporter: Vineet Chaudhary (vineetc) <code.vineet>
Component: FormsAssignee: Vineet Chaudhary (vineetc) <code.vineet>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dglazkov, mifenton, ojan.autocc, rniwa, tkent, webkit.review.bot
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.whatwg.org/specs/web-apps/current-work/#form-submission-0
Attachments:
Description Flags
proposed_patch
darin: review-, buildbot: commit-queue-
updated_patch
none
updated_patch none

Description Vineet Chaudhary (vineetc) 2013-01-29 17:08:28 PST
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
Comment 1 Vineet Chaudhary (vineetc) 2013-01-29 17:21:46 PST
Created attachment 185354 [details]
proposed_patch
Comment 2 Vineet Chaudhary (vineetc) 2013-01-29 17:46:34 PST
Setting review flag to get initial comments.
If this change makes sense then similar is also applies to formenctype.
Comment 3 Build Bot 2013-01-29 18:27:23 PST
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
Comment 4 WebKit Review Bot 2013-01-29 19:07:09 PST
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
Comment 5 Build Bot 2013-01-29 20:13:39 PST
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
Comment 6 Build Bot 2013-01-29 20:31:58 PST
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
Comment 7 Darin Adler 2013-01-30 09:17:17 PST
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.
Comment 8 Vineet Chaudhary (vineetc) 2013-01-30 12:06:06 PST
Created attachment 185532 [details]
updated_patch

Thanks darin for review comments.
Comment 9 Kent Tamura 2013-01-30 18:05:51 PST
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.
Comment 10 Kent Tamura 2013-01-30 18:16:09 PST
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.
Comment 11 Vineet Chaudhary (vineetc) 2013-01-30 19:20:14 PST
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.
Comment 12 Kent Tamura 2013-01-30 23:39:52 PST
Comment on attachment 185658 [details]
updated_patch

ok
Comment 13 WebKit Review Bot 2013-01-31 05:03:32 PST
Comment on attachment 185658 [details]
updated_patch

Clearing flags on attachment: 185658

Committed r141405: <http://trac.webkit.org/changeset/141405>
Comment 14 WebKit Review Bot 2013-01-31 05:03:37 PST
All reviewed patches have been landed.  Closing bug.