[webkit-reviews] review denied: [Bug 177853] Add basic support for the version of DataTransferItemList.add that takes a File : [Attachment 322626] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 3 22:50:23 PDT 2017


Wenson Hsieh <wenson_hsieh at apple.com> has denied 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 322626: Patch

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




--- Comment #3 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 322626
  --> https://bugs.webkit.org/attachment.cgi?id=322626
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!


More information about the webkit-reviews mailing list