Bug 76367 - Add DataTransferItems support for drag-and-drop'ed files and texts
Summary: Add DataTransferItems support for drag-and-drop'ed files and texts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kinuko Yasuda
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-15 23:14 PST by Kinuko Yasuda
Modified: 2012-01-23 13:02 PST (History)
10 users (show)

See Also:


Attachments
Patch (48.51 KB, patch)
2012-01-16 00:55 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (48.39 KB, patch)
2012-01-16 03:27 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (53.56 KB, patch)
2012-01-16 05:48 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (56.77 KB, patch)
2012-01-17 07:51 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (67.54 KB, patch)
2012-01-18 09:08 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (69.08 KB, patch)
2012-01-19 00:37 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch (69.36 KB, patch)
2012-01-19 03:23 PST, Kinuko Yasuda
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2012-01-15 23:14:24 PST
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().
Comment 1 Kinuko Yasuda 2012-01-16 00:55:43 PST
Created attachment 122601 [details]
Patch
Comment 2 Early Warning System Bot 2012-01-16 01:09:42 PST
Comment on attachment 122601 [details]
Patch

Attachment 122601 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11255087
Comment 3 Gyuyoung Kim 2012-01-16 01:15:20 PST
Comment on attachment 122601 [details]
Patch

Attachment 122601 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11214095
Comment 4 Kinuko Yasuda 2012-01-16 03:27:39 PST
Created attachment 122610 [details]
Patch
Comment 5 Early Warning System Bot 2012-01-16 03:56:35 PST
Comment on attachment 122610 [details]
Patch

Attachment 122610 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11135131
Comment 6 Gyuyoung Kim 2012-01-16 04:05:01 PST
Comment on attachment 122610 [details]
Patch

Attachment 122610 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11256055
Comment 7 Kinuko Yasuda 2012-01-16 05:48:56 PST
Created attachment 122621 [details]
Patch
Comment 8 Adam Barth 2012-01-17 01:12:21 PST
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)?
Comment 9 Kinuko Yasuda 2012-01-17 07:51:45 PST
Created attachment 122768 [details]
Patch
Comment 10 Kinuko Yasuda 2012-01-17 07:54:52 PST
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:'.
Comment 11 Daniel Cheng 2012-01-17 10:28:58 PST
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.
}
Comment 12 Kinuko Yasuda 2012-01-18 09:08:47 PST
Created attachment 122940 [details]
Patch
Comment 13 Kinuko Yasuda 2012-01-18 09:19:38 PST
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).
Comment 14 Daniel Cheng 2012-01-18 09:45:28 PST
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.
Comment 15 Kinuko Yasuda 2012-01-18 10:23:01 PST
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.
Comment 16 Daniel Cheng 2012-01-18 13:36:27 PST
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).
Comment 17 Kinuko Yasuda 2012-01-19 00:37:12 PST
Created attachment 123086 [details]
Patch
Comment 18 Kinuko Yasuda 2012-01-19 01:09:21 PST
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.
Comment 19 WebKit Review Bot 2012-01-19 01:34:50 PST
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
Comment 20 Kinuko Yasuda 2012-01-19 03:23:04 PST
Created attachment 123095 [details]
Patch

Fixed the test (and updated the test expectation with the new error code).
Comment 21 Tony Chang 2012-01-19 11:27:32 PST
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()
Comment 22 Kinuko Yasuda 2012-01-19 21:28:17 PST
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.
Comment 23 Kinuko Yasuda 2012-01-20 04:09:06 PST
Committed r105506: <http://trac.webkit.org/changeset/105506>
Comment 24 Daniel Cheng 2012-01-23 12:46:12 PST
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'.
Comment 25 Daniel Cheng 2012-01-23 13:02:17 PST
(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.