[webkit-reviews] review granted: [Bug 177853] Add basic support for the version of DataTransferItemList.add that takes a File : [Attachment 322635] Second pass

[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 322635: Second pass

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




--- Comment #6 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 322635
  --> https://bugs.webkit.org/attachment.cgi?id=322635
Second pass

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

> Source/WebCore/dom/DataTransfer.cpp:167
> +    ASSERT(m_fileList);

It's a bit subtle that m_fileList is guaranteed to exist because
DataTransferItemList::ensureItems() calls DataTransfer::files().
We should probably add a comment about it.

> Source/WebCore/dom/DataTransferItemList.cpp:87
> -RefPtr<DataTransferItem> DataTransferItemList::add(Ref<File>&&)
> +RefPtr<DataTransferItem> DataTransferItemList::add(Ref<File>&& file)

What happens when you add the same File object twice?
There is no guarantee that they're unique... HTML spec doesn't say anything
about this case but we should check the behavior.
If we're allowed to have duplicate entires (that's a bit crazy), then we need
to make FileList::removeFile remove the right element.
If not, this function needs to de-duplicate it.

> Source/WebCore/dom/DataTransferItemList.cpp:195
> +const Vector<Ref<DataTransferItem>>& DataTransferItemList::items() const
> +{
> +    ASSERT(m_items);
> +    return *m_items;
> +}

Can't we just inline this?