Bug 63039

Created attachment 97913 [details] WIP:Cook on bots.

Comment on attachment 97913 [details] WIP:Cook on bots. Attachment 97913 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8908736

Created attachment 97929 [details] Qt build fix.

Created attachment 97934 [details] Energize.

Comment on attachment 97934 [details] Energize. Attachment 97934 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8913813 New failing tests: fast/files/apply-blob-url-to-xhr.html fast/forms/get-file-upload.html editing/pasteboard/file-input-files-access.html fast/files/file-reader-abort.html fast/files/apply-blob-url-to-img.html

Created attachment 97936 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 97934 [details] Energize. View in context: https://bugs.webkit.org/attachment.cgi?id=97934&action=review > Source/WebCore/rendering/RenderFileUploadControl.cpp:130 > +#if ENABLE(DIRECTORY_UPLOAD) > + settings.allowsDirectoryUpload = input->fastHasAttribute(webkitdirectoryAttr); > + settings.allowsMultipleFiles = true; Looks wrong. allowMultipleFiles should be false if !fastHasAttribute(webkitdirectoryAttr) && !fastHasAttribute(multipleAttr).

Created attachment 98114 [details] Silly mistake fixed.

Doh! Thanks for catching this. Can you take a look again?

Comment on attachment 98114 [details] Silly mistake fixed. View in context: https://bugs.webkit.org/attachment.cgi?id=98114&action=review > Source/WebCore/ChangeLog:24 > + 1) Introduce FileChooserSettings to decouple setting querying from > + FileChooser. It's a simple copyable settings object, which allows us > + to capture the settings atomically and treat them as discardable data. > + > + 2) Encapsulate lifetime management of FileChooser entirely in > + FileChooserClient. It's now a "smart" client, and allows us to > + completely remove FileChooser management concerns from a FileChooserClient > + implementor. > + > + 3) Change creation of FileChooser to be on-demand, only when we actually > + need to choose file. > + > + 4) Rearrange calling of dispatchFormControlChangeEvent to be at the end > + of a function and remove "am-I-dead" checks that are now unnecessary. > + > + 5) Clean up directory upload code a bit, and make use of FileChooserSettings > + to pass directory name. In general, doing 5 things in a single patch is not good :-) > Source/WebCore/rendering/RenderFileUploadControl.cpp:57 > +static void filenamesFromFileList(FileList* list, Vector<String>& filenames) FileList* should be const FileList*. > Source/WebCore/rendering/RenderFileUploadControl.cpp:94 > + inputElement->dispatchFormControlChangeEvent(); nit: We had better have a comment that this dispatchFormControlChangeEvent() call should be put on the bottom of the function.

Comment on attachment 98114 [details] Silly mistake fixed. View in context: https://bugs.webkit.org/attachment.cgi?id=98114&action=review >> Source/WebCore/ChangeLog:24 >> + to pass directory name. > > In general, doing 5 things in a single patch is not good :-) I agree! I really got carried away here. Thought the first 4 would be very hard to untangle into separate patches. >> Source/WebCore/rendering/RenderFileUploadControl.cpp:57 >> +static void filenamesFromFileList(FileList* list, Vector<String>& filenames) > > FileList* should be const FileList*. Sounds good. >> Source/WebCore/rendering/RenderFileUploadControl.cpp:94 >> + inputElement->dispatchFormControlChangeEvent(); > > nit: We had better have a comment that this dispatchFormControlChangeEvent() call should be put on the bottom of the function. Sure! Will do.

Committed r89430: <http://trac.webkit.org/changeset/89430>

Comment on attachment 98114 [details] Silly mistake fixed. View in context: https://bugs.webkit.org/attachment.cgi?id=98114&action=review > Source/WebCore/rendering/RenderFileUploadControl.cpp:106 > + if (Chrome* chrome = this->chrome()) { > + FileChooserSettings settings; > + settings.allowsDirectoryUpload = true; > + settings.allowsMultipleFiles = true; > + settings.selectedFiles.append(paths[0]); > + chrome->enumerateChosenDirectory(newFileChooser(settings)); > + } Is there a particular reason that this FileChooser doesn't set the acceptTypes? It looks like the accept="..." mime type filter would get ignored in this case, but it is respected with the click/runOpenPanel case.

Comment on attachment 98114 [details] Silly mistake fixed. View in context: https://bugs.webkit.org/attachment.cgi?id=98114&action=review >> Source/WebCore/rendering/RenderFileUploadControl.cpp:106 >> + } > > Is there a particular reason that this FileChooser doesn't set the acceptTypes? > It looks like the accept="..." mime type filter would get ignored in this case, > but it is respected with the click/runOpenPanel case. There is no reason. We had better pass acceptTypes. Chromium's enumeratChosenDirectory() implementaion doesn't have the acceptTypes parameter for now. So adding acceptTypes here is harmless.