Bug 177853 – Add basic support for the version of DataTransferItemList.add that takes a File

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

Implement a version of DataTransferItemList.add(File) that doesn't yet write to the pasteboard, for parity with other browsers on macOS.

[Wenson Hsieh]

Created attachment 322626 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 322626 [details]
Patch

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

> Source/WebCore/dom/DataTransfer.cpp:169
> +    // Invalidate the present FileList. An up-to-date FileList will be regenerated if necessary.
> +    m_fileList = nullptr;

We can't quite do this because dataTransfer.files need to return the same object each time.
In fact, we should add a test making sure dataTransfer.files don't change before/after adding or removing a file.
We need to add a new method to FileList and remove the relevant entry.

> Source/WebCore/dom/DataTransfer.cpp:203
> +        m_itemList->enumerateItems([&containsFile] (const auto& item) {
> +            containsFile |= item.isFile();
> +        });

If I were you, I'd just add a new method on DataTransferItemList which returns
const Vector<Ref<DataTransferItem>>& like m_itemList->items()
and call findMatching() here instead of traversing through the entire list.
Alternatively, we could just add two methods: containsFiles() and addFiles(FileList&).

> Source/WebCore/dom/DataTransferItemList.cpp:94
> +    return RefPtr<DataTransferItem> { m_items->last().copyRef() };

Why don't we just return m_items->last().ptr()?

> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-copy.html:13
> +    <div style="font-size: 40px;" id="source">Try to copy me.</div>

You should add instructions on how to run this test manually when opening in a browser.

> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-copy.html:27
> +    for (let type of event.clipboardData.types)

Use const.

> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-copy.html:29
> +    let itemsInfo = []

Use const. Missing ;.

> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-copy.html:30
> +    for (let item of event.clipboardData.items) {

Use const.

> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-copy.html:60
> +    event.clipboardData.items.remove(2);

You should check that calling getAsFile() on the removed item returns null.

> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-copy.html:64
> +    event.clipboardData.items.add(new File([ "This is just a plain string" ], "second.txt", { type: "text/plain" }));
> +    updateOutputText("after adding another plain text file", event);

You should add a test case for when files and non-file types are interleaved.

> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-drag.html:13
> +    <div style="font-size: 40px;" id="source" draggable="true">Try to drag me out.</div>

Ditto to add the instructions on how to test this manually.

[Wenson Hsieh]

Comment on attachment 322626 [details]
Patch

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

Thanks for taking a look!

Marking as r-, due to the changing FileList on DataTransfer.

>> Source/WebCore/dom/DataTransfer.cpp:169
>> +    m_fileList = nullptr;
> 
> We can't quite do this because dataTransfer.files need to return the same object each time.
> In fact, we should add a test making sure dataTransfer.files don't change before/after adding or removing a file.
> We need to add a new method to FileList and remove the relevant entry.

Oh, ok. Upon closer inspection, I now see this is marked as [SameObject] in the WHATWG spec (https://html.spec.whatwg.org/multipage/dnd.html#dom-datatransfer-files) :P

Fixed by adding support to remove a File from the FileList.

>> Source/WebCore/dom/DataTransfer.cpp:203
>> +        });
> 
> If I were you, I'd just add a new method on DataTransferItemList which returns
> const Vector<Ref<DataTransferItem>>& like m_itemList->items()
> and call findMatching() here instead of traversing through the entire list.
> Alternatively, we could just add two methods: containsFiles() and addFiles(FileList&).

Sure! I added a containsFiles() at first, but (somewhat arbitrarily) settled on this enumeration approach. But I like your approach of just returning a const Vector<Ref<DataTransferItem>>& the best!

>> Source/WebCore/dom/DataTransferItemList.cpp:94
>> +    return RefPtr<DataTransferItem> { m_items->last().copyRef() };
> 
> Why don't we just return m_items->last().ptr()?

Good point. Fixed!

>> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-copy.html:13
>> +    <div style="font-size: 40px;" id="source">Try to copy me.</div>
> 
> You should add instructions on how to run this test manually when opening in a browser.

Sure. It's a bit difficult to describe, since the test adds and removes a lot of miscellaneous items and dumps a lot of output text, but I can put in a few more words here describing one should expect.

>> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-copy.html:27
>> +    for (let type of event.clipboardData.types)
> 
> Use const.

Ok! Replaced lets with consts, where possible.

>> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-copy.html:29
>> +    let itemsInfo = []
> 
> Use const. Missing ;.

👍🏻

>> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-copy.html:30
>> +    for (let item of event.clipboardData.items) {
> 
> Use const.

👍🏻

>> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-copy.html:60
>> +    event.clipboardData.items.remove(2);
> 
> You should check that calling getAsFile() on the removed item returns null.

👍🏻

>> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-copy.html:64
>> +    updateOutputText("after adding another plain text file", event);
> 
> You should add a test case for when files and non-file types are interleaved.

👍🏻

>> LayoutTests/editing/pasteboard/data-transfer-item-list-add-file-on-drag.html:13
>> +    <div style="font-size: 40px;" id="source" draggable="true">Try to drag me out.</div>
> 
> Ditto to add the instructions on how to test this manually.

Yep, added!

[Wenson Hsieh]

Created attachment 322635 [details]
Second pass

[Wenson Hsieh]

<rdar://problem/34807346>

[Ryosuke Niwa]

Comment on attachment 322635 [details]
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?

[Wenson Hsieh]

Comment on attachment 322635 [details]
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>&& 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.

Oh, good point! With the current implementation, we're able to add duplicate items, but removeFirstMatching doesn't remove the right element. The spec doesn't explicitly disallow this case, and FF/Chrome support this, so I think we should also allow adding the same File multiple times, and make sure the correct one is removed when we remove() a File-backed item. I'll post a new patch that does this.

>> Source/WebCore/dom/DataTransferItemList.cpp:195
>> +}
> 
> Can't we just inline this?

👍🏻

[Wenson Hsieh]

Comment on attachment 322635 [details]
Second pass

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

>>> Source/WebCore/dom/DataTransferItemList.cpp:87
>>> +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.
> 
> Oh, good point! With the current implementation, we're able to add duplicate items, but removeFirstMatching doesn't remove the right element. The spec doesn't explicitly disallow this case, and FF/Chrome support this, so I think we should also allow adding the same File multiple times, and make sure the correct one is removed when we remove() a File-backed item. I'll post a new patch that does this.

