Bug 58401

Created attachment 89318 [details] Patch

This is confusing. I can drag and drop folders onto webkitdirectory inputs perfectly fine in Safari 5. What does this patch enable? + virtual void enumerateDirectory(const WTF::String&, PassRefPtr<WebCore::FileChooser>); This almost certainly shouldn't be a PassRefPtr.

Comment on attachment 89318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89318&action=review > Source/WebKit/chromium/public/WebViewClient.h:175 > + virtual bool enumerateDirectory(const WebString& path, enumerateChosenDirectory might be a better name here. you want to convey the fact that only chosen directories may be enumerated, right? any reason not to name it like this? this function shouldn't live in the "editing" section of WebViewClient. it might be nice to group it with runFileChooser, since the two functions are related. however, that lives in a section for "dialogs" :( maybe put this in the "misc" section nearby the queryIconForFiles method?

Alexey: You can always drag a directory onto the input, but what happens is that you get a single File with path = that directory. On a webkitdirectory input in Chromium, where DIRECTORY_UPLOAD is enabled, that's inconsistent with what happens when you select the directory through the selection dialog, where the directory is enumerated and you get a list of all the Files with the webkitRelativePath attribute. This patch makes drag-n-drop consistent with selecting a directory through the dialog, by doing the equivalent enumeration. > + virtual void enumerateDirectory(const WTF::String&, PassRefPtr<WebCore::FileChooser>); > This almost certainly shouldn't be a PassRefPtr. Can you clarify? It's the same pattern as runOpenPanel(). Darin: I will rearrange.

Created attachment 89400 [details] Patch

(In reply to comment #4) > Alexey: You can always drag a directory onto the input, but what happens is that you get a single File with path = that directory. That's a requirement of HTML5. "Unless the multiple attribute is set, there must be no more than one file in the list of selected files." Was webkitdirectory discussed on appropriate mailing lists? There are many ways one could upload a directory, for example, it can be archived as a .zip file. Is there a spec for webkitdirectory that I could read to figure out what the idea behind it is? > > + virtual void enumerateDirectory(const WTF::String&, PassRefPtr<WebCore::FileChooser>); > > This almost certainly shouldn't be a PassRefPtr. > > Can you clarify? It's the same pattern as runOpenPanel(). PassRefPtr should only be used when passing ownership, and neither enumerateDirectory() nor runOpenPanel() pass FileChooser ownership. See e.g. RenderFileUploadControl::click() - it passes m_fileChooser, obviously keeping ownership for itself. So, runOpenPanel() should be changed to take a raw pointer (or a reference if the argument can't be null).

(In reply to comment #6) > (In reply to comment #4) > > Alexey: You can always drag a directory onto the input, but what happens is that you get a single File with path = that directory. > > That's a requirement of HTML5. "Unless the multiple attribute is set, there must be no more than one file in the list of selected files." > > Was webkitdirectory discussed on appropriate mailing lists? There are many ways one could upload a directory, for example, it can be archived as a .zip file. Is there a spec for webkitdirectory that I could read to figure out what the idea behind it is? It was discussed at some length about a year ago -- indeed there were many options proposed, but the conclusion was to proceed with an experimental implementation. There is no official spec, but the behavior is as described in the original proposal. Regarding the semantics of multiple, I would think "directory" (aka "webkitdirectory") implies "multiple". [http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-April/025764.html] > > > > + virtual void enumerateDirectory(const WTF::String&, PassRefPtr<WebCore::FileChooser>); > > > This almost certainly shouldn't be a PassRefPtr. > > > > Can you clarify? It's the same pattern as runOpenPanel(). > > PassRefPtr should only be used when passing ownership, and neither enumerateDirectory() nor runOpenPanel() pass FileChooser ownership. See e.g. RenderFileUploadControl::click() - it passes m_fileChooser, obviously keeping ownership for itself. > > So, runOpenPanel() should be changed to take a raw pointer (or a reference if the argument can't be null). Makes sense. I will address this in the patch.

Thanks, I've read through the e-mail thread now. This is not the right place to discuss it, but since there is no other discussion going on anywhere at the moment, I'll dump my concerns here. With this proposal, there will be separate "multiple" and "directory" file input modifications, and they will be limited in different ways. Users will be able to upload multiple arbitrary files with the first variety, and they'll only be able to upload a single full directory with the second. I don't see how users could possibly ever learn the difference, or what the use case for allowing only the latter is. Well, there is a use case for only uploading full directories, and that's OS X document bundles, which look like normal files to users. Safari already handles that case by creating a .zip archive on upload, so there is no need for new attributes for that.

(In reply to comment #8) > Thanks, I've read through the e-mail thread now. > > This is not the right place to discuss it, but since there is no other discussion going on anywhere at the moment, I'll dump my concerns here. I'll respond to your concerns, but I agree it would be more appropriate to address the concerns elsewhere, perhaps in the context of evaluating the experiment for something spec'd and permanent. My goal with this bug is to fix a clear deficiency of the experimental feature. > > With this proposal, there will be separate "multiple" and "directory" file input modifications, and they will be limited in different ways. Users will be able to upload multiple arbitrary files with the first variety, and they'll only be able to upload a single full directory with the second. I don't see how users could possibly ever learn the difference, or what the use case for allowing only the latter is. This is true, it is up to the web application to use the right control and present it in a clear way. That's a limitation of file inputs and the traditional UI that browsers use to present them (the multiple attribute is also not apparent to the user in Safari Chrome Firefox). I agree with the comments on the original thread that better UI will help, such as an input that accepts anything dragged on it and has buttons for selecting and/or appending anything through dialogs. One outcome could be that the webkitdirectory work is eventually rolled into that, but that is a larger undertaking. > > Well, there is a use case for only uploading full directories, and that's OS X document bundles, which look like normal files to users. Safari already handles that case by creating a .zip archive on upload, so there is no need for new attributes for that. The premise of this feature is that there are use cases for having a input with the semantics of representing a full directory. Using a .zip file does not achieve that -- it has the semantics of a single file, and particularly doesn't expose the hierarchy to the web application.

> My goal with this bug is to fix a clear deficiency of the experimental feature. Given that the code in this patch is all under an #if, I don't have an objection to it. Do you have a plan for the feature to either come out of experimental stage, or be dropped? With the information I have, I would consider dropping it soon - with the UI complications that it has, there doesn't seem to much chance of it being enabled by default or in many ports.

Returning to a more immediate question, I took another look at the RefPtr usage and I think the use of PassRefPtr is preferable here. The enumeration is asynchronous, so in Chromium we put the FileChooser into a completion object (both here and in runOpenPanel) and we want to guarantee that FileChooser remains alive until completion, using a RefPtr<FileChooser>. Putting a raw pointer in that completion object appears to risk a use-after-free bug if the RenderFileUploadControl is destroyed when the file chooser finishes. Most other ports do runOpenPanel in a synchronous fashion -- WebKit2 is async, and puts the raw pointer in a completion object, maybe that's safe -- but the point is it's very hard to tell by code inspection. It seems better to use refcounting in this case (given what FileChooser::disconnectClient() does to allow files to come back after the RenderFileUploadControl goes away).

I disagree. The implementation may or may not temporarily protect the pointer, but this has nothing to do with the idea of taking ownership, or with the performance optimization from PassRefPtr. It's certainly fine to use RefPtr in the completion object, but that doesn't mean that a function argument can be a PassRefPtr.

Having PassRefPtr as an argument is very dangerous, and a common source of mistakes - see e.g. bug 58383 that was fixed today.

Created attachment 89511 [details] Patch

(In reply to comment #13) > Having PassRefPtr as an argument is very dangerous, and a common source of mistakes - see e.g. bug 58383 that was fixed today. Okay, I'm fine with using your approach here. Patch updated to not use PassRefPtr.

Comment on attachment 89511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89511&action=review > Source/WebCore/rendering/RenderFileUploadControl.cpp:117 > + return; gratuitous return statement.

Created attachment 90163 [details] Patch

Comment on attachment 90163 [details] Patch same patch with extra return; removed.

Comment on attachment 90163 [details] Patch Clearing flags on attachment: 90163 Committed r84238: <http://trac.webkit.org/changeset/84238>

The commit-queue encountered the following flaky tests while processing attachment 90163 [details]: fast/canvas/webgl/gl-teximage.html bug 58766 (authors: kbr@google.com, mihaip@chromium.org, and zmo@google.com) fast/dom/onerror-img.html bug 51019 The commit-queue is continuing to process your patch.

Comment on attachment 89511 [details] Patch A attachment obsoleted this one.

Patch was committed in #19.