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
Created attachment 185354 [details] proposed_patch
Setting review flag to get initial comments. If this change makes sense then similar is also applies to formenctype.
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 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 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 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 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.
Created attachment 185532 [details] updated_patch Thanks darin for review comments.
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 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.
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 on attachment 185658 [details] updated_patch ok
Comment on attachment 185658 [details] updated_patch Clearing flags on attachment: 185658 Committed r141405: <http://trac.webkit.org/changeset/141405>
All reviewed patches have been landed. Closing bug.