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.
Created attachment 131550 [details] Patch
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?
Created attachment 131561 [details] Patch
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.
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.
Created attachment 131564 [details] Patch
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.
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?
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.
Created attachment 131569 [details] Addressed comments; for EWS
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
Comment on attachment 131564 [details] Patch Attachment 131564 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11941454
Comment on attachment 131564 [details] Patch Attachment 131564 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11949344
Comment on attachment 131569 [details] Addressed comments; for EWS Attachment 131569 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11949351
Created attachment 131586 [details] Build fix (for EWS)
Comment on attachment 131564 [details] Patch Attachment 131564 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11944536
Comment on attachment 131564 [details] Patch Attachment 131564 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11948486
Comment on attachment 131586 [details] Build fix (for EWS) Attachment 131586 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11941552
Comment on attachment 131586 [details] Build fix (for EWS) Attachment 131586 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11947536
Committed r110560: <http://trac.webkit.org/changeset/110560>