[webkit-reviews] review granted: [Bug 131694] DataTransfer should cache its FileList : [Attachment 229416] proposed fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 131694: DataTransfer should cache its FileList
https://bugs.webkit.org/show_bug.cgi?id=131694

Attachment 229416: proposed fix
https://bugs.webkit.org/attachment.cgi?id=229416&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.