Bug 76367 – Add DataTransferItems support for drag-and-drop'ed files and texts

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

Implement DataTransferItems support for drag-and-drop'ed files and texts.

Per http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#the-datatransfer-interface the new interface should also support drag-and-dropped files and texts in addition to pasted texts/images.  The user apps should also be able to add texts/files to the drag store by calling event.dataTransfer.items.add().

[Kinuko Yasuda]

Created attachment 122601 [details]
Patch

[Early Warning System Bot]

Comment on attachment 122601 [details]
Patch

Attachment 122601 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11255087

[Gyuyoung Kim]

Comment on attachment 122601 [details]
Patch

Attachment 122601 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11214095

[Kinuko Yasuda]

Created attachment 122610 [details]
Patch

[Early Warning System Bot]

Comment on attachment 122610 [details]
Patch

Attachment 122610 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11135131

[Gyuyoung Kim]

Comment on attachment 122610 [details]
Patch

Attachment 122610 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11256055

[Kinuko Yasuda]

Created attachment 122621 [details]
Patch

[Adam Barth]

Comment on attachment 122621 [details]
Patch

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

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:379
> +    downloadUrl.append(file->name());

Do we need to do any escaping here to make sure file->name() doen't have a ":" (for example)?

[Kinuko Yasuda]

Created attachment 122768 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 122621 [details]
Patch

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

>> Source/WebCore/platform/chromium/ClipboardChromium.cpp:379
>> +    downloadUrl.append(file->name());
> 
> Do we need to do any escaping here to make sure file->name() doen't have a ":" (for example)?

Good point, updated the patch to escape the file name and added a new test resource file whose name starts with 'test:'.

[Daniel Cheng]

Comment on attachment 122768 [details]
Patch

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

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:53
> +    return V8Blob::wrap(impl);

This makes me wonder if we should really just return File for images too with an empty filename or something.

> Source/WebCore/dom/Clipboard.h:102
> +        virtual void addDragFile(PassRefPtr<File>) = 0;

It may be better to add this as a non-virtual member of ClipboardChromium and ClipboardQt. This is an implementation detail of the Chromium and Qt ports since we both don't conform to the HTML5 DataTransfer spec. The spec actually defines the legacy clipboard operations, like accessing .files or calling setData/getData, in terms of DataTransferItems. So internally, what we'll eventually do is call items() and then implement getData()/setData() by using the steps described in whatwg.org/html5 on the returned items -- which would make addDragFile() unnecessary.

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:355
> +    String plainText = m_dataObject->getData(mimeTypeTextPlain, success);

It's probably fine to just file a bug about this and fix it in a separate patch, but I think we're going to have to save the items pointer. If I call items() multiple times, won't I get a pointer to a different collection?

For example...
function ondragstart(event)
{
    event.dataTransfer.items.add('Hello world!', 'text/plain');
    var items = event.dataTransfer.items;
    event.dataTransfer.items.add('<b>Markup</b>', 'text/html');
    var items2 = event.dataTransfer.items;
    // items and items2 will contain different items at this point.
}

[Kinuko Yasuda]

Created attachment 122940 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 122768 [details]
Patch

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

>> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:53
>> +    return V8Blob::wrap(impl);
> 
> This makes me wonder if we should really just return File for images too with an empty filename or something.

I think it's questionable, since Blob and File are handled differently in many places and to return a File object we'll need to change the code elsewhere or to actually create a (temporary) file for pasted images.  (But if this custom code looks undesirable maybe we can at least add a FIXME comment for now?)

>> Source/WebCore/dom/Clipboard.h:102
>> +        virtual void addDragFile(PassRefPtr<File>) = 0;
> 
> It may be better to add this as a non-virtual member of ClipboardChromium and ClipboardQt. This is an implementation detail of the Chromium and Qt ports since we both don't conform to the HTML5 DataTransfer spec. The spec actually defines the legacy clipboard operations, like accessing .files or calling setData/getData, in terms of DataTransferItems. So internally, what we'll eventually do is call items() and then implement getData()/setData() by using the steps described in whatwg.org/html5 on the returned items -- which would make addDragFile() unnecessary.

Makes sense, updated the code to make this non-virtual.

>> Source/WebCore/platform/chromium/ClipboardChromium.cpp:355
>> +    String plainText = m_dataObject->getData(mimeTypeTextPlain, success);
> 
> It's probably fine to just file a bug about this and fix it in a separate patch, but I think we're going to have to save the items pointer. If I call items() multiple times, won't I get a pointer to a different collection?
> 
> For example...
> function ondragstart(event)
> {
>     event.dataTransfer.items.add('Hello world!', 'text/plain');
>     var items = event.dataTransfer.items;
>     event.dataTransfer.items.add('<b>Markup</b>', 'text/html');
>     var items2 = event.dataTransfer.items;
>     // items and items2 will contain different items at this point.
> }

I see..  It looks this issue is more subtle than I thought initially.  I revised the patch and made the clipboard hold a pointer to the items.  I also added a boolean flag in the clipboard (m_dragStorageUpdated) to indicate if the buffered data has been updated or not.  Probably we could do more refactoring to make them consolidated in a better way but I didn't want to change the code too much (your feedback is welcome).

