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.
Created attachment 83615 [details] Patch
Comment on attachment 83615 [details] Patch Forgot to include a change.
Created attachment 83619 [details] Patch
Created attachment 83624 [details] Patch
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?
Attachment 83624 [details] did not build on win: Build output: http://queues.webkit.org/results/7984415
Attachment 83619 [details] did not build on win: Build output: http://queues.webkit.org/results/7983471
Created attachment 84122 [details] Patch
(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.
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.
Created attachment 84992 [details] Patch
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?
Created attachment 85003 [details] Patch
(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.
> Done for ExceptionCode, though see my previous comment about WTFString. Please ignore this comment. I was confused.
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.
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.
(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.)
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?
Created attachment 85009 [details] Patch
(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.
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.
Created attachment 85010 [details] Patch
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.
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 ..."
Committed r80536: <http://trac.webkit.org/changeset/80536>
http://trac.webkit.org/changeset/80536 might have broken EFL Linux Release (Build)
The commit broke gtk compilation: http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/20119/steps/compile-webkit/logs/stdio