WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
55115
Add support for DataTransferItems
https://bugs.webkit.org/show_bug.cgi?id=55115
Summary
Add support for DataTransferItems
Daniel Cheng
Reported
2011-02-23 23:07:41 PST
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.
Attachments
Patch
(21.32 KB, patch)
2011-02-24 00:29 PST
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Patch
(21.61 KB, patch)
2011-02-24 01:27 PST
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Patch
(21.64 KB, patch)
2011-02-24 01:58 PST
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Patch
(35.33 KB, patch)
2011-02-28 14:32 PST
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Patch
(65.69 KB, patch)
2011-03-07 16:01 PST
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Patch
(69.44 KB, patch)
2011-03-07 18:00 PST
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Patch
(76.04 KB, patch)
2011-03-07 19:02 PST
,
Daniel Cheng
no flags
Details
Formatted Diff
Diff
Patch
(76.05 KB, patch)
2011-03-07 19:06 PST
,
Daniel Cheng
levin
: review+
levin
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Daniel Cheng
Comment 1
2011-02-24 00:29:32 PST
Created
attachment 83615
[details]
Patch
Daniel Cheng
Comment 2
2011-02-24 00:42:32 PST
Comment on
attachment 83615
[details]
Patch Forgot to include a change.
Daniel Cheng
Comment 3
2011-02-24 01:27:28 PST
Created
attachment 83619
[details]
Patch
Daniel Cheng
Comment 4
2011-02-24 01:58:52 PST
Created
attachment 83624
[details]
Patch
Eric Seidel (no email)
Comment 5
2011-02-24 02:23:56 PST
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
Comment 6
2011-02-24 02:51:28 PST
Attachment 83624
[details]
did not build on win: Build output:
http://queues.webkit.org/results/7984415
Build Bot
Comment 7
2011-02-24 03:52:01 PST
Attachment 83619
[details]
did not build on win: Build output:
http://queues.webkit.org/results/7983471
Daniel Cheng
Comment 8
2011-02-28 14:32:57 PST
Created
attachment 84122
[details]
Patch
Daniel Cheng
Comment 9
2011-02-28 18:09:47 PST
(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
Comment 10
2011-02-28 23:35:03 PST
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
Comment 11
2011-03-07 16:01:42 PST
Created
attachment 84992
[details]
Patch
David Levin
Comment 12
2011-03-07 16:38:19 PST
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
Comment 13
2011-03-07 18:00:20 PST
Created
attachment 85003
[details]
Patch
Daniel Cheng
Comment 14
2011-03-07 18:00:30 PST
(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
Comment 15
2011-03-07 18:01:19 PST
> Done for ExceptionCode, though see my previous comment about WTFString.
Please ignore this comment. I was confused.
WebKit Review Bot
Comment 16
2011-03-07 18:02:19 PST
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 17
2011-03-07 18:03:38 PST
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
Comment 18
2011-03-07 18:06:59 PST
(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 19
2011-03-07 18:08:14 PST
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
Comment 20
2011-03-07 19:02:57 PST
Created
attachment 85009
[details]
Patch
Daniel Cheng
Comment 21
2011-03-07 19:03:11 PST
(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
Comment 22
2011-03-07 19:05:27 PST
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
Comment 23
2011-03-07 19:06:04 PST
Created
attachment 85010
[details]
Patch
WebKit Review Bot
Comment 24
2011-03-07 19:08:44 PST
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 25
2011-03-07 21:23:38 PST
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
Comment 26
2011-03-07 21:48:56 PST
Committed
r80536
: <
http://trac.webkit.org/changeset/80536
>
WebKit Review Bot
Comment 27
2011-03-07 22:01:25 PST
http://trac.webkit.org/changeset/80536
might have broken EFL Linux Release (Build)
Alejandro G. Castro
Comment 28
2011-03-07 23:30:56 PST
The commit broke gtk compilation:
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/20119/steps/compile-webkit/logs/stdio
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug