Allow WebFileChooser to return extra file info (like displayName) in addition to mere file paths

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

Allow WebFileChooser to return extra file info (like displayName) in addition to mere file paths.

On some filesystems/platforms we want to use a separate display name from the base part of the given platform file path.

E.g. we may want to keep a snapshot file of the selected file in a temporary directory with a cryptic name while it's being processed, but want to use a different human-readable display name for the name that is exposed as File.name or as filename= field in the content disposition.

[Kinuko Yasuda]

Created attachment 131078 [details]
Patch

[Kinuko Yasuda]

This is only for WebKit API changes-- the callsites and WebCore part also need to be changed.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 131078 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131078&action=review

> Source/WebKit/chromium/public/WebFileChooserCompletion.h:55
>      virtual void didChooseFile(const WebVector<WebString>& fileNames) = 0;

is the idea that didChooseFile will be deprecated and removed?  assuming that is the
case, then you might just want to use the same name for the new method.  (although,
perhaps it should be "didChooseFiles" instead of "didChooseFile"!)

[Kinuko Yasuda]

Created attachment 131125 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 131078 [details]
Patch

Thanks.  I added corresponding WebCore changes with some FIXME comments.  The change may feel a bit noisy as we need to keep the existing methods as well not to break other ports.

View in context: https://bugs.webkit.org/attachment.cgi?id=131078&action=review

> Source/WebKit/chromium/public/WebFileChooserCompletion.h:55
>      virtual void didChooseFile(const WebVector<WebString>& fileNames) = 0;

Either one of the method should be deprecated.  I think probably we should eventually just rewind what we're doing now... I added FIXME comment about that, renamed it to didChooseFile, and also added another comment that we should consider renaming it to "didChooseFiles".

[Early Warning System Bot]

Comment on attachment 131125 [details]
Patch

Attachment 131125 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11901024

[Early Warning System Bot]

Comment on attachment 131125 [details]
Patch

Attachment 131125 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11892795

[WebKit Review Bot]

Comment on attachment 131125 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 131125 [details]
Patch

Attachment 131125 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11911900

[Kent Tamura]

Comment on attachment 131125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131125&action=review

> Source/WebCore/html/FileInputType.cpp:364
> +    for (unsigned i = 0; i < paths.size(); ++i) {
> +      FileChooserFileInfo file;
> +      file.path = paths[i];
> +      files.append(file);
> +    }

Wrong indentation.

I recommend to add constructors to FileChooserFileInfo, and make FileChooserFileInfo immutable.

[Kinuko Yasuda]

Created attachment 131504 [details]
build fix (for EWS)

[Kinuko Yasuda]

(In reply to comment #11)
> (From update of attachment 131125 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131125&action=review
> 
> > Source/WebCore/html/FileInputType.cpp:364
> > +    for (unsigned i = 0; i < paths.size(); ++i) {
> > +      FileChooserFileInfo file;
> > +      file.path = paths[i];
> > +      files.append(file);
> > +    }
> 
> Wrong indentation.
> 
> I recommend to add constructors to FileChooserFileInfo, and make FileChooserFileInfo immutable.

Will fix this and re-upload the patch.

[Kinuko Yasuda]

Created attachment 131507 [details]
Patch

[Kent Tamura]

Comment on attachment 131507 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131507&action=review

> Source/WebCore/html/FileInputType.cpp:127
> +    Vector<FileChooserFileInfo> files;
> +    Vector<String> pathAndNames;
> +    state.split('\0', pathAndNames);
> +    for (unsigned i = 0; i < pathAndNames.size();) {
> +        ASSERT(i + 1 < pathAndNames.size());
> +        String path = pathAndNames[i++];
> +        String name = pathAndNames[i++];
> +        files.append(FileChooserFileInfo(path, name));
> +    }

We can't do this.
It's possible that a form control state saved by a browser without this patch, and it is restored by a browser with this patch.

> Source/WebKit/chromium/src/WebFileChooserCompletionImpl.cpp:51
> +    for (unsigned i = 0; i < fileNames.size(); ++i) {
> +      SelectedFileInfo file;
> +      file.path = fileNames[i];
> +      files.append(file);

Wrong indentation.

[Kinuko Yasuda]

Created attachment 131546 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 131546 [details]
Patch

Attachment 131546 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11949267

[Kinuko Yasuda]

Comment on attachment 131546 [details]
Patch

Oops, sorry this has wrong change...

[Kinuko Yasuda]

Created attachment 131549 [details]
Patch

[Kent Tamura]

Comment on attachment 131549 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131549&action=review

> Source/WebCore/html/FileInputType.cpp:110
> +        result.append("\xFF\xFE");

This is interpreted as ISO-8859-1, and U+00FF and U+00FE are valid characters.
I recommend to use U+FFFF.

String::fromUTF8("\xEF\xBF\xBF")

[Kinuko Yasuda]

Created attachment 131555 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 131549 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131549&action=review

>> Source/WebCore/html/FileInputType.cpp:110
>> +        result.append("\xFF\xFE");
> 
> This is interpreted as ISO-8859-1, and U+00FF and U+00FE are valid characters.
> I recommend to use U+FFFF.
> 
> String::fromUTF8("\xEF\xBF\xBF")

Great, thanks for the comment!!  Updated.

[Build Bot]

Comment on attachment 131555 [details]
Patch

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

[Kent Tamura]

(In reply to comment #22)
> (From update of attachment 131549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131549&action=review
> 
> >> Source/WebCore/html/FileInputType.cpp:110
> >> +        result.append("\xFF\xFE");
> > 
> > This is interpreted as ISO-8859-1, and U+00FF and U+00FE are valid characters.
> > I recommend to use U+FFFF.
> > 
> > String::fromUTF8("\xEF\xBF\xBF")
> 
> Great, thanks for the comment!!  Updated.

I'm sorry that U+FFFF is also a wrong choice because it is an invalid character according to the standard.  Some string conversion code might break this characters.
I recommend another control characters such as \1.

[Collabora GTK+ EWS bot]

Comment on attachment 131555 [details]
Patch

Attachment 131555 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11944405

[Kinuko Yasuda]

Created attachment 131562 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #24)
> (In reply to comment #22)
> > (From update of attachment 131549 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=131549&action=review
> > 
> > >> Source/WebCore/html/FileInputType.cpp:110
> > >> +        result.append("\xFF\xFE");
> > > 
> > > This is interpreted as ISO-8859-1, and U+00FF and U+00FE are valid characters.
> > > I recommend to use U+FFFF.
> > > 
> > > String::fromUTF8("\xEF\xBF\xBF")
> > 
> > Great, thanks for the comment!!  Updated.
> 
> I'm sorry that U+FFFF is also a wrong choice because it is an invalid character according to the standard.  Some string conversion code might break this characters.
> I recommend another control characters such as \1.

Thanks, character handling is difficult.  Updated.

[Kent Tamura]

Comment on attachment 131562 [details]
Patch

Looks ok.

[WebKit Review Bot]

Comment on attachment 131562 [details]
Patch

Clearing flags on attachment: 131562

Committed r110557: <http://trac.webkit.org/changeset/110557>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.