Bug 80719 - Allow WebFileChooser to return extra file info (like displayName) in addition to mere file paths
Summary: Allow WebFileChooser to return extra file info (like displayName) in addition...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kinuko Yasuda
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-09 12:41 PST by Kinuko Yasuda
Modified: 2012-03-13 07:07 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.75 KB, patch)
2012-03-09 12:52 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (16.37 KB, patch)
2012-03-09 15:30 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
build fix (for EWS) (16.41 KB, patch)
2012-03-12 21:00 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (17.79 KB, patch)
2012-03-12 21:20 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (18.01 KB, patch)
2012-03-12 22:28 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (19.35 KB, patch)
2012-03-12 22:49 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (19.25 KB, patch)
2012-03-12 23:25 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (19.75 KB, patch)
2012-03-13 00:22 PDT, Kinuko Yasuda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2012-03-09 12:41:24 PST
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.
Comment 1 Kinuko Yasuda 2012-03-09 12:52:08 PST
Created attachment 131078 [details]
Patch
Comment 2 Kinuko Yasuda 2012-03-09 12:52:50 PST
This is only for WebKit API changes-- the callsites and WebCore part also need to be changed.
Comment 3 WebKit Review Bot 2012-03-09 12:59:57 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 Darin Fisher (:fishd, Google) 2012-03-09 14:30:34 PST
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"!)
Comment 5 Kinuko Yasuda 2012-03-09 15:30:42 PST
Created attachment 131125 [details]
Patch
Comment 6 Kinuko Yasuda 2012-03-09 15:38:50 PST
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".
Comment 7 Early Warning System Bot 2012-03-09 16:25:51 PST
Comment on attachment 131125 [details]
Patch

Attachment 131125 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11901024
Comment 8 Early Warning System Bot 2012-03-09 16:51:55 PST
Comment on attachment 131125 [details]
Patch

Attachment 131125 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11892795
Comment 9 WebKit Review Bot 2012-03-09 17:30:39 PST
Comment on attachment 131125 [details]
Patch

Attachment 131125 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11904828
Comment 10 Gyuyoung Kim 2012-03-09 18:06:51 PST
Comment on attachment 131125 [details]
Patch

Attachment 131125 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11911900
Comment 11 Kent Tamura 2012-03-11 17:55:15 PDT
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.
Comment 12 Kinuko Yasuda 2012-03-12 21:00:04 PDT
Created attachment 131504 [details]
build fix (for EWS)
Comment 13 Kinuko Yasuda 2012-03-12 21:13:34 PDT
(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.
Comment 14 Kinuko Yasuda 2012-03-12 21:20:54 PDT
Created attachment 131507 [details]
Patch
Comment 15 Kent Tamura 2012-03-12 21:58:40 PDT
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.
Comment 16 Kinuko Yasuda 2012-03-12 22:28:51 PDT
Created attachment 131546 [details]
Patch
Comment 17 Gustavo Noronha (kov) 2012-03-12 22:40:21 PDT
Comment on attachment 131546 [details]
Patch

Attachment 131546 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11949267
Comment 18 Kinuko Yasuda 2012-03-12 22:40:56 PDT
Comment on attachment 131546 [details]
Patch

Oops, sorry this has wrong change...
Comment 19 Kinuko Yasuda 2012-03-12 22:49:31 PDT
Created attachment 131549 [details]
Patch
Comment 20 Kent Tamura 2012-03-12 23:18:39 PDT
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")
Comment 21 Kinuko Yasuda 2012-03-12 23:25:58 PDT
Created attachment 131555 [details]
Patch
Comment 22 Kinuko Yasuda 2012-03-12 23:28:30 PDT
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.
Comment 23 Build Bot 2012-03-12 23:35:59 PDT
Comment on attachment 131555 [details]
Patch

Attachment 131555 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11949280
Comment 24 Kent Tamura 2012-03-12 23:44:19 PDT
(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.
Comment 25 Collabora GTK+ EWS bot 2012-03-13 00:06:41 PDT
Comment on attachment 131555 [details]
Patch

Attachment 131555 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11944405
Comment 26 Kinuko Yasuda 2012-03-13 00:22:09 PDT
Created attachment 131562 [details]
Patch
Comment 27 Kinuko Yasuda 2012-03-13 00:35:10 PDT
(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.
Comment 28 Kent Tamura 2012-03-13 01:29:51 PDT
Comment on attachment 131562 [details]
Patch

Looks ok.
Comment 29 WebKit Review Bot 2012-03-13 07:07:06 PDT
Comment on attachment 131562 [details]
Patch

Clearing flags on attachment: 131562

Committed r110557: <http://trac.webkit.org/changeset/110557>
Comment 30 WebKit Review Bot 2012-03-13 07:07:13 PDT
All reviewed patches have been landed.  Closing bug.