Bug 55115 – Add support for DataTransferItems

• Component: DOM
• Status: RESOLVED FIXED

[Daniel Cheng]

Per http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#the-datatransferitems-interface, DataTransferItems and DataTransferItem are new interfaces intended to support async data retrieval for drops/pastes.

[Daniel Cheng]

Created attachment 83615 [details]
Patch

[Daniel Cheng]

Comment on attachment 83615 [details]
Patch

Forgot to include a change.

[Daniel Cheng]

Created attachment 83619 [details]
Patch

[Daniel Cheng]

Created attachment 83624 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 83624 [details]
Patch

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

Seems a bit silly to add non-compiled code.  Especially since you didn't add any .cpp files even.

> Source/WebCore/dom/DataTransferItem.h:41
> +class DataTransferItem : public RefCounted<DataTransferItem> {

Shouldn't this header be conditional?

> Source/WebCore/dom/DataTransferItems.h:43
> +class DataTransferItems : public RefCounted<DataTransferItems> {

Wrapped in #ifs?

[Build Bot]

Attachment 83624 [details] did not build on win:
Build output: http://queues.webkit.org/results/7984415

[Build Bot]

Attachment 83619 [details] did not build on win:
Build output: http://queues.webkit.org/results/7983471

[Daniel Cheng]

Created attachment 84122 [details]
Patch

[Daniel Cheng]

(In reply to comment #5)
> (From update of attachment 83624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83624&action=review
> 
> Seems a bit silly to add non-compiled code.  Especially since you didn't add any .cpp files even.

I wasn't sure how to split up the change. For the most part, I think each implementation would be highly platform-specific.

> 
> > Source/WebCore/dom/DataTransferItem.h:41
> > +class DataTransferItem : public RefCounted<DataTransferItem> {
> 
> Shouldn't this header be conditional?
> 

Done.

> > Source/WebCore/dom/DataTransferItems.h:43
> > +class DataTransferItems : public RefCounted<DataTransferItems> {
> 
> Wrapped in #ifs?

Done.

[Daniel Cheng]

Does anyone know why the patch is failing to apply on the Mac bots? I suspect it has something to do with line-endings since it's just the .vcproj, but I'm not sure what.

[Daniel Cheng]

Created attachment 84992 [details]
Patch

[David Levin]

Comment on attachment 84992 [details]
Patch

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

Initial pass of comments.

> Source/WebCore/dom/DataTransferItem.h:36
> +#include "PlatformString.h"

fwd decl?

> Source/WebCore/dom/DataTransferItem.h:37
> +#include "StringCallback.h"

fwd decl?

> Source/WebCore/dom/DataTransferItems.h:36
> +#include "DataTransferItem.h"

Add fed decl instead?

> Source/WebCore/dom/DataTransferItems.h:37
> +#include "ExceptionCode.h"

Seems typical to put in a typedef instead.

> Source/WebCore/dom/DataTransferItems.h:39
> +#include "PlatformString.h"

Use fwd decl (fwiw, I think it is wtf/text/WTFString.h now.)

> Source/WebCore/dom/DataTransferItems.h:41
> +#include <wtf/RefCounted.h>

Why is this needed?

> Source/WebCore/dom/StringCallback.h:36
> +

Should include WTFString.h

> Source/WebCore/dom/StringCallback.h:45
> +    void scheduleCallback(ScriptExecutionContext* context, const String& data)

Consider moving to cpp file and changing the ScriptExecutionContext include to a fwd decl.

> Source/WebCore/dom/StringCallback.h:53
> +        DispatchCallbackTask(PassRefPtr<StringCallback> callback, const String& data)

Hide the constructor and expose a static create method which returns a PassOwnPtr.

> Source/WebCore/dom/StringCallback.h:54
> +            : m_callback(callback), m_data(data)

separate lines.

> Source/WebCore/dom/StringCallback.h:60
> +            m_callback->handleEvent(m_data);

This isn't inlined (due to being virtual) so put it in a cpp file.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:45
> +    : m_owner(owner), m_context(context), m_kind(kindString), m_type(type), m_data(data)

separate lines.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:52
> +        return "";

Why "" as opposed to String() ?

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:59
> +        return "";

Ditto.

> Source/WebCore/platform/chromium/DataTransferItemChromium.h:36
> +#include "Clipboard.h"

a fwd decl should suffice if you declare a destructor and put it in the cpp file.

> Source/WebCore/platform/chromium/DataTransferItemChromium.h:40
> +

include wtfstring.h

> Source/WebCore/platform/chromium/DataTransferItemChromium.h:56
> +    ScriptExecutionContext* m_context;

I would put in a fwd decl for ScriptExecutionContext.

> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:43
> +DataTransferItemsChromium::DataTransferItemsChromium(PassRefPtr<Clipboard> owner, ScriptExecutionContext* context) : m_owner(owner), m_context(context)

separate lines for the initializers.

> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:68
> +    if (index >= length())

Why doesn't this cause an exception code to be set?

> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:84
> +    if (m_owner->policy() != ClipboardWritable)

Why doesn't this cause an exception code to be set?

> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:88
> +      if (m_items[i]->type() == type && m_items[i]->kind() == DataTransferItem::kindString) {

indentation should be 4 spaces.

> Source/WebCore/platform/chromium/DataTransferItemsChromium.h:36
> +#include "Clipboard.h"

fwd decl.

> Source/WebCore/platform/chromium/DataTransferItemsChromium.h:55
> +    virtual void add(const String& data, const String& type, ExceptionCode&);

typedef for ExceptionCode?

fwd decl for String?

> Source/WebCore/platform/chromium/DataTransferItemsChromium.h:58
> +    DataTransferItemsChromium(PassRefPtr<Clipboard>, ScriptExecutionContext*);

fwd decl for ScriptExecutionContext?

[Daniel Cheng]

Created attachment 85003 [details]
Patch

[Daniel Cheng]

(In reply to comment #12)
> (From update of attachment 84992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84992&action=review
> 
> Initial pass of comments.
> 
> > Source/WebCore/dom/DataTransferItem.h:36
> > +#include "PlatformString.h"
> 
> fwd decl?
> 

Done.

> > Source/WebCore/dom/DataTransferItem.h:37
> > +#include "StringCallback.h"
> 
> fwd decl?
> 

Done.

> > Source/WebCore/dom/DataTransferItems.h:36
> > +#include "DataTransferItem.h"
> 
> Add fed decl instead?
> 

Done.

> > Source/WebCore/dom/DataTransferItems.h:37
> > +#include "ExceptionCode.h"
> 
> Seems typical to put in a typedef instead.
> 

Done.

> > Source/WebCore/dom/DataTransferItems.h:39
> > +#include "PlatformString.h"
> 
> Use fwd decl (fwiw, I think it is wtf/text/WTFString.h now.)
> 

Done.

> > Source/WebCore/dom/DataTransferItems.h:41
> > +#include <wtf/RefCounted.h>
> 
> Why is this needed?
> 

Base template for DataTransferItems.

> > Source/WebCore/dom/StringCallback.h:36
> > +
> 
> Should include WTFString.h
> 
> > Source/WebCore/dom/StringCallback.h:45
> > +    void scheduleCallback(ScriptExecutionContext* context, const String& data)
> 
> Consider moving to cpp file and changing the ScriptExecutionContext include to a fwd decl.

Done.

> 
> > Source/WebCore/dom/StringCallback.h:53
> > +        DispatchCallbackTask(PassRefPtr<StringCallback> callback, const String& data)
> 
> Hide the constructor and expose a static create method which returns a PassOwnPtr.
> 

I stole this from DOMFileSystem.h. Are they doing it wrong?

> > Source/WebCore/dom/StringCallback.h:54
> > +            : m_callback(callback), m_data(data)
> 
> separate lines.

Done.

> 
> > Source/WebCore/dom/StringCallback.h:60
> > +            m_callback->handleEvent(m_data);
> 
> This isn't inlined (due to being virtual) so put it in a cpp file.
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:45
> > +    : m_owner(owner), m_context(context), m_kind(kindString), m_type(type), m_data(data)
> 
> separate lines.
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:52
> > +        return "";
> 
> Why "" as opposed to String() ?
> 
> > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:59
> > +        return "";
> 
> Ditto.
> 

I've seen it both ways in the code.

> > Source/WebCore/platform/chromium/DataTransferItemChromium.h:36
> > +#include "Clipboard.h"
> 
> a fwd decl should suffice if you declare a destructor and put it in the cpp file.
> 
> > Source/WebCore/platform/chromium/DataTransferItemChromium.h:40
> > +
> 
> include wtfstring.h
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemChromium.h:56
> > +    ScriptExecutionContext* m_context;
> 
> I would put in a fwd decl for ScriptExecutionContext.
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:43
> > +DataTransferItemsChromium::DataTransferItemsChromium(PassRefPtr<Clipboard> owner, ScriptExecutionContext* context) : m_owner(owner), m_context(context)
> 
> separate lines for the initializers.
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:68
> > +    if (index >= length())
> 
> Why doesn't this cause an exception code to be set?
> 

The spec doesn't say to.

> > Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:84
> > +    if (m_owner->policy() != ClipboardWritable)
> 
> Why doesn't this cause an exception code to be set?
> 

Strict interpretation of the spec.

> > Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:88
> > +      if (m_items[i]->type() == type && m_items[i]->kind() == DataTransferItem::kindString) {
> 
> indentation should be 4 spaces.
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemsChromium.h:36
> > +#include "Clipboard.h"
> 
> fwd decl.
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemsChromium.h:55
> > +    virtual void add(const String& data, const String& type, ExceptionCode&);
> 
> typedef for ExceptionCode?
> 
> fwd decl for String?
> 

Done for ExceptionCode, though see my previous comment about WTFString.

> > Source/WebCore/platform/chromium/DataTransferItemsChromium.h:58
> > +    DataTransferItemsChromium(PassRefPtr<Clipboard>, ScriptExecutionContext*);
> 
> fwd decl for ScriptExecutionContext?

Done.

[Daniel Cheng]

> Done for ExceptionCode, though see my previous comment about WTFString.
 
Please ignore this comment. I was confused.

[WebKit Review Bot]

Attachment 85003 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/dom/Clipboard.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Comment on attachment 84992 [details]
Patch

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

A few more comments.

> LayoutTests/editing/pasteboard/data-transfer-items.html:4
> +<div>This test checks that reading/writing functionality of DataTransferItems. This test requires DRT.</div>

I don't understand this sentence "This test checks that reading/writing functionality of DataTransferItems"

> LayoutTests/editing/pasteboard/data-transfer-items.html:25
> +      items.add('Moo', 'text/plain');

inconsistent indentation. (2 spaces vs 4 in other places in this file).

> LayoutTests/editing/pasteboard/data-transfer-items.html:86
> +    if (savedDataTransferItem.type!= '') {

add space before !=

> LayoutTests/editing/pasteboard/data-transfer-items.html:88
> +        console.log('DataTransferItem.typeaccessible outside event handler!');

missing space "typeaccessible"

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:131
> +    if (!$codeGenerator->IsPrimitiveType($type) and !$codeGenerator->IsStringType($type) and !$codeGenerator->AvoidInclusionOfType($type) and $type ne "Date") {

Would be nice to explain this change in the ChangeLog.

Do we need to regenerate the output files?

Also do we need an equivalent change in CodeGeneratorJS.pm?

> Source/WebCore/dom/Clipboard.h:29
> +#include "DataTransferItems.h"

Can this be replaced with a forward declaration?

> Source/WebCore/dom/Clipboard.idl:46
> +        readonly attribute [Conditional=DATA_TRANSFER_ITEMS] DataTransferItems items;

This should have a runtime flag to turn it on, so that it isn't exposed until it is cleared for shipping in chromium.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:67
> +        || m_kind != kindString)

Why so many lines?

> Source/WebCore/platform/chromium/DataTransferItemChromium.h:43
> +class DataTransferItemChromium : public DataTransferItem {

Why is this class chromium specific? It looks very generic.

> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:63
> +    if (m_owner->policy() != ClipboardWritable) {

Why doesn't this also check for ClipboardNumb?

>>> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:84
>>> +    if (m_owner->policy() != ClipboardWritable)
>> 
>> Why doesn't this cause an exception code to be set?
> 
> Strict interpretation of the spec.

Why doesn't this also check for ClipboardNumb?

> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:87
> +    for (size_t i = 0; i < m_items.size(); ++i) {

Would be nice to add a comment right before the for loop.

Something like:
// Only one string item with a given type is allowed in the items collection.

[David Levin]

(In reply to comment #14)
> (In reply to comment #12)
> > (From update of attachment 84992 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=84992&action=review
> > 
> > 
> > > Source/WebCore/dom/StringCallback.h:53
> > > +        DispatchCallbackTask(PassRefPtr<StringCallback> callback, const String& data)
> > 
> > Hide the constructor and expose a static create method which returns a PassOwnPtr.
> > 
> 
> I stole this from DOMFileSystem.h. Are they doing it wrong?

Yes. (Well it works but it isn't the desired WebKit style.)

[David Levin]

Comment on attachment 85003 [details]
Patch

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

See comments above and one last comment. Once these are all addressed this should be good to go. Thanks!

> Source/WebCore/dom/DataTransferItems.h:45
> +class DataTransferItems : public RefCounted<DataTransferItems> {

Do we need a virtual destructor here?

[Daniel Cheng]

Created attachment 85009 [details]
Patch

[Daniel Cheng]

(In reply to comment #17)
> (From update of attachment 84992 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84992&action=review
> 
> A few more comments.
> 
> > LayoutTests/editing/pasteboard/data-transfer-items.html:4
> > +<div>This test checks that reading/writing functionality of DataTransferItems. This test requires DRT.</div>
> 
> I don't understand this sentence "This test checks that reading/writing functionality of DataTransferItems"
> 

Um. Is that any better? =P

> > LayoutTests/editing/pasteboard/data-transfer-items.html:25
> > +      items.add('Moo', 'text/plain');
> 
> inconsistent indentation. (2 spaces vs 4 in other places in this file).
> 

Done.

> > LayoutTests/editing/pasteboard/data-transfer-items.html:86
> > +    if (savedDataTransferItem.type!= '') {
> 
> add space before !=
> 

Done.

> > LayoutTests/editing/pasteboard/data-transfer-items.html:88
> > +        console.log('DataTransferItem.typeaccessible outside event handler!');
> 
> missing space "typeaccessible"
> 

Done.

> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:131
> > +    if (!$codeGenerator->IsPrimitiveType($type) and !$codeGenerator->IsStringType($type) and !$codeGenerator->AvoidInclusionOfType($type) and $type ne "Date") {
> 
> Would be nice to explain this change in the ChangeLog.

Done.

> 
> Do we need to regenerate the output files?
> 

This happens magically.

> Also do we need an equivalent change in CodeGeneratorJS.pm?
> 

I have no idea. It seemed to work last time I tried it.

> > Source/WebCore/dom/Clipboard.h:29
> > +#include "DataTransferItems.h"
> 
> Can this be replaced with a forward declaration?
> 

Already done.

> > Source/WebCore/dom/Clipboard.idl:46
> > +        readonly attribute [Conditional=DATA_TRANSFER_ITEMS] DataTransferItems items;
> 
> This should have a runtime flag to turn it on, so that it isn't exposed until it is cleared for shipping in chromium.
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:67
> > +        || m_kind != kindString)
> 
> Why so many lines?
> 

Done.

> > Source/WebCore/platform/chromium/DataTransferItemChromium.h:43
> > +class DataTransferItemChromium : public DataTransferItem {
> 
> Why is this class chromium specific? It looks very generic.
> 

It's getting some pasteboard specific logic in a follow up patch.

> > Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:63
> > +    if (m_owner->policy() != ClipboardWritable) {
> 
> Why doesn't this also check for ClipboardNumb?
> 
> >>> Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:84
> >>> +    if (m_owner->policy() != ClipboardWritable)
> >> 
> >> Why doesn't this cause an exception code to be set?
> > 
> > Strict interpretation of the spec.
> 
> Why doesn't this also check for ClipboardNumb?

I don't follow. We only allow these operations if the data store should be writable.

> 
> > Source/WebCore/platform/chromium/DataTransferItemsChromium.cpp:87
> > +    for (size_t i = 0; i < m_items.size(); ++i) {
> 
> Would be nice to add a comment right before the for loop.
> 
> Something like:
> // Only one string item with a given type is allowed in the items collection.

Done.

(In reply to comment #19)
> (From update of attachment 85003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85003&action=review
> 
> See comments above and one last comment. Once these are all addressed this should be good to go. Thanks!
> 
> > Source/WebCore/dom/DataTransferItems.h:45
> > +class DataTransferItems : public RefCounted<DataTransferItems> {
> 
> Do we need a virtual destructor here?

Done.

[WebKit Review Bot]

Attachment 85009 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/dom/Clipboard.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Cheng]

Created attachment 85010 [details]
Patch

[WebKit Review Bot]

Attachment 85010 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1

Source/WebCore/dom/Clipboard.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Comment on attachment 85010 [details]
Patch

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

Please fix the items commented on and check in.

> Source/WebCore/dom/DataTransferItem.h:44
> +public:

Need virtual destructor.

> Source/WebCore/dom/DataTransferItems.h:46
> +public:

Add virtual destructor.

> Source/WebCore/dom/StringCallback.cpp:43
> +    static DispatchCallbackTask* create(PassRefPtr<StringCallback> callback, const String& data)

return type should be PassOwnPtr<>

> Source/WebCore/dom/StringCallback.cpp:45
> +        return new DispatchCallbackTask(callback, data);

Should be "return adoptPtr(new ..."

[Daniel Cheng]

Committed r80536: <http://trac.webkit.org/changeset/80536>

[WebKit Review Bot]

http://trac.webkit.org/changeset/80536 might have broken EFL Linux Release (Build)

[Alejandro G. Castro]

The commit broke gtk compilation:

http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/20119/steps/compile-webkit/logs/stdio