[webkit-reviews] review granted: [Bug 177853] Add basic support for the version of DataTransferItemList.add that takes a File : [Attachment 322712] Support adding and removing the same File (2)

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 177853: Add basic support for the version of DataTransferItemList.add that
takes a File
https://bugs.webkit.org/show_bug.cgi?id=177853

Attachment 322712: Support adding and removing the same File (2)

https://bugs.webkit.org/attachment.cgi?id=322712&action=review




--- Comment #13 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 322712
  --> https://bugs.webkit.org/attachment.cgi?id=322712
Support adding and removing the same File (2)

View in context: https://bugs.webkit.org/attachment.cgi?id=322712&action=review

> Source/WebCore/dom/DataTransfer.cpp:171
> +    m_fileList->m_files = updatedFilesForFileList();

It's a bit crazy that DataTransfer is a friend of FileList but okay.

> Source/WebCore/dom/DataTransfer.cpp:174
> +void DataTransfer::itemListDidAddFile()

I would call this didAddToItemList instead to match the naming convention
elsewhere.

> Source/WebCore/dom/DataTransfer.cpp:218
> +Vector<Ref<File>> DataTransfer::updatedFilesForFileList() const

I don't think it makes sense to call this *updated* in that we're constructing
a new vector of File objects.
Maybe filesFromPasteboardAndItemList?

> Source/WebCore/dom/DataTransfer.cpp:230
> +	   files = WTFMove(reader.files);
> +    }
> +
> +    if (m_itemList && m_itemList->hasItems()) {
> +	   for (auto& item : m_itemList->items()) {
> +	       if (auto file = item->file())
> +		   files.append(*file);

Hm... how does this work when pasteboard & items both have items?
I guess that can't currently happen because we only call this method when
deleting an item
but that's not allowed when we're reading off a real pasteboard.
I think we need an assertion here that either we've read from pasteboard or
item list is empty.