Bug 63039 – FileChooser should be only created when we need to choose files.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

FileChooser should be only created when we need to choose files.

[Dimitri Glazkov (Google)]

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

[Early Warning System Bot]

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

[Dimitri Glazkov (Google)]

Created attachment 97929 [details]
Qt build fix.

[Dimitri Glazkov (Google)]

Created attachment 97934 [details]
Energize.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kent Tamura]

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

[Dimitri Glazkov (Google)]

Created attachment 98114 [details]
Silly mistake fixed.

[Dimitri Glazkov (Google)]

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

[Kent Tamura]

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.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

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

[Joseph Pecoraro]

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.

[Kent Tamura]

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.