Bug 80970

Summary:

File upload control should use File.name() rather than File.path() to show chosen filenames

Product:

WebKit

Reporter:

Kinuko Yasuda <kinuko>

Component:

WebCore Misc.

Assignee:

Kinuko Yasuda <kinuko>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

gustavo, satorux, tkent, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	tkent: review+, buildbot: commit-queue-
Addressed comments; for EWS	buildbot: commit-queue-
Build fix (for EWS)	webkit-ews: commit-queue-

Kinuko Yasuda

Reported 2012-03-12 22:35:33 PDT

File upload control should use File.name() rather than File.path() to show chosen filenames. In some rare cases (e.g. files from FileSystem API or files created using the newly added WebKit API) File.name has different displayName from the basename of file.path, and in the file uploader controller we should use File.name rather than File.path.

Attachments
Patch (5.28 KB, patch) 2012-03-12 22:52 PDT, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Patch (12.05 KB, patch) 2012-03-13 00:03 PDT, Kinuko Yasuda	no flags	Details Formatted Diff Diff
Patch (11.97 KB, patch) 2012-03-13 00:29 PDT, Kinuko Yasuda	tkent: review+ buildbot: commit-queue-	Details Formatted Diff Diff
Addressed comments; for EWS (11.50 KB, patch) 2012-03-13 00:58 PDT, Kinuko Yasuda	buildbot: commit-queue-	Details Formatted Diff Diff
Build fix (for EWS) (11.87 KB, patch) 2012-03-13 03:50 PDT, Kinuko Yasuda	webkit-ews: commit-queue-	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Kinuko Yasuda

Comment 1 2012-03-12 22:52:27 PDT

Created attachment 131550 [details] Patch

Kent Tamura

Comment 2 2012-03-12 23:31:30 PDT

Comment on attachment 131550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131550&action=review > Source/WebCore/rendering/RenderFileUploadControl.cpp:239 > - return theme()->fileListNameForWidth(input->files()->paths(), style()->font(), maxFilenameWidth(), input->multiple()); > + return theme()->fileListNameForWidth(input->files()->names(), style()->font(), maxFilenameWidth(), input->multiple()); I think this is not a compatible change. RenderThemeMac.mm uses NSFileMaanger::displayNameAtPath to make a user-visible string. It requires a full path name. How about passing a FileList to fileListNameForWidth() instead of a Vector of paths?

Kinuko Yasuda

Comment 3 2012-03-13 00:03:33 PDT

Created attachment 131561 [details] Patch

Kinuko Yasuda

Comment 4 2012-03-13 00:10:58 PDT

Comment on attachment 131550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131550&action=review >> Source/WebCore/rendering/RenderFileUploadControl.cpp:239 >> + return theme()->fileListNameForWidth(input->files()->names(), style()->font(), maxFilenameWidth(), input->multiple()); > > I think this is not a compatible change. > RenderThemeMac.mm uses NSFileMaanger::displayNameAtPath to make a user-visible string. It requires a full path name. > > How about passing a FileList to fileListNameForWidth() instead of a Vector of paths? (google'd how NSFileManager::displayNameAtPath works...) I see, passing a FileList sounds like a better idea (then each platform can do what they want). Updated.

Kent Tamura

Comment 5 2012-03-13 00:22:45 PDT

Comment on attachment 131561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131561&action=review > Source/WebCore/rendering/RenderTheme.h:219 > - virtual String fileListNameForWidth(const Vector<String>& filenames, const Font&, int width, bool multipleFilesAllowed) const; > + virtual String fileListNameForWidth(PassRefPtr<FileList>, const Font&, int width, bool multipleFilesAllowed) const; Using PassRefPtr<> is not appropriate because we don't need to move reference count ownership from the caller to fileListNameForWidth. FileList* is enough.

Kinuko Yasuda

Comment 6 2012-03-13 00:29:30 PDT

Created attachment 131564 [details] Patch

Kinuko Yasuda

Comment 7 2012-03-13 00:32:07 PDT

Comment on attachment 131561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131561&action=review >> Source/WebCore/rendering/RenderTheme.h:219 >> + virtual String fileListNameForWidth(PassRefPtr<FileList>, const Font&, int width, bool multipleFilesAllowed) const; > > Using PassRefPtr<> is not appropriate because we don't need to move reference count ownership from the caller to fileListNameForWidth. > FileList* is enough. Good point, I should have thought about that. Fixed.

Kent Tamura

Comment 8 2012-03-13 00:37:27 PDT

Comment on attachment 131564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131564&action=review > Source/WebCore/ChangeLog:3 > + [chromium] File upload control should use File.name() rather than File.path() to show chosen filenames Please do not prepend [Chrmoum] for a patch updating non-Chromium part. > Source/WebCore/rendering/RenderTheme.h:219 > - virtual String fileListNameForWidth(const Vector<String>& filenames, const Font&, int width, bool multipleFilesAllowed) const; > + virtual String fileListNameForWidth(FileList*, const Font&, int width, bool multipleFilesAllowed) const; nit: FileList* can be const?

Kent Tamura

Comment 9 2012-03-13 00:38:22 PDT

Comment on attachment 131564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131564&action=review > Source/WebCore/platform/gtk/RenderThemeGtk.h:184 > - virtual String fileListNameForWidth(const Vector<String>& filenames, const Font&, int width, bool multipleFilesAllowed) const; > + virtual String fileListNameForWidth(FileList*, const Font&, int width, bool multipleFilesAllowed) const; nit: We had better append OVERRIDE. > Source/WebCore/platform/qt/RenderThemeQt.h:158 > - virtual String fileListNameForWidth(const Vector<String>& filenames, const Font&, int width) const; > + virtual String fileListNameForWidth(FileList*, const Font&, int width) const; ditto. > Source/WebCore/rendering/RenderThemeMac.h:180 > - virtual String fileListNameForWidth(const Vector<String>& filenames, const Font&, int width, bool multipleFilesAllowed) const; > + virtual String fileListNameForWidth(FileList*, const Font&, int width, bool multipleFilesAllowed) const; nit: We had better append OVERRIDE.

Kinuko Yasuda

Comment 10 2012-03-13 00:58:02 PDT

Created attachment 131569 [details] Addressed comments; for EWS

Build Bot

Comment 11 2012-03-13 02:31:51 PDT

Comment on attachment 131569 [details] Addressed comments; for EWS Attachment 131569 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11942424

Build Bot

Comment 12 2012-03-13 02:39:29 PDT

Comment on attachment 131564 [details] Patch Attachment 131564 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11941454

Gustavo Noronha (kov)

Comment 13 2012-03-13 02:43:36 PDT

Comment on attachment 131564 [details] Patch Attachment 131564 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11949344