Bug 131694 – DataTransfer should cache its FileList

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Created attachment 229403 [details]
proposed fix

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Alexey Proskuryakov]

Created attachment 229416 [details]
proposed fix

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Alexey Proskuryakov]

The WebKit2 failure reminds me that the test should be skipped (unimplemented functionality in WKTR). This patch is still up for review.

[Darin Adler]

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.

[Alexey Proskuryakov]

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!

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 229454 [details]
patch for landing

Clearing flags on attachment: 229454

Committed r167368: <http://trac.webkit.org/changeset/167368>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.