[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