Bug 175596

Created attachment 318166 [details] Implements the support

Comment on attachment 318166 [details] Implements the support View in context: https://bugs.webkit.org/attachment.cgi?id=318166&action=review > Source/WebCore/dom/DataTransferItem.cpp:-54 > - return nullptr; Spec says to also check m_dataTransfer.canReadData(), don't we need it? > Source/WebCore/dom/DataTransferItem.h:45 > +class DataTransferItem { Shouldn't this subclass ScriptWrappable given that it is web-exposed? > Source/WebCore/dom/DataTransferItem.idl:39 > + [CallWith=ScriptExecutionContext] void getAsString(optional StringCallback? callback); The parameter is not optional is the spec, merely nullable. > Source/WebCore/dom/DataTransferItemList.cpp:78 > + items.append(std::make_unique<DataTransferItem>(m_dataTransfer, type)); It is weird that DataTransferItem has ref/deref methods and is stored in an unique_ptr here. We need it to be refcounted due because it is script wrappable , so I don't think we should use unique_ptr here. > Source/WebCore/dom/DataTransferItemList.cpp:82 > + for (unsigned i = 0, length = files.length(); i < length; i++) { ++i > Source/WebCore/dom/DataTransferItemList.cpp:85 > + if (isSupportedType(type) || true) || true ? > Source/WebCore/dom/DataTransferItemList.h:44 > class DataTransferItemList { Shouldn't this subclass ScriptWrappable? > Source/WebCore/dom/DataTransferItemList.h:67 > + std::optional<Vector<std::unique_ptr<DataTransferItem>>> m_items; Personally, I'd rather we make this member mutable and keep the length getter const. The fact that this is lazy-initialized is an implementation detail.

Comment on attachment 318166 [details] Implements the support View in context: https://bugs.webkit.org/attachment.cgi?id=318166&action=review >> Source/WebCore/dom/DataTransferItem.cpp:-54 >> - return nullptr; > > Spec says to also check m_dataTransfer.canReadData(), don't we need it? Fixed, and updated editing/pasteboard/datatransfer-items-drop-plaintext-file.html. >> Source/WebCore/dom/DataTransferItem.h:45 >> +class DataTransferItem { > > Shouldn't this subclass ScriptWrappable given that it is web-exposed? Fixed. >> Source/WebCore/dom/DataTransferItem.idl:39 >> + [CallWith=ScriptExecutionContext] void getAsString(optional StringCallback? callback); > > The parameter is not optional is the spec, merely nullable. :( The reason this wasn't caught was because my IDL test was broken. Fixed this and the test. >> Source/WebCore/dom/DataTransferItemList.cpp:78 >> + items.append(std::make_unique<DataTransferItem>(m_dataTransfer, type)); > > It is weird that DataTransferItem has ref/deref methods and is stored in an unique_ptr here. We need it to be refcounted due because it is script wrappable , so I don't think we should use unique_ptr here. This is the pattern we use when we forward ref. See CanvasRenderingContext for example. CanvasRenderingContext holds onto that by unique_ptr. If we kept Ref<DataTransferItem> in DataTransferItemList, then we'd be creating a ref cycle between DataTransfer, DataTransferItemList, and DataTransferItem because DataTransferItem forwards ref to DataTransfer, which has to keep DataTransferItemList alive which in turn has to keep its DataTransferItem alive. >> Source/WebCore/dom/DataTransferItemList.cpp:82 >> + for (unsigned i = 0, length = files.length(); i < length; i++) { > > ++i Fixed. >> Source/WebCore/dom/DataTransferItemList.cpp:85 >> + if (isSupportedType(type) || true) > > || true ? Oops, this was for debugging. Removed. >> Source/WebCore/dom/DataTransferItemList.h:44 >> class DataTransferItemList { > > Shouldn't this subclass ScriptWrappable? Fixed. >> Source/WebCore/dom/DataTransferItemList.h:67 >> + std::optional<Vector<std::unique_ptr<DataTransferItem>>> m_items; > > Personally, I'd rather we make this member mutable and keep the length getter const. The fact that this is lazy-initialized is an implementation detail. Sure, changed.

Created attachment 318201 [details] Addressed review comments

Comment on attachment 318201 [details] Addressed review comments View in context: https://bugs.webkit.org/attachment.cgi?id=318201&action=review > Source/WebCore/ChangeLog:8 > + Added the basic machinary to get the list of plain text items to DataTransferItemList and DataTransferItem. Nit - s/machinary/machinery/ > Source/WebCore/dom/DataTransferItem.cpp:69 > + String value = m_dataTransfer.getData(m_type); Nit - auto > Source/WebCore/dom/DataTransferItemList.cpp:83 > + File& file = *files.item(i); Nit - I think these would be more elegant as auto& file and auto type. > LayoutTests/ChangeLog:10 > + Unfortunately, dropping a file is only supported by DumpRenderTree on Mac :( so it's disabled elsewhere. We already have existing iOS drag and drop tests that access the DataTransfer's FileList on drop (event.dataTransfer.files), so I would think we should also be able to use items there. An iOS DnD test would be nice, but probably in a followup when we start iterating on iOS.

(In reply to Wenson Hsieh from comment #5) > Comment on attachment 318201 [details] > Addressed review comments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=318201&action=review > > > Source/WebCore/ChangeLog:8 > > + Added the basic machinary to get the list of plain text items to DataTransferItemList and DataTransferItem. > > Nit - s/machinary/machinery/ Fixed. > > Source/WebCore/dom/DataTransferItem.cpp:69 > > + String value = m_dataTransfer.getData(m_type); > > Nit - auto I inlined the code into scheduleCallback. > > Source/WebCore/dom/DataTransferItemList.cpp:83 > > + File& file = *files.item(i); > > Nit - I think these would be more elegant as auto& file and auto type. No, I specifically avoided auto here to make following the code easier. > > LayoutTests/ChangeLog:10 > > + Unfortunately, dropping a file is only supported by DumpRenderTree on Mac :( so it's disabled elsewhere. > > We already have existing iOS drag and drop tests that access the > DataTransfer's FileList on drop (event.dataTransfer.files), so I would think > we should also be able to use items there. An iOS DnD test would be nice, > but probably in a followup when we start iterating on iOS. Okay, I'll look into that.

Committed r220782: <http://trac.webkit.org/changeset/220782>

<rdar://problem/33910813>