Bug 131694

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 Flags
proposed fix
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
proposed fix
darin: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
patch for landing none

Description Alexey Proskuryakov 2014-04-15 14:34:39 PDT
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.
Comment 1 Alexey Proskuryakov 2014-04-15 14:47:51 PDT
Created attachment 229403 [details]
proposed fix
Comment 2 WebKit Commit Bot 2014-04-15 14:50:05 PDT
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 3 Build Bot 2014-04-15 16:09:55 PDT
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
Comment 4 Build Bot 2014-04-15 16:09:58 PDT
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
Comment 5 Alexey Proskuryakov 2014-04-15 16:32:20 PDT
Created attachment 229416 [details]
proposed fix
Comment 6 WebKit Commit Bot 2014-04-15 16:34:26 PDT
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 7 Build Bot 2014-04-15 17:23:09 PDT
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
Comment 8 Build Bot 2014-04-15 17:23:11 PDT
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
Comment 9 Alexey Proskuryakov 2014-04-15 18:57:04 PDT
The WebKit2 failure reminds me that the test should be skipped (unimplemented functionality in WKTR). This patch is still up for review.
Comment 10 Darin Adler 2014-04-16 09:45:56 PDT
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.
Comment 11 Alexey Proskuryakov 2014-04-16 10:13:59 PDT
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!
Comment 12 WebKit Commit Bot 2014-04-16 10:16:00 PDT
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 13 WebKit Commit Bot 2014-04-16 11:50:06 PDT
Comment on attachment 229454 [details]
patch for landing

Clearing flags on attachment: 229454

Committed r167368: <http://trac.webkit.org/changeset/167368>
Comment 14 WebKit Commit Bot 2014-04-16 11:50:10 PDT
All reviewed patches have been landed.  Closing bug.