[Daniel Cheng]

Comment on attachment 122940 [details]
Patch

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

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:347
> +    if (!m_dataObject || (policy() != ClipboardReadable && policy() != ClipboardWritable))

I don't think we're supposed to return null. We should just return an empty .items collection if the clipboard isn't available for some reason.

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:356
> +void ClipboardChromium::mayUpdateItems(Vector<RefPtr<DataTransferItem> >& items)

I haven't thought this through fully, but I think that the cleanest approach here is to merge DataTransferItemList and ChromiumDataObject together. I think a lot of the complexity here comes from converting from a struct with named fields with values to a list of name-value pairs (essentially). If CDO itself is a DataTransferItemList, then all the operations here become much simpler because we don't have to manage a 'is dirty, must update items' bit.

Unfortunately, there are a lot of concepts that don't translate cleanly (url title, file content+file content file name, and HTML base URL) but I think it's probably better to wrap that in the CDO either way.

> Source/WebCore/platform/chromium/DataTransferItemListChromium.cpp:151
> +    dataObject->addFilename(file->path());

ChromiumDataObject::addFilename() is unnecessary. We don't even look at the filenames when we dispatch DragHostMsg_StartDragging.

[Kinuko Yasuda]

Comment on attachment 122940 [details]
Patch

Thanks, 

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

> Source/WebCore/dom/DataTransferItemList.cpp:48
> +size_t DataTransferItemList::length()

Note: I needed to remove this const because in the chromium subclass this updates the m_items field value.

> Source/WebCore/dom/DataTransferItemList.cpp:94
> +            ec = NOT_SUPPORTED_ERR;

