Bug 91231 – FormController, WebHistoryItem: Enable reading selected file names from document state

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Marja Hölttä]

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()

[Marja Hölttä]

Created attachment 152246 [details]
.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] 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?

[WebKit Review Bot]

Comment on attachment 152246 [details]
.

Attachment 152246 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13237129

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

(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

[Marja Hölttä]

Created attachment 156925 [details]
Patch

ok,

since it touches the WebKit API, we'll need to wait for Darin

[Marja Hölttä]

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

[Marja Hölttä]

Created attachment 157448 [details]
Patch

[Marja Hölttä]

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?

[Darin Fisher (:fishd, Google)]

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.

[Kent Tamura]

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.

[Marja Hölttä]

Created attachment 158251 [details]
Patch

[Marja Hölttä]

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

[Marja Hölttä]

Created attachment 158268 [details]
Patch

[Kent Tamura]

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().

[Build Bot]

Comment on attachment 158268 [details]
Patch

Attachment 158268 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13504109

[Build Bot]

Comment on attachment 158268 [details]
Patch

Attachment 158268 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13490533

[Marja Hölttä]

Created attachment 158299 [details]
Patch

[Marja Hölttä]

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

[Build Bot]

Comment on attachment 158299 [details]
Patch

Attachment 158299 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13502183

[Marja Hölttä]

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

[Marja Hölttä]

Created attachment 158333 [details]
Patch

[Marja Hölttä]

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

[Kent Tamura]

Comment on attachment 158333 [details]
Patch

Looks ok.

[WebKit Review Bot]

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

[Marja Hölttä]

Created attachment 158744 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158744 [details]
Patch

Clearing flags on attachment: 158744

Committed r125759: <http://trac.webkit.org/changeset/125759>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.