Bug 49240

Created attachment 73354 [details] Patch

Add Dimitri and Darin who are familiar with form submission.

Comment on attachment 73354 [details] Patch 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.

Comment on attachment 73354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73354&action=review > LayoutTests/fast/forms/formaction-attribute-expected.txt:2 > + > +This test: PASSED! The test result should have a test description. > LayoutTests/fast/forms/formaction-attribute.html:4 > + if (window.layoutTestController) Indentation looks strange. 1 space? > LayoutTests/fast/forms/formaction-attribute.html:11 > +ul.document.getElementById('sourceViewDiv').innerHTML=' This test: FAILED 2' > +} > +function test2(ul) { > +ul.document.getElementById('sourceViewDiv').innerHTML=' This test: PASSED!' No indentation. No spaces around "=" We had better use functions in fast/js/resources/js-test-pre.js as possible. > LayoutTests/fast/forms/formaction-attribute.html:27 > +var x=document.getElementsByTagName('input')[0]; Need spaces around "=" > LayoutTests/fast/forms/formmethod-attribute-expected.txt:2 > + > +Success The test result should have a test description. > LayoutTests/fast/forms/formmethod-attribute.html:3 > +<meta http-equiv="content-type" content="text/html; charset=iso-8859-1"> Do we need this line? > LayoutTests/fast/forms/formmethod-attribute.html:24 > + if (document.URL.substring(document.URL.indexOf('?')+1, document.URL.length) == "") style: need spaces around "+" > LayoutTests/fast/forms/formmethod-attribute.html:25 > + document.write("<p>Success</p>"); We had better use functions in fast/js/resources/js-test-pre.js as possible. > LayoutTests/fast/forms/formmethod-attribute.html:35 > + document.write("<p>This test doesn't work directly from bugzilla, please save it to a local file first.</p>"); We don't need to mention it. > LayoutTests/fast/forms/formtarget-attribute.html:6 > + { Your JavaScript style is inconsistent: function name() { and function name() { Either is ok because we don't have explicit style rules for JavaScript, but you should be consistent. > LayoutTests/fast/forms/formtarget-attribute.html:10 > + // Not dumping as text since the DumpRenderTree does not dump the text content of child frames. We can access the content of iframe by iframe.contentDocument. We might need to call layoutTestContentroller.setAllowUniversalAccessFromFileURLs(true). > LayoutTests/fast/forms/mailto/formenctype-attribute.html:11 > + if (window.layoutTestController) { We usually use 4-space indentation. > WebCore/html/HTMLFormControlElement.cpp:87 > +String HTMLFormControlElement::formAction() const I don't think we need these access functions. JavaScript binding doesn't need them because of [Reflect], and HTMLFormElement can access them by getAttribute().

Comment on attachment 73354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73354&action=review >> LayoutTests/fast/forms/formaction-attribute-expected.txt:2 >> +This test: PASSED! > > The test result should have a test description. Thanks. Made it more descriptive. These four new tests were variations from existing tests with as small changes as possible. For this formaction-attribute.html, from form-action.html. >> LayoutTests/fast/forms/formtarget-attribute.html:8 >> + layoutTestController.waitUntilDone(); > > This definitely should be a dumpAsText test. Thanks. I chose dumpAsText and changed how to verify. >> WebCore/html/HTMLFormControlElement.cpp:87 >> +String HTMLFormControlElement::formAction() const > > I don't think we need these access functions. JavaScript binding doesn't need them because of [Reflect], and HTMLFormElement can access them by getAttribute(). Exactly. Removed. >> WebCore/html/HTMLFormElement.cpp:319 >> + 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? firstSuccessfulSubmitButton was different from this "button" object. In cases of my tests, firstSuccessfulSubmitButton was 0, but "button" pointed the button clicked. >> WebCore/html/HTMLFormElement.cpp:330 >> + } > > This whole thing looks like it should just live in FormSubmission::create. I guess it should be handled here for the same reason as copyFrom() below. >> WebCore/loader/FormSubmission.cpp:121 >> + > > And this becomes unnecessary. Copying is still required. Values of attributes form* (like formaction) must be parsed by parser methods (like FormSubmission::Attributes::parseAction()). But HTMLFormElement::m_attribute should not be modified.

Created attachment 73579 [details] Patch

(In reply to comment #5) > >> WebCore/html/HTMLFormElement.cpp:330 > >> + } > > > > This whole thing looks like it should just live in FormSubmission::create. > > I guess it should be handled here for the same reason as copyFrom() below. > > >> WebCore/loader/FormSubmission.cpp:121 > >> + > > > > And this becomes unnecessary. > > Copying is still required. Values of attributes form* (like formaction) must be parsed by parser methods (like FormSubmission::Attributes::parseAction()). But HTMLFormElement::m_attribute should not be modified. I understand the requirement of copyFrom(), but we can move the HTMLFormElement.cpp change to FormSubmission.cpp because FormSubmission::create() knows all of necessary information.

Comment on attachment 73579 [details] Patch 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()?

BTW, <button> should have these attributes too. http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-button-element

Comment on attachment 73579 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73579&action=review >> WebCore/html/HTMLFormControlElement.cpp:87 >> +String HTMLFormControlElement::formEnctype() const > > Why didn't you remove formEnctype() and setFormEnctype()? They are required since formenctype is [ConvertNullToNullString]. It followed the enctype attribute in form tags. (See HTMLFormElement.idl)

Created attachment 73588 [details] Patch

(In reply to comment #10) > (From update of attachment 73579 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73579&action=review > > >> WebCore/html/HTMLFormControlElement.cpp:87 > >> +String HTMLFormControlElement::formEnctype() const > > > > Why didn't you remove formEnctype() and setFormEnctype()? > > They are required since formenctype is [ConvertNullToNullString]. It followed the enctype attribute in form tags. (See HTMLFormElement.idl) [Reflect] for DOMString implies ConvertNullToNullString.

Comment on attachment 73588 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73588&action=review > LayoutTests/ChangeLog:5 > + Implement formaction, formenctype, formmethod and formtarget attributes for the input tag Now we support <button>. The summary should be updated. > WebCore/ChangeLog:5 > + Implement formaction, formenctype, formmethod and formtarget attributes for the input tag ditto.

(In reply to comment #12) When using ConvertNullToNullString, formEnctype() and setFormEnctype() are required to compile DerivedSources/WebCore/DOMHTMLFormControlElement.mm, a generated file from idl. This mm calls formEnctype and setFormEnctype.

Created attachment 73591 [details] Patch

(In reply to comment #14) > (In reply to comment #12) > When using ConvertNullToNullString, formEnctype() and setFormEnctype() are required to compile DerivedSources/WebCore/DOMHTMLFormControlElement.mm, a generated file from idl. This mm calls formEnctype and setFormEnctype. You don't need to add ConvertNullToNullString. attribute [Reflect] DOMString formEnctype; is enough. It contains ConvertNullToNullString behavior.

Comment on attachment 73591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73591&action=review > WebCore/html/HTMLInputElement.idl:31 > + attribute [Reflect, URL] DOMString formAction; > + attribute [ConvertNullToNullString] DOMString formEnctype; > + attribute [Reflect] DOMString formMethod; > attribute [Reflect] boolean formNoValidate; > + attribute [Reflect] DOMString formTarget; We need a similar change for HTMLButtonElement.idl. Also, we need tests for accessing them by JavaScript properties. e.g. input.formAction reflects formaction= HTML attribute correctly?

Created attachment 73704 [details] Patch

(In reply to comment #17) Kent-san, Thanks for the comments. Revised.

Comment on attachment 73704 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73704&action=review > LayoutTests/ChangeLog:33 > + * fast/forms/formmethod-attribute-button-html-expected.txt: Added. > + * fast/forms/formmethod-attribute-button-html.html: Added. > + * fast/forms/formmethod-attribute-button-js-expected.txt: Added. > + * fast/forms/formmethod-attribute-button-js.html: Added. > + * fast/forms/formmethod-attribute-input-html-expected.txt: Added. > + * fast/forms/formmethod-attribute-input-html.html: Added. > + * fast/forms/formmethod-attribute-input-js-expected.txt: Added. > + * fast/forms/formmethod-attribute-input-js.html: Added. > + * fast/forms/formtarget-attribute-button-html-expected.txt: Added. > + * fast/forms/formtarget-attribute-button-html.html: Added. > + * fast/forms/formtarget-attribute-button-js-expected.txt: Added. > + * fast/forms/formtarget-attribute-button-js.html: Added. > + * fast/forms/formtarget-attribute-input-html-expected.txt: Added. > + * fast/forms/formtarget-attribute-input-html.html: Added. > + * fast/forms/formtarget-attribute-input-js-expected.txt: Added. > + * fast/forms/formtarget-attribute-input-js.html: Added. > + * fast/forms/mailto/formenctype-attribute-button-html-expected.txt: Added. > + * fast/forms/mailto/formenctype-attribute-button-html.html: Added. > + * fast/forms/mailto/formenctype-attribute-button-js-expected.txt: Added. > + * fast/forms/mailto/formenctype-attribute-button-js.html: Added. > + * fast/forms/mailto/formenctype-attribute-input-html-expected.txt: Added. > + * fast/forms/mailto/formenctype-attribute-input-html.html: Added. > + * fast/forms/mailto/formenctype-attribute-input-js-expected.txt: Added. > + * fast/forms/mailto/formenctype-attribute-input-js.html: Added. I don't think we should test the same test cases with both of an HTML attribute and the corresponding DOM property. As for tests of form submission behavior, we should test it with either of the HTML attribute or the DOM property, and we should have additional tests for association of the HTML attribute and the DOM property like LayoutTests/fast/forms/script-tests/input-minmax.html.

(In reply to comment #20) > I don't think we should test the same test cases with both of an HTML attribute and the corresponding DOM property. because running a complex test case twice is not good for test performance.

Created attachment 73712 [details] Patch

Comment on attachment 73712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73712&action=review > LayoutTests/ChangeLog:21 > + * fast/forms/formaction-attribute-expected.txt: Added. > + * fast/forms/formaction-attribute.html: Added. > + * fast/forms/formmethod-attribute-button-html-expected.txt: Added. > + * fast/forms/formmethod-attribute-button-html.html: Added. > + * fast/forms/formmethod-attribute-input-html-expected.txt: Added. > + * fast/forms/formmethod-attribute-input-html.html: Added. > + * fast/forms/formtarget-attribute-button-html-expected.txt: Added. > + * fast/forms/formtarget-attribute-button-html.html: Added. > + * fast/forms/formtarget-attribute-input-html-expected.txt: Added. > + * fast/forms/formtarget-attribute-input-html.html: Added. > + * fast/forms/mailto/formenctype-attribute-button-html-expected.txt: Added. > + * fast/forms/mailto/formenctype-attribute-button-html.html: Added. > + * fast/forms/mailto/formenctype-attribute-input-html-expected.txt: Added. > + * fast/forms/mailto/formenctype-attribute-input-html.html: Added. There is no submit-form-attribute.html. > LayoutTests/fast/forms/script-tests/submit-form-attributes.js:6 > +input.type="submit"; nit: Need spaces around "=". > LayoutTests/fast/forms/script-tests/submit-form-attributes.js:100 > +button.type="submit"; ditto. > WebCore/ChangeLog:20 > + Tests: fast/forms/formaction-attribute.html > + fast/forms/formmethod-attribute-button-html.html > + fast/forms/formmethod-attribute-button-js.html > + fast/forms/formmethod-attribute-input-html.html > + fast/forms/formmethod-attribute-input-js.html > + fast/forms/formtarget-attribute-button-html.html > + fast/forms/formtarget-attribute-button-js.html > + fast/forms/formtarget-attribute-input-html.html > + fast/forms/formtarget-attribute-input-js.html > + fast/forms/mailto/formenctype-attribute-button-html.html > + fast/forms/mailto/formenctype-attribute-button-js.html > + fast/forms/mailto/formenctype-attribute-input-html.html > + fast/forms/mailto/formenctype-attribute-input-js.html Looks obsolete.

Created attachment 73715 [details] Patch

Comment on attachment 73715 [details] Patch Looks ok. Thank you!

The commit-queue encountered the following flaky tests while processing attachment 73715 [details]: animations/suspend-resume-animation.html http/tests/misc/empty-file-formdata.html Please file bugs against the tests. These tests were authored by cmarrin@apple.com and zimmermann@kde.org. The commit-queue is continuing to process your patch.

Comment on attachment 73715 [details] Patch Rejecting patch 73715 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: tests/inspector ...... http/tests/loading .................... http/tests/local ........ http/tests/local/blob ... http/tests/local/formdata .... http/tests/media ....... http/tests/messaging .. http/tests/mime ....... http/tests/misc ......................... http/tests/misc/empty-file-formdata.html -> failed Exiting early after 1 failures. 21119 tests run. 480.33s total testing time 21118 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 11 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/5752011

(In reply to comment #27) > http/tests/misc ......................... > http/tests/misc/empty-file-formdata.html -> failed > > Exiting early after 1 failures. 21119 tests run. > 480.33s total testing time > > 21118 test cases (99%) succeeded > 1 test case (<1%) had incorrect layout Mikurube-san, This commit-queue output means your patch broke empty-file-formdata.html. Would you investigate it please?

(In reply to comment #28) Yes, I'm checking it. Thanks for your notification.

Created attachment 73874 [details] Patch

(In reply to comment #28) Fixed. Added > m_isMultiPartForm = other.m_isMultiPartForm; in WebCore/loader/FormSubmission.cpp: FormSubmission::Attributes::copyFrom.

Comment on attachment 73874 [details] Patch > Fixed. Added > > m_isMultiPartForm = other.m_isMultiPartForm; > in WebCore/loader/FormSubmission.cpp: FormSubmission::Attributes::copyFrom. ok, I understand. Our tests have good coverage :-)

Comment on attachment 73874 [details] Patch Rejecting patch 73874 from commit-queue. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 73874]" exit_code: 2 Last 500 characters of output: th fuzz 3. patching file WebCore/html/HTMLAttributeNames.in Hunk #1 FAILED at 94. 1 out of 1 hunk FAILED -- saving rejects to file WebCore/html/HTMLAttributeNames.in.rej patching file WebCore/html/HTMLButtonElement.idl patching file WebCore/html/HTMLInputElement.idl patching file WebCore/loader/FormSubmission.cpp patching file WebCore/loader/FormSubmission.h Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kent Tamura', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/6002058

Created attachment 73882 [details] Patch

(In reply to comment #33) Looks like a conflict... Merged and re-submitted.

Kent-san, Thanks!

Comment on attachment 73882 [details] Patch Clearing flags on attachment: 73882 Committed r72003: <http://trac.webkit.org/changeset/72003>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/72003 might have broken Qt Linux Release The following tests are not passing: fast/forms/formaction-attribute.html fast/forms/formmethod-attribute-button-html.html fast/forms/formtarget-attribute-button-html.html fast/forms/mailto/formenctype-attribute-button-html.html fast/forms/mailto/formenctype-attribute-input-html.html

http://trac.webkit.org/changeset/72003 caused the test 'fast/forms/submit-form-attributes.html' to crash DumpRenderTree on the Leopard Intel Debug (Tests) bot. Filed https://bugs.webkit.org/show_bug.cgi?id=49562.