Bug 175596 - Make DataTransferItemList work with plain text entries
Summary: Make DataTransferItemList work with plain text entries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 170449
  Show dependency treegraph
 
Reported: 2017-08-15 13:39 PDT by Ryosuke Niwa
Modified: 2017-08-15 19:24 PDT (History)
12 users (show)

See Also:


Attachments
Implements the support (37.26 KB, patch)
2017-08-15 13:48 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Addressed review comments (39.44 KB, patch)
2017-08-15 17:10 PDT, Ryosuke Niwa
wenson_hsieh: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2017-08-15 13:39:10 PDT
Add the support for reading off plain text from dataTransfer.items for drag & drop,
and clipboardData.items for copy & paste.
Comment 1 Ryosuke Niwa 2017-08-15 13:48:52 PDT
Created attachment 318166 [details]
Implements the support
Comment 2 Chris Dumez 2017-08-15 15:19:51 PDT
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 3 Ryosuke Niwa 2017-08-15 17:08:54 PDT
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.
Comment 4 Ryosuke Niwa 2017-08-15 17:10:23 PDT
Created attachment 318201 [details]
Addressed review comments
Comment 5 Wenson Hsieh 2017-08-15 19:06:54 PDT
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.
Comment 6 Ryosuke Niwa 2017-08-15 19:22:35 PDT
(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.
Comment 7 Ryosuke Niwa 2017-08-15 19:23:43 PDT
Committed r220782: <http://trac.webkit.org/changeset/220782>
Comment 8 Radar WebKit Bug Importer 2017-08-15 19:24:46 PDT
<rdar://problem/33910813>