Note: I changed this error value per this page:
http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#dom-datatransferitemlist-add
(Please let me know if I'm referring a wrong or obsolete draft)

>> Source/WebCore/platform/chromium/ClipboardChromium.cpp:347
>> +    if (!m_dataObject || (policy() != ClipboardReadable && policy() != ClipboardWritable))
> 
> I don't think we're supposed to return null. We should just return an empty .items collection if the clipboard isn't available for some reason.

Sounds right; I'll fix it.

>> Source/WebCore/platform/chromium/ClipboardChromium.cpp:356
>> +void ClipboardChromium::mayUpdateItems(Vector<RefPtr<DataTransferItem> >& items)
> 
> I haven't thought this through fully, but I think that the cleanest approach here is to merge DataTransferItemList and ChromiumDataObject together. I think a lot of the complexity here comes from converting from a struct with named fields with values to a list of name-value pairs (essentially). If CDO itself is a DataTransferItemList, then all the operations here become much simpler because we don't have to manage a 'is dirty, must update items' bit.
> 
> Unfortunately, there are a lot of concepts that don't translate cleanly (url title, file content+file content file name, and HTML base URL) but I think it's probably better to wrap that in the CDO either way.

I totally agree with you and I also thought that'd be our goal, but let me tell you the two issues which motivated me to hedge the way in this version:

1. CDO seems to be designed as a simple structure which has mirroring struct (i.e. WebDragData) in WebKit API layer to be exported outside WebKit, and I wanted to make it as simple as is unless I see a strong motivation.
2. dataTransfer.items needs to behave like an array (i.e. apps would expect calling items.length and items[i] would be very lightweight) so I thought we'd probably want to construct and cache a Vector<Item> like field internally in CDO.  And if we do so the resulting code would look similar to what the current patch looks.  So I wasn't strongly motivated to refactor all the CDO and ItemList structures together.

I'll give it another thought to see if I could do something better (tomorrow in our time) but wdyt about these issues?

>> Source/WebCore/platform/chromium/DataTransferItemListChromium.cpp:151
>> +    dataObject->addFilename(file->path());
> 
> ChromiumDataObject::addFilename() is unnecessary. We don't even look at the filenames when we dispatch DragHostMsg_StartDragging.

This is (was) necessary because the current code tries to recreate items() from dataObject's filenames when any of other field is updated.

[Daniel Cheng]

Comment on attachment 122940 [details]
Patch

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

>> Source/WebCore/dom/DataTransferItemList.cpp:94
>> +            ec = NOT_SUPPORTED_ERR;
> 
> Note: I changed this error value per this page:
> http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#dom-datatransferitemlist-add
> (Please let me know if I'm referring a wrong or obsolete draft)

Nope, this is correct. Thanks for fixing this.

>>> Source/WebCore/platform/chromium/ClipboardChromium.cpp:356
>>> +void ClipboardChromium::mayUpdateItems(Vector<RefPtr<DataTransferItem> >& items)
>> 
>> I haven't thought this through fully, but I think that the cleanest approach here is to merge DataTransferItemList and ChromiumDataObject together. I think a lot of the complexity here comes from converting from a struct with named fields with values to a list of name-value pairs (essentially). If CDO itself is a DataTransferItemList, then all the operations here become much simpler because we don't have to manage a 'is dirty, must update items' bit.
>> 
>> Unfortunately, there are a lot of concepts that don't translate cleanly (url title, file content+file content file name, and HTML base URL) but I think it's probably better to wrap that in the CDO either way.
> 
> I totally agree with you and I also thought that'd be our goal, but let me tell you the two issues which motivated me to hedge the way in this version:
> 
> 1. CDO seems to be designed as a simple structure which has mirroring struct (i.e. WebDragData) in WebKit API layer to be exported outside WebKit, and I wanted to make it as simple as is unless I see a strong motivation.
> 2. dataTransfer.items needs to behave like an array (i.e. apps would expect calling items.length and items[i] would be very lightweight) so I thought we'd probably want to construct and cache a Vector<Item> like field internally in CDO.  And if we do so the resulting code would look similar to what the current patch looks.  So I wasn't strongly motivated to refactor all the CDO and ItemList structures together.
> 
> I'll give it another thought to see if I could do something better (tomorrow in our time) but wdyt about these issues?

For #1, I think if we hide the complexity mostly inside CDO it won't be visible in WebDragData.
For #2, I have some ideas. Let me put together a simple example patch that illustrates some ideas I have. I think this will help us avoid diverging behavior between Chrome and DumpRenderTree (since you mentioned you need to call addFilename() for the layout test to work, while that call has no actual affect in the actual browser).

[Kinuko Yasuda]

Created attachment 123086 [details]
Patch

[Kinuko Yasuda]

Per offline discussion with dcheng@ I updated the patch with minor fixes but left the bigger refactoring portion (i.e. integrating CDO and DataTransferItemList) untouched for now.  Also I added tony@ to the reviewers list per his suggestion.  Tony, could you take a look at this one?  Thanks.

[WebKit Review Bot]

Comment on attachment 123086 [details]
Patch

Attachment 123086 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11298039

New failing tests:
editing/pasteboard/data-transfer-items.html

[Kinuko Yasuda]

Created attachment 123095 [details]
Patch

Fixed the test (and updated the test expectation with the new error code).

[Tony Chang]

Comment on attachment 123095 [details]
Patch

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

> Source/WebCore/bindings/v8/custom/V8BlobCustom.cpp:41
> +#include "V8Attr.h"
> +#include "V8Binding.h"
> +#include "V8BindingState.h"
> +#include "V8Blob.h"
> +#include "V8DirectoryEntry.h"
> +#include "V8File.h"
> +#include "V8Proxy.h"
> +#include <wtf/RefPtr.h>

Do we need to include all of these files?

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:360
> +    if (items.size() > 0U && !isStorageUpdated())

Nit: !items.isEmpty()

> Source/WebCore/platform/chromium/ClipboardChromium.cpp:393
> +bool ClipboardChromium::isStorageUpdated() const

Nit: Maybe name this storageHasUpdated().

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:80
> +    : DataTransferItem(owner, DataTransferItem::kindFile, file.get() ? file->type() : "")

Nit: "" -> String()

> Source/WebCore/platform/chromium/DataTransferItemListChromium.cpp:91
> +    RefPtr<ChromiumDataObject> dataObject = static_cast<ClipboardChromium*>(m_owner.get())->dataObject();

Nit: Maybe add a helper method for "static_cast<ClipboardChromium*>(m_owner.get())"?

> Source/WebCore/platform/chromium/DataTransferItemListChromium.h:52
> +    // Overrides.

Nit: I would name the idl file these are from.

> Source/WebCore/platform/qt/DataTransferItemQt.cpp:49
> +    return adoptRef(new DataTransferItemQt(owner, context, DataTransferItemQta::InternalSource, kindString, type, data));

DataTransferItemQta? Is the 'a' intentional?  Actually, can you just use InternalSource?

> Source/WebCore/platform/qt/DataTransferItemQt.cpp:56
> +    return adoptRef(new DataTransferItemQt(owner, context, DataTransferItemQta::InternalSource, file));

Ditto.

> Source/WebCore/platform/qt/DataTransferItemQt.cpp:85
> +    : DataTransferItem(owner, DataTransferItem::kindFile, file.get() ? file->type() : "")

Nit: "" -> String()

[Kinuko Yasuda]

Thanks!  Will fix them all before submitting.

(In reply to comment #21)
> (From update of attachment 123095 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123095&action=review

> > Source/WebCore/platform/qt/DataTransferItemQt.cpp:49
> > +    return adoptRef(new DataTransferItemQt(owner, context, DataTransferItemQta::InternalSource, kindString, type, data));
> 
> DataTransferItemQta? Is the 'a' intentional?  Actually, can you just use InternalSource?

Looks like a silly typo... thanks for catching them.

[Kinuko Yasuda]

Committed r105506: <http://trac.webkit.org/changeset/105506>

[Daniel Cheng]

I wonder if this change broke image copy and paste. We pass the type, but we ignore it, which means it will get set to an empty type string when creating a DataTransferItem for 'image/png'.

[Daniel Cheng]

(In reply to comment #24)
> I wonder if this change broke image copy and paste. We pass the type, but we ignore it, which means it will get set to an empty type string when creating a DataTransferItem for 'image/png'.

Oh never mind, it's still calling the right constructor. This code is confusing.