Per HTML5, "The files attribute must return a live FileList sequence consisting of File objects representing the files found by the following steps. The same object must be returned each time. Furthermore, for a given FileList object and a given underlying file, the same File object must be used each time." Besides spec compliance, this is important because initializing File objects can be expensive, so we shouldn't dispose of them easily.
Created attachment 229403 [details] proposed fix
Attachment 229403 [details] did not pass style-queue: ERROR: Source/WebCore/fileapi/FileList.h:58: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 229403 [details] proposed fix Attachment 229403 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6402081439088640 New failing tests: http/tests/security/clipboard/clipboard-file-access.html
Created attachment 229413 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 229416 [details] proposed fix
Attachment 229416 [details] did not pass style-queue: ERROR: Source/WebCore/fileapi/FileList.h:58: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 229416 [details] proposed fix Attachment 229416 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5764541862379520 New failing tests: fast/events/data-transfer-files-attribute-identity.html
Created attachment 229418 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
The WebKit2 failure reminds me that the test should be skipped (unimplemented functionality in WKTR). This patch is still up for review.
Comment on attachment 229416 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=229416&action=review > Source/WebCore/dom/DataTransfer.cpp:182 > Vector<String> filenames = m_pasteboard->readFilenames(); I don’t think we need to put this vector into a local variable. I’d just write: for (const String& filename : m_pasteboard->readFilenames()) Actually, I’d really write: for (auto& filename : m_pasteboard->readFilenames()) But I know you prefer to explicitly state additional type names in cases like this. I wouldn’t keep the local variable, though, even though it means that the Vector<String> type is no longer seen here explicitly. > Source/WebCore/dom/DataTransfer.cpp:184 > + for (const String& filename : filenames) > + m_fileList->append(File::create(filename, File::AllContentTypes)); If the filenames vector is empty, won’t that cause us to call readFilenames multiple times for the same DataTransfer object? Maybe we need a separate boolean rather than using m_fileList->isEmpty()? Or maybe we should use the special value of null to indicate that m_fileList hasn’t been cached.
Created attachment 229454 [details] patch for landing > If the filenames vector is empty, won’t that cause us to call readFilenames multiple times for the same DataTransfer object? Good catch, thank you!
Attachment 229454 [details] did not pass style-queue: ERROR: Source/WebCore/fileapi/FileList.h:58: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 229454 [details] patch for landing Clearing flags on attachment: 229454 Committed r167368: <http://trac.webkit.org/changeset/167368>
All reviewed patches have been landed. Closing bug.