Chromium would need to read the selected file names from the document state, in order to set the file permissions properly after restoring a page which contains a form with selected files. The patch I'll soon add adds WebDocumentState which has a function for getting the file names. Migration plan: Patch 1, webkit: WebDocumentState WebHistoryItem::documentState(bool) << added WebVector<WebString> WebHistoryItem::documentState() << old one Patch 2, chromium: use documentState(true) Patch 3, webkit: WebDocumentState WebHistoryItem::documentState(bool = true) Patch 4: chromium: use documentState() Patch 5, webkit: WebDocumentState WebHistoryItem::documentState()
Created attachment 152246 [details] .
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 152246 [details] . Can you add a test that this works? See tests/WebViewTest for how to easily create a HTMLDocument Also, a ChangeLog is missing :) View in context: https://bugs.webkit.org/attachment.cgi?id=152246&action=review > Source/WebKit/chromium/public/WebDocumentState.h:39 > + WebDocumentState(const WebVector<WebString>&); explicit? > Source/WebKit/chromium/public/WebHistoryItem.h:106 > WEBKIT_EXPORT WebVector<WebString> documentState() const; I'd group the new and old methods together and add a // FIXME: comment about removing the old ones and dropping the bool > Source/WebKit/chromium/src/WebDocumentState.cpp:37 > + : m_data(documentState) the constructor and the data() accessor are so small, you could move them to the header > Source/WebKit/chromium/src/WebHistoryItem.cpp:221 > +void WebHistoryItem::setDocumentState(const WebDocumentState& state) can you implement the old setDocumentState / documentState by means of the new methods, so we don't have a copy of the code?
Comment on attachment 152246 [details] . Attachment 152246 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13237129
Comment on attachment 152246 [details] . Is exposing a WebDocumentState primitive really the best thing to do? I've always found the concept of "document state" rather vague / cryptic. Maybe it just needs a better name, or maybe the functionality that you really need could / should just be a method on WebHistoryItem?
(In reply to comment #5) > (From update of attachment 152246 [details]) > Is exposing a WebDocumentState primitive really the best thing to do? I've always > found the concept of "document state" rather vague / cryptic. Maybe it just needs > a better name, or maybe the functionality that you really need could / should just > be a method on WebHistoryItem? The intention is to allow access to the files for the file input elements restored from the document state (see https://chromiumcodereview.appspot.com/10695107/) without deserializing the document state in webkit/glue
(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 152246 [details] [details]) > > Is exposing a WebDocumentState primitive really the best thing to do? I've always > > found the concept of "document state" rather vague / cryptic. Maybe it just needs > > a better name, or maybe the functionality that you really need could / should just > > be a method on WebHistoryItem? > > The intention is to allow access to the files for the file input elements restored from the document state (see https://chromiumcodereview.appspot.com/10695107/) without deserializing the document state in webkit/glue WebHistoryItem::getReferencedFilePaths?
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > (From update of attachment 152246 [details] [details] [details]) > > > Is exposing a WebDocumentState primitive really the best thing to do? I've always > > > found the concept of "document state" rather vague / cryptic. Maybe it just needs > > > a better name, or maybe the functionality that you really need could / should just > > > be a method on WebHistoryItem? > > > > The intention is to allow access to the files for the file input elements restored from the document state (see https://chromiumcodereview.appspot.com/10695107/) without deserializing the document state in webkit/glue > > WebHistoryItem::getReferencedFilePaths? sg
Created attachment 156925 [details] Patch
ok, since it touches the WebKit API, we'll need to wait for Darin
Created attachment 157429 [details] Patch
Comment on attachment 157429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157429&action=review > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2355 > + result->setNull(); why not ->set("") ? > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2356 > + WebHistoryItem historyItem = m_shell->webViewHost()->webView()->focusedFrame()->currentHistoryItem(); m_shell->webView() should be enough > LayoutTests/platform/chromium/fast/forms/file/selected-files-from-history-state.html:13 > + testFailed('This test requires a LayoutTestController.'); it's called TestRunner these days > LayoutTests/platform/chromium/fast/forms/file/selected-files-from-history-state.html:43 > +<p id="description"></p> please add a short description what the test is supposed to do
Created attachment 157448 [details] Patch
Thanks for comments, the new patch addresses all of them.
(In reply to comment #14) > Thanks for comments, the new patch addresses all of them. Thanks Darin, are the WebKit API changes ok?
Comment on attachment 157448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157448&action=review > Source/WebKit/chromium/src/WebHistoryItem.cpp:290 > +WebVector<WebString> WebHistoryItem::getReferredFilePaths() const nit: getReferencedFilePaths(). getReferredFilePaths() sounds a bit awkward to me. did you have a reason for preferring "referred" here? just because it is shorter? > Source/WebKit/chromium/src/WebHistoryItem.cpp:292 > + WTF::Vector<WebString> filePaths; nit: no need for WTF:: in .cpp files > Source/WebKit/chromium/src/WebHistoryItem.cpp:295 > + WebHTTPBody::Element element; nit: prefer using WebCore types inside the implementation of WebKit. There are hidden costs to using WebKit API here. > Source/WebKit/chromium/src/WebHistoryItem.cpp:302 > + const WebVector<WebString>& state = documentState(); it is a bit wasteful to allocate a WebVector here. you should just use Vector<String> as returned by m_private->documentState() to avoid copying the array. > Source/WebKit/chromium/src/WebHistoryItem.cpp:304 > + while (i + 2 < state.size()) { you should explain what these magic numbers are all about. > Source/WebKit/chromium/src/WebHistoryItem.cpp:317 > + WebVector<WebString> toReturn(filePaths.size()); nit: you can just do "return filePaths;" and it will automagically create a WebVector<WebString> for you. > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2353 > +void LayoutTestController::getReferredFiles(const CppArgumentList& arguments, CppVariant* result) can you do this by extending Internals.idl instead? we try to avoid adding new methods on LayoutTestController (now TestRunner) when possible.
Comment on attachment 157448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157448&action=review > Source/WebKit/chromium/src/WebHistoryItem.cpp:316 > + const WebVector<WebString>& state = documentState(); > + size_t i = 3; > + while (i + 2 < state.size()) { > + bool ok = false; > + size_t fields = WTF::String(state[i + 2]).toInt(&ok); > + if (!ok) > + break; > + if (i + 2 + fields >= state.size()) > + break; > + if (state[i + 1] == "file") { > + for (size_t j = i + 3; j < i + 3 + fields; j += 2) > + filePaths.append(state[j]); > + } > + i += (3 + fields); > + } This is not a good idea. The internal structure of documentState() is not exposed from WebCore and it is changeable. Actually, this code is for an old version of the structure and it doesn't work now. I recommend to add a function to WebCore/html/FormController.{cpp,h}. The function deserializes SavedFormState objects from the documentState vector, collects the file paths in SavedFormState, and returns them.
Created attachment 158251 [details] Patch
Comment on attachment 157448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157448&action=review >> Source/WebKit/chromium/src/WebHistoryItem.cpp:290 >> +WebVector<WebString> WebHistoryItem::getReferredFilePaths() const > > nit: getReferencedFilePaths(). getReferredFilePaths() sounds a bit awkward > to me. did you have a reason for preferring "referred" here? just because > it is shorter? Changed to getReferencedFilePaths. >> Source/WebKit/chromium/src/WebHistoryItem.cpp:292 >> + WTF::Vector<WebString> filePaths; > > nit: no need for WTF:: in .cpp files Done >> Source/WebKit/chromium/src/WebHistoryItem.cpp:295 >> + WebHTTPBody::Element element; > > nit: prefer using WebCore types inside the implementation of WebKit. > There are hidden costs to using WebKit API here. Done. >> Source/WebKit/chromium/src/WebHistoryItem.cpp:302 >> + const WebVector<WebString>& state = documentState(); > > it is a bit wasteful to allocate a WebVector here. you should just > use Vector<String> as returned by m_private->documentState() to avoid > copying the array. Done. >> Source/WebKit/chromium/src/WebHistoryItem.cpp:304 >> + while (i + 2 < state.size()) { > > you should explain what these magic numbers are all about. The code was moved to FormController / SavedFormState / FormControlState -> less magic numbers. >> Source/WebKit/chromium/src/WebHistoryItem.cpp:316 >> + } > > This is not a good idea. > The internal structure of documentState() is not exposed from WebCore and it is changeable. Actually, this code is for an old version of the structure and it doesn't work now. > > I recommend to add a function to WebCore/html/FormController.{cpp,h}. The function deserializes SavedFormState objects from the documentState vector, collects the file paths in SavedFormState, and returns them. Moved the logic to FormController / SavedFormState / FormControlState. (The serialization format has changed recently but afaics this uses the new version.) >> Source/WebKit/chromium/src/WebHistoryItem.cpp:317 >> + WebVector<WebString> toReturn(filePaths.size()); > > nit: you can just do "return filePaths;" and it will automagically > create a WebVector<WebString> for you. Done >> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:2353 >> +void LayoutTestController::getReferredFiles(const CppArgumentList& arguments, CppVariant* result) > > can you do this by extending Internals.idl instead? we try to avoid adding new methods > on LayoutTestController (now TestRunner) when possible. Done
Comment on attachment 158251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158251&action=review > Source/WebCore/ChangeLog:18 > + (WebCore::FormController::getReferencedFilePaths): please add a comment to the newly added methods explaining what they do > Source/WebCore/html/FormController.cpp:81 > + for (size_t i = 0; i < m_values.size(); i += 2) { this seems to assume that the FormControlState is representing a file input which is not necessarily true. I think I'd prefer a static function that does the magic of getting the filepaths out of the form control state. It could be something like getReferencedFilePathsForFileFormControlState(FormControlState&) or so > Source/WebCore/html/FormController.cpp:287 > + const Vector<String>& files = queIterator->getReferencedFilePaths(); just toReturn.append(queIterator->getReferencedFilePaths()) > Source/WebCore/html/FormController.cpp:522 > + const Vector<String>& filePaths = state->getReferencedFilePaths(); just toReturn.append(state->getReferencedFilePaths()) > Source/WebCore/testing/Internals.cpp:1198 > + for (size_t i = 0; i < filePaths.size(); ++i) you could Vector<String>& result = stringList; result.append(filePaths)
You should probably also update the bug title now that it's not a WebHistoryItem-only change anymore
Created attachment 158268 [details] Patch
Comment on attachment 158251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158251&action=review > Source/WebCore/html/FormController.cpp:84 > +Vector<String> FormControlState::getReferencedFilePaths() const > +{ > + Vector<String> toReturn; > + for (size_t i = 0; i < m_values.size(); i += 2) { > + if (!m_values[i].isNull()) > + toReturn.append(m_values[i]); > + } This logic should be in FileInputType.cpp because of encapsulation. > Source/WebCore/html/FormController.cpp:521 > +Vector<String> FormController::getReferencedFilePaths(const Vector<String>& stateVector) > +{ > + Vector<String> toReturn; > + size_t i = 0; > + if (stateVector.size() < 1 || stateVector[i++] != formStateSignature()) > + return toReturn; > + while (i + 1 < stateVector.size()) { > + ++i; // Skip form key. > + OwnPtr<SavedFormState> state = SavedFormState::deserialize(stateVector, i); > + if (!state) > + break; We should share the code with FormController::setStateForNewFormElements().
Comment on attachment 158268 [details] Patch Attachment 158268 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13504109
Comment on attachment 158268 [details] Patch Attachment 158268 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13490533
Created attachment 158299 [details] Patch
Comment on attachment 158251 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158251&action=review >> Source/WebCore/ChangeLog:18 >> + (WebCore::FormController::getReferencedFilePaths): > > please add a comment to the newly added methods explaining what they do Done >> Source/WebCore/html/FormController.cpp:81 >> + for (size_t i = 0; i < m_values.size(); i += 2) { > > this seems to assume that the FormControlState is representing a file input which is not necessarily true. I think I'd prefer a static function that does the magic of getting the filepaths out of the form control state. It could be something like getReferencedFilePathsForFileFormControlState(FormControlState&) or so The logic is moved to FileInputType.cpp >> Source/WebCore/html/FormController.cpp:84 >> + } > > This logic should be in FileInputType.cpp because of encapsulation. Done >> Source/WebCore/html/FormController.cpp:521 >> + break; > > We should share the code with FormController::setStateForNewFormElements(). Done
Comment on attachment 158299 [details] Patch Attachment 158299 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13502183
Created attachment 158316 [details] Patch
Comment on attachment 158316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158316&action=review > Source/WebCore/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). You forgot to copy this over from the last version > ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). same here
Created attachment 158333 [details] Patch
Comment on attachment 158316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158316&action=review >> Source/WebCore/ChangeLog:8 >> + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > > You forgot to copy this over from the last version Fixed >> ChangeLog:8 >> + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > > same here Fixed
Comment on attachment 158333 [details] Patch let's wait for darin or tkent before cq
Comment on attachment 158333 [details] Patch Looks ok.
Comment on attachment 158333 [details] Patch Rejecting attachment 158333 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ucceeded at 71 (offset 1 line). patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/forms/file/selected-files-from-history-state-expected.txt patching file LayoutTests/fast/forms/file/selected-files-from-history-state.html patching file ChangeLog Hunk #1 succeeded at 1 with fuzz 3. Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Jochen Eis..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13509508
Created attachment 158744 [details] Patch
Comment on attachment 158744 [details] Patch Clearing flags on attachment: 158744 Committed r125759: <http://trac.webkit.org/changeset/125759>
All reviewed patches have been landed. Closing bug.