This is a tiny bit annoying to get right (we'll need something to the effect of a map from indices in the DataTransferItemList => indices in the FileList) but certainly doable. On a related note: I was curious if Blink had a more elegant solution, but in their implementation of DataTransfer::files() (https://chromium.googlesource.com/chromium/blink/+/master/Source/core/clipboard/DataTransfer.cpp) they just regenerate a new FileList every time from their DataObject backing, so that'll actually fail the new DataTransfer tests here since, the FileList object is different before and after the test :P

[Wenson Hsieh]

Created attachment 322691 [details]
Support adding and removing the same File

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #9)
> Created attachment 322691 [details]
> Support adding and removing the same File

As Ryosuke pointed out in person, this approach still exhibits a bug where bindings can hold a reference to the DataTransfer's FileList itself. And then, upon accessing the FileList's files, it'll be empty, since we didn't go through DataTransfer::files.

It seems that we should just not bother to implement an invalidation mechanism, and just update the FileList then and there if items are removed.

[Ryosuke Niwa]

Comment on attachment 322691 [details]
Support adding and removing the same File

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

> Source/WebCore/dom/DataTransfer.cpp:174
> +    // We don't destroy and recreate the entire FileList here because the FileList object needs to stay the same, per the DataTransfer spec.
> +    // Instead, flag the file list for an update, so the next time it is queried, we recompute the state, respecting any updated data transfer items.
> +    m_fileList->clear();
> +    m_fileListNeedsUpdate = true;

This won't quite work because JS can hold onto a reference to FileList object.
So if the script calls dataTransfer.files first, removes a file via dataTransfer.items.remove,
and then accesses the FileList without calling dataTransfer.files,
then they're gonna see an empty file list.

[Wenson Hsieh]

Created attachment 322712 [details]
Support adding and removing the same File (2)

[Ryosuke Niwa]

Comment on attachment 322712 [details]
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.

[Wenson Hsieh]

Comment on attachment 322712 [details]
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.

Agreed :/ we could pull some of this out into public methods, but then I'm not sure that would be an improvement, since we really want just the owners of the FileList to be able to clear/remove files. Let's revisit this.

>> Source/WebCore/dom/DataTransfer.cpp:174
>> +void DataTransfer::itemListDidAddFile()
> 
> I would call this didAddToItemList instead to match the naming convention elsewhere.

Ok. I would strongly prefer "didAddFileToItemList" though, since this is only invoked in the add(File) codepath, so we should make that clear.

>> 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?

Sure! That makes sense.

>> Source/WebCore/dom/DataTransfer.cpp:230
>> +                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.

Correct. In our current codepaths where we have read/write access, we aren't "connected" yet to the platform pasteboard yet. Added an assertion here.

[Wenson Hsieh]

Created attachment 322729 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 322729 [details]
Patch for landing

Clearing flags on attachment: 322729

Committed r222885: <http://trac.webkit.org/changeset/222885>

[Daniel Bates]

Comment on attachment 322729 [details]
Patch for landing

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

I know this patch was already reviewed and landed. I noticed some issues and had some questions.

> Source/WebCore/dom/DataTransfer.cpp:167
> +    // If we're removing an item, then the item list must exist, which implies that the file list must have been initialized already.

The information in this comment is obvious, including the derivation in comment, and can be easily derived by reading the code as well as diagnosed from a crash report. Either we should improve this comment to say something non-obvious or we should remove this comment.

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

Is this much benefit to having this assert? I mean, it seems like it would be straightforward to diagnose is this invariant was violated from a crash report showing a null dereference inside this function.

> Source/WebCore/dom/DataTransfer.cpp:206
> +        return { "Files" };

I know that you are following the style of the code in this function. We should be using ASCIILiteral().

> Source/WebCore/dom/DataTransfer.cpp:232
> +            if (auto file = item->file()) {

No braces for an if-statement whose body is a single line per the WebKit Code Style guidelines. I'm surprised the style checker did not notice this.

> Source/WebCore/dom/DataTransfer.cpp:233
> +                files.append(*file);

Is this the preferred way to make a copy of a Ref?

> Source/WebCore/dom/DataTransferItem.h:57
> +    RefPtr<File> file() const { return m_file; }

Although this function does not actually modify m_file (and hence is const with respect to mutation of its own intense variables) the caller can mutate m_file since we effectively return a non-const pointer to it. We tend to have member functions be in one of two groups: either be const and return const data or be non-cont and return non-const data. Can we pick one of these forms?

> Source/WebCore/dom/DataTransferItemList.cpp:94
> +    return RefPtr<DataTransferItem> { m_items->last().ptr() };

Is it necessary to explicitly call the RefPtr<DataTransferItem> constructor?

> Source/WebCore/dom/DataTransferItemList.cpp:106
> +    // Since file-backed DataTransferItems are not actually written to the pasteboard yet, we don't need to remove any

Should this be a FIXME? The last sentence states that we need to take action to update this code once we support file-backed DataTransferItems.

> Source/WebCore/dom/DataTransferItemList.cpp:114
> +    if (removedItem.isFile())

This does not look correct. Notice that removedItem is a temporary. Suppose items[index] has one ref. Then when we remove it from |items| on line 113 it will be derefed => deallocated. removedItem is now a null reference.

> Source/WebCore/dom/DataTransferItemList.h:68
> +    bool hasItems() const { return m_items.has_value(); }

I know that the data type for m_items was chosen outside of this patch. I take it that it is necessary to differentiate between an empty list and no list? If so, is there a significant performance benefit (or some other benefit) to declaring m_items to be of type std::optional<Vector<Ref<DataTransferItem>>> compared to type Vector<Ref<DataTransferItem>>*?

[Ryosuke Niwa]

Comment on attachment 322729 [details]
Patch for landing

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

>> Source/WebCore/dom/DataTransferItemList.cpp:114
>> +    if (removedItem.isFile())
> 
> This does not look correct. Notice that removedItem is a temporary. Suppose items[index] has one ref. Then when we remove it from |items| on line 113 it will be derefed => deallocated. removedItem is now a null reference.

Oops, yeah, this is indeed an issue. I don't understand why line 109 needs to call get.
We can just create a Ref there. In fact, we probably shouldn't use auto in that line to make this semantics clear.

[Wenson Hsieh]

Comment on attachment 322729 [details]
Patch for landing

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

Thanks for taking a look, Dan! I'll post a followup patch momentarily.

>> Source/WebCore/dom/DataTransfer.cpp:167
>> +    // If we're removing an item, then the item list must exist, which implies that the file list must have been initialized already.
> 
> The information in this comment is obvious, including the derivation in comment, and can be easily derived by reading the code as well as diagnosed from a crash report. Either we should improve this comment to say something non-obvious or we should remove this comment.

I added this comment in response to one of Ryosuke's earlier comments, wherein he felt that it was subtle that m_fileList should be nonnull here, and that it deserved a comment describing why this is the case...

Given your below comment though, I'm inclined to just remove this assert altogether.

>> Source/WebCore/dom/DataTransfer.cpp:168
>> +    ASSERT(m_fileList);
> 
> Is this much benefit to having this assert? I mean, it seems like it would be straightforward to diagnose is this invariant was violated from a crash report showing a null dereference inside this function.

Yeah, I think that's a fair assessment. Removed this assertion.

>> Source/WebCore/dom/DataTransfer.cpp:206
>> +        return { "Files" };
> 
> I know that you are following the style of the code in this function. We should be using ASCIILiteral().

Sure. I updated the return below as well.

>> Source/WebCore/dom/DataTransfer.cpp:232
>> +            if (auto file = item->file()) {
> 
> No braces for an if-statement whose body is a single line per the WebKit Code Style guidelines. I'm surprised the style checker did not notice this.

Whoa, good catch -- I'm also surprised that the style checker didn't flag this! Fixed.

>> Source/WebCore/dom/DataTransfer.cpp:233
>> +                files.append(*file);
> 
> Is this the preferred way to make a copy of a Ref?

file here is a RefPtr, so this code is constructing a Ref<File> out of the File& that results from using the dereference operator on file. I thought this was an OK way to construct a new Ref<File>, but I'm certainly open to suggestions!

>> Source/WebCore/dom/DataTransferItem.h:57
>> +    RefPtr<File> file() const { return m_file; }
> 
> Although this function does not actually modify m_file (and hence is const with respect to mutation of its own intense variables) the caller can mutate m_file since we effectively return a non-const pointer to it. We tend to have member functions be in one of two groups: either be const and return const data or be non-cont and return non-const data. Can we pick one of these forms?

Sure -- changed the function to be non-const. In the two places where we use this, we use it to populate FileList with Files. These Files are non-const, so I can't make this a RefPtr<const File>.

>> Source/WebCore/dom/DataTransferItemList.cpp:94
>> +    return RefPtr<DataTransferItem> { m_items->last().ptr() };
> 
> Is it necessary to explicitly call the RefPtr<DataTransferItem> constructor?

No it's not -- removed.

>> Source/WebCore/dom/DataTransferItemList.cpp:106
>> +    // Since file-backed DataTransferItems are not actually written to the pasteboard yet, we don't need to remove any
> 
> Should this be a FIXME? The last sentence states that we need to take action to update this code once we support file-backed DataTransferItems.

Sure, this could use a FIXME. Added.

>> Source/WebCore/dom/DataTransferItemList.cpp:114
>> +    if (removedItem.isFile())
> 
> This does not look correct. Notice that removedItem is a temporary. Suppose items[index] has one ref. Then when we remove it from |items| on line 113 it will be derefed => deallocated. removedItem is now a null reference.

Good catch. Fixed by not accessing removedItem after calling items.remove().

>> Source/WebCore/dom/DataTransferItemList.h:68
>> +    bool hasItems() const { return m_items.has_value(); }
> 
> I know that the data type for m_items was chosen outside of this patch. I take it that it is necessary to differentiate between an empty list and no list? If so, is there a significant performance benefit (or some other benefit) to declaring m_items to be of type std::optional<Vector<Ref<DataTransferItem>>> compared to type Vector<Ref<DataTransferItem>>*?

It is important to distinguish between an empty list (i.e. some state was requested that requires the item list to exist; there's just no items on it) vs. null list (we haven't lazily generated the item list yet, since no state was requested that requires it to have been created). I suppose there are alternatives, like a bool flag indicating whether or not the item list has been computed, but using std::optional seems to be a reasonable approach.

[Daniel Bates]

(In reply to Daniel Bates from comment #17)
> > Source/WebCore/dom/DataTransferItemList.h:68
> > +    bool hasItems() const { return m_items.has_value(); }
> 
> I know that the data type for m_items was chosen outside of this patch. I
> take it that it is necessary to differentiate between an empty list and no
> list? If so, is there a significant performance benefit (or some other
> benefit) to declaring m_items to be of type
> std::optional<Vector<Ref<DataTransferItem>>> compared to type
> Vector<Ref<DataTransferItem>>*?

Err, std::unique_ptr<Vector<Ref<DataTransferItem>>>?

[Wenson Hsieh]

Created attachment 322778 [details]
Post-review feedback

[Ryosuke Niwa]

Comment on attachment 322778 [details]
Post-review feedback

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

> Source/WebCore/dom/DataTransferItemList.cpp:108
> +    // FIXME: Since file-backed DataTransferItems are not actually written to the pasteboard yet, we don't need to
> +    // remove any temporary files. When we support writing file-backed DataTransferItems to the platform pasteboard,
> +    // we will need to clean up here.

Instead of three sentence line FIXME, why don't we just say:
FIXME: Remove the file from the pasteboard object once we added the support for it.

[Daniel Bates]

Comment on attachment 322778 [details]
Post-review feedback

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

> Source/WebCore/dom/DataTransferItemList.cpp:109
> +    Ref<DataTransferItem> removedItem = items[index].copyRef();

Is the copyRef() needed?

[Daniel Bates]

Comment on attachment 322778 [details]
Post-review feedback

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

> Source/WebCore/dom/DataTransfer.cpp:231
>                  files.append(*file);

I suggest that we RefPtr::releaseNonNull() here to avoid a ref.

[Wenson Hsieh]

Comment on attachment 322778 [details]
Post-review feedback

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

>> Source/WebCore/dom/DataTransfer.cpp:231
>>                  files.append(*file);
> 
> I suggest that we RefPtr::releaseNonNull() here to avoid a ref.

Sure! Done.

>> Source/WebCore/dom/DataTransferItemList.cpp:108
>> +    // we will need to clean up here.
> 
> Instead of three sentence line FIXME, why don't we just say:
> FIXME: Remove the file from the pasteboard object once we added the support for it.

👍🏻 done.

>> Source/WebCore/dom/DataTransferItemList.cpp:109
>> +    Ref<DataTransferItem> removedItem = items[index].copyRef();
> 
> Is the copyRef() needed?

Well, items[index].get() would also work, since that would implicitly construct a new Ref<DataTransferItem> using a DataTransferItem&. I can't simply set Ref<DataTransferItem> removedItem = items[index] though, since that would call the deleted constructor on Ref (if this is what you meant?)

[Daniel Bates]

(In reply to Wenson Hsieh from comment #25)
> >> Source/WebCore/dom/DataTransferItemList.cpp:109
> >> +    Ref<DataTransferItem> removedItem = items[index].copyRef();
> > 
> > Is the copyRef() needed?
> 
> Well, items[index].get() would also work, since that would implicitly
> construct a new Ref<DataTransferItem> using a DataTransferItem&. I can't
> simply set Ref<DataTransferItem> removedItem = items[index] though, since
> that would call the deleted constructor on Ref (if this is what you meant?)

OK.

[Wenson Hsieh]

Created attachment 322797 [details]
Hold for EWS.

[WebKit Commit Bot]

Comment on attachment 322797 [details]
Hold for EWS.

Clearing flags on attachment: 322797

Committed r222904: <http://trac.webkit.org/changeset/222904>

[Matt Lewis]

This looks to have cause a test that was marked as failing on iOS consistently to start timing out consistently on iOS. 

Test: editing/pasteboard/data-transfer-items.html 

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=editing%2Fpasteboard%2Fdata-transfer-items.html

https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r222911%20(343)/results.html
https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/builds/343


diff:

--- /Volumes/Data/slave/ios-simulator-11-release-tests-wk2/build/layout-test-results/editing/pasteboard/data-transfer-items-expected.txt
+++ /Volumes/Data/slave/ios-simulator-11-release-tests-wk2/build/layout-test-results/editing/pasteboard/data-transfer-items-actual.txt
@@ -1,25 +1,6 @@
-This file tests the basic functionality and properties of DataTransferItems. This test requires DRT.
-Populating DataTransferItems...
-Caught exception "NotSupportedError (DOM Exception 9): The operation is not supported." as expected.
-Verifying contents of DataTransferItems...
-items.length: 2
-items[0].kind: string
-items[0].type: text/plain
-items[1].kind: string
-items[1].type: text/html
-Checking if items past the end of the collection can be indexed:
-items[2] is undefined: undefined
-Checking that a read-only DataTransferItems cannot be mutated...
-items.length: 2
-items[0].kind: string
-items[0].type: text/plain
-items[1].kind: string
-items[1].type: text/html
-items[2] is undefined: undefined
-Testing if DataTransferItems can be accessed outside an event handler...
-DataTransferItem accessed outside event handler!
-copy: items[0] value: Hello World!
-copy: items[1] value: <b>Hello World!
-paste: items[0] value: This file tests the basic functionality and properties of DataTransferItems. This test requires DRT.
-paste: items[1] value: <span style="color: rgb(0, 0, 0); font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; display: inline !important; float: none;">This file tests the basic functionality and properties of DataTransferItems. This test requires DRT.</span>
+CONSOLE MESSAGE: line 79: TypeError: undefined is not an object (evaluating 'items[0].kind')
+#PID UNRESPONSIVE - com.apple.WebKit.WebContent.Development (pid 7381)
+FAIL: Timed out waiting for notifyDone to be called
 
+#EOF
+#EOF

[Matt Lewis]

Talked with Wenson and we did some testing. Looks like the test has started to timeout even when rolling out the change and testing with much older revisions despite what the dashboard says.