| Summary: | FileInputType doesn't support (save|restore)FormControlState | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Marja Hölttä <marja> | ||||||||||||||||||||||
| Component: | Forms | Assignee: | Marja Hölttä <marja> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | ap, beidson, jochen, jonlee, tkent, webkit.review.bot | ||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Marja Hölttä
2012-03-02 04:13:14 PST
Created attachment 129874 [details]
Patch
Comment on attachment 129874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129874&action=review > Source/WebCore/ChangeLog:1 > +2012-03-02 Marja Hölttä <marja@chromium.org> character encoding? > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) can you write a unit test for this, and explain in the changelog why this change is needed? Created attachment 130571 [details]
Patch
Comment on attachment 130571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130571&action=review > Source/WebCore/html/FileInputType.cpp:104 > + result = String(); Can you change this so result is not touched, if you return false? > LayoutTests/ChangeLog:12 > + * platform/wk2/Skipped: can you add a comment here why the test is skipped on wk2? > LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:37 > + eventSender.beginDragWithFiles(["resources/test.txt"]); can you protect this by if (window.eventSender)? And if window.layotuTestController is not defined, set the result div to "Needs to be run with DumpRenderTree" and don't try to execute the test Created attachment 130577 [details]
.
Thanks for comments, the new patch addresses them. Comment on attachment 130577 [details] . LGTM with nits View in context: https://bugs.webkit.org/attachment.cgi?id=130577&action=review > Source/WebCore/html/FileInputType.cpp:112 > + return !m_fileList->isEmpty(); return true > LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:32 > + if (!window.layoutTestController) since you exit early, I think you can skip the layoutTestController and eventSender tests below Created attachment 130580 [details]
.
Comment on attachment 130580 [details] . View in context: https://bugs.webkit.org/attachment.cgi?id=130580&action=review > Source/WebCore/html/FileInputType.cpp:119 > + element()->setFiles(files); HTMLInputElement::setFiles() is unnecessary. filesChosen() can be called here. > LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:52 > + var xhr = new XMLHttpRequest(); > + xhr.open("POST", "http://127.0.0.1:8000/xmlhttprequest/resources/multipart-post-echo.php", false); > + xhr.send(new FormData(document.getElementById("form"))); > + var expectedResponse = "posted-text=text to be posted&posted-file=test.txt:text/plain:Hello"; > + if (xhr.responseText = expectedResponse) Why do you need to submit the form? Checking the value of file-input isn't enough? Comment on attachment 130580 [details] . View in context: https://bugs.webkit.org/attachment.cgi?id=130580&action=review >> Source/WebCore/html/FileInputType.cpp:119 >> + element()->setFiles(files); > > HTMLInputElement::setFiles() is unnecessary. filesChosen() can be called here. This function is const and filesChosen is not. Other InputTypes, e.g., BaseCheckableInputType circumvent this by calling a setter in HTMLElement, and I followed that pattern. Should I instead have made InputType::restoreFormControlState non-const? >> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:52 >> + if (xhr.responseText = expectedResponse) > > Why do you need to submit the form? Checking the value of file-input isn't enough? The value is obscured by FileInputType::getTypeSpecificValue(). I want to ensure in the test that we store and restore the real value, not this fake value. Comment on attachment 130580 [details] . View in context: https://bugs.webkit.org/attachment.cgi?id=130580&action=review >>> Source/WebCore/html/FileInputType.cpp:119 >>> + element()->setFiles(files); >> >> HTMLInputElement::setFiles() is unnecessary. filesChosen() can be called here. > > This function is const and filesChosen is not. > > Other InputTypes, e.g., BaseCheckableInputType circumvent this by calling a setter in HTMLElement, and I followed that pattern. > > Should I instead have made InputType::restoreFormControlState non-const? Ah, I see. It's ok to make this non-const. 'restore' should be non-const essentially. >>> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:52 >>> + if (xhr.responseText = expectedResponse) >> >> Why do you need to submit the form? Checking the value of file-input isn't enough? > > The value is obscured by FileInputType::getTypeSpecificValue(). I want to ensure in the test that we store and restore the real value, not this fake value. It's fake, but you can confirm the basename, can't you? Also, we have HTMLInputElement::files. Created attachment 130593 [details]
Patch
Created attachment 130596 [details]
Patch.
Created attachment 130597 [details]
Patch
Created attachment 130599 [details]
Patch.
Created attachment 130600 [details]
Patch.
Comment on attachment 130580 [details] . View in context: https://bugs.webkit.org/attachment.cgi?id=130580&action=review >>>> Source/WebCore/html/FileInputType.cpp:119 >>>> + element()->setFiles(files); >>> >>> HTMLInputElement::setFiles() is unnecessary. filesChosen() can be called here. >> >> This function is const and filesChosen is not. >> >> Other InputTypes, e.g., BaseCheckableInputType circumvent this by calling a setter in HTMLElement, and I followed that pattern. >> >> Should I instead have made InputType::restoreFormControlState non-const? > > Ah, I see. > > It's ok to make this non-const. 'restore' should be non-const essentially. Ok, I made it non-const. >>>> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:52 >>>> + if (xhr.responseText = expectedResponse) >>> >>> Why do you need to submit the form? Checking the value of file-input isn't enough? >> >> The value is obscured by FileInputType::getTypeSpecificValue(). I want to ensure in the test that we store and restore the real value, not this fake value. > > It's fake, but you can confirm the basename, can't you? Also, we have HTMLInputElement::files. Ok, I changed this so that it reads the file contents via the File object retrieved from HTMLInputElement::files, and moved the test to fast/forms. Comment on attachment 130600 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=130600&action=review > Source/WebCore/ChangeLog:1 > +2012-03-07 Marja Hölttä <marja@google.com> Looks like the encoding got messed up again. > Source/WebCore/html/FileInputType.cpp:110 > + result.append('\0'); Are we sure this works for multiple files? Can we add a test, or extend the current one, with a multiple file input? > LayoutTests/ChangeLog:10 > + * fast/forms/recover-file-input-in-unposted-form-expected.txt: Added. I think we've been moving toward using the subdirectories for each individual input type, so it really should be in fast/forms/file. > LayoutTests/fast/forms/recover-file-input-in-unposted-form.html:53 > + if (fileString == "Hello" && document.getElementById('text-input').value == "text to be posted") The test ought to be using shouldBe(). (checkout file-input-change-event.html for an example) Comment on attachment 130571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130571&action=review Did someone check that this works in IE and Firefox? >> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:37 >> + eventSender.beginDragWithFiles(["resources/test.txt"]); > > can you protect this by if (window.eventSender)? And if window.layotuTestController is not defined, set the result div to "Needs to be run with DumpRenderTree" and don't try to execute the test It's best to make tests that can be run manually, and it's also easy to achieve here. Just tell the tester to select any file manually, and what to expect for PASS/FAIL. Created attachment 130797 [details]
Patch.
Comment on attachment 130600 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=130600&action=review >> Source/WebCore/ChangeLog:1 >> +2012-03-07 Marja Hölttä <marja@google.com> > > Looks like the encoding got messed up again. Afaics this is only a problem in the diff viewer, not a problem in the file itself. The file is (valid) UTF-8. >> Source/WebCore/html/FileInputType.cpp:110 >> + result.append('\0'); > > Are we sure this works for multiple files? Can we add a test, or extend the current one, with a multiple file input? I extended the test to test also multiple file inputs. >> LayoutTests/ChangeLog:10 >> + * fast/forms/recover-file-input-in-unposted-form-expected.txt: Added. > > I think we've been moving toward using the subdirectories for each individual input type, so it really should be in fast/forms/file. Done. >> LayoutTests/fast/forms/recover-file-input-in-unposted-form.html:53 >> + if (fileString == "Hello" && document.getElementById('text-input').value == "text to be posted") > > The test ought to be using shouldBe(). (checkout file-input-change-event.html for an example) Done. Comment on attachment 130571 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130571&action=review >> Source/WebCore/html/FileInputType.cpp:104 >> + result = String(); > > Can you change this so result is not touched, if you return false? Done. >> LayoutTests/ChangeLog:12 >> + * platform/wk2/Skipped: > > can you add a comment here why the test is skipped on wk2? Done. >>> LayoutTests/http/tests/local/formdata/recover-file-input-in-unposted-form.html:37 >>> + eventSender.beginDragWithFiles(["resources/test.txt"]); >> >> can you protect this by if (window.eventSender)? And if window.layotuTestController is not defined, set the result div to "Needs to be run with DumpRenderTree" and don't try to execute the test > > It's best to make tests that can be run manually, and it's also easy to achieve here. Just tell the tester to select any file manually, and what to expect for PASS/FAIL. Made the test manually runnable + added instructions. (In reply to comment #19) > Did someone check that this works in IE and Firefox? Works in Firefox, doesn't work in IE 9 (it recovers the text fields but not the file inputs). Comment on attachment 130797 [details]
Patch.
ok
Comment on attachment 130797 [details] Patch. Clearing flags on attachment: 130797 Committed r110275: <http://trac.webkit.org/changeset/110275> All reviewed patches have been landed. Closing bug. |