Summary: | DataTransfer should cache its FileList | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Alexey Proskuryakov <ap> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, darin, rniwa | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2014-04-15 14:34:39 PDT
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. |