Bug 40540 – [chromium] Add new stubs for querying platform drag-and-drop and copy-and-paste data.

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Daniel Cheng]

The interface needs to be defined in WebKit first; the Chrome-side patches will come once this lands. With this new model, the data will only be pulled on demand.
For simplicity on Mac and Windows, we combine the two interfaces. This, unfortunately, doesn't help Linux out very much, but that's just how it is.

[Tony Chang]

Will this be sync IPC calls from the renderer to the browser?  That's ok, but it seems like it could be a lot of IPCs when moving the mouse quickly during a drag.  Isn't this how paste currently works (a sync IPC from the renderer to the browser to get the paste data)?

[Daniel Cheng]

(In reply to comment #1)
> Will this be sync IPC calls from the renderer to the browser?  That's ok, but it seems like it could be a lot of IPCs when moving the mouse quickly during a drag.  Isn't this how paste currently works (a sync IPC from the renderer to the browser to get the paste data)?

It's a sync IPC call. It is a lot of IPC calls, but getting the list of types is generally cheap. The expensive part is getting the data. I've tested it locally, and it doesn't /seem/ to cause problems.

An alternative approach we could take if we find that performance is an issue is populate the most commonly requested pieces of data (such as the types in the clipboard, for example) in advance rather than passing it by IPC.

[Tony Chang]

(In reply to comment #2)
> It's a sync IPC call. It is a lot of IPC calls, but getting the list of types is generally cheap. The expensive part is getting the data. I've tested it locally, and it doesn't /seem/ to cause problems.

Is the sync message being handled in browser/renderer_host/resource_message_filter.cc?  I think there are possible deadlocks if you try to handle it in render_view_host.

> An alternative approach we could take if we find that performance is an issue is populate the most commonly requested pieces of data (such as the types in the clipboard, for example) in advance rather than passing it by IPC.

It seems like it would be good to pre-populate the types in the clipboard since I don't think those can change during a drag operation.

[Daniel Cheng]

(In reply to comment #3)
> (In reply to comment #2)
> > It's a sync IPC call. It is a lot of IPC calls, but getting the list of types is generally cheap. The expensive part is getting the data. I've tested it locally, and it doesn't /seem/ to cause problems.
> 
> Is the sync message being handled in browser/renderer_host/resource_message_filter.cc?  I think there are possible deadlocks if you try to handle it in render_view_host.

Yes.

> 
> > An alternative approach we could take if we find that performance is an issue is populate the most commonly requested pieces of data (such as the types in the clipboard, for example) in advance rather than passing it by IPC.
> 
> It seems like it would be good to pre-populate the types in the clipboard since I don't think those can change during a drag operation.

Fair enough. I'll make the change.

[Daniel Cheng]

Created attachment 58696 [details]
Patch

[Daniel Cheng]

Created attachment 58697 [details]
Patch

Remove an unintended change.

[WebKit Review Bot]

Attachment 58697 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3310150

[Tony Chang]

Comment on attachment 58697 [details]
Patch

WebCore/ChangeLog:15
 +          No new tests.
Please add a reason why no new tests.  E.g., no new tests because this is adding an api. when we switch to it, it will be covered by the existing tests.

WebCore/platform/chromium/ChromiumBridge.h: 
 +      class Color;
Please do the style changes in a separate change.  It's hard to see the actual changes.

WebCore/platform/chromium/PasteboardPrivate.h: 
 +      class PasteboardPrivate
This style fix can be included in the above style patch.

[Daniel Cheng]

Created attachment 58736 [details]
Patch

Still trying to figure out why it doesn't build on cr-linux. Other comments should be addressed though.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/chromium/ChromiumBridge.h:54:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/chromium/PasteboardPrivate.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 8 files


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

[Tony Chang]

(In reply to comment #9)
> Created an attachment (id=58736) [details]
> Patch
> 
> Still trying to figure out why it doesn't build on cr-linux. Other comments should be addressed though.

I think it's failing because the bot doesn't re-run gyp, so it doesn't pick up the new file.  If it builds on a linux machine (webkit only checkout), it's probably fine.

[Tony Chang]

LGTM.  Might want to re-write the ChangeLog description (it's not IPC specific).

cc-ing darin for final review since it touches the webkit api.

[Daniel Cheng]

Created attachment 58739 [details]
Patch

Remove bogus use of const PassRefPtr<T>& and remove stale reference to IPC in ChangeLog. There's a couple places in WebKit/chromium that do this, and though I don't really understand why, I'm pretty sure my use of it was incorrect.

I'll test the compile on my Linux machine.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/chromium/PasteboardPrivate.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


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

[WebKit Review Bot]

Attachment 58739 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3284127

[Darin Fisher (:fishd, Google)]

Comment on attachment 58739 [details]
Patch

WebCore/platform/chromium/ChromiumBridge.h:96
 +          static bool dataTransferGetData(PasteboardPrivate::Source source, const String& type, String& data, String& metadata);
nit: in webkit style, parameter names should be excluded when they don't add any information.
e.g., calling a parameter name "source" when the type name is "Source" doesn't
add any information.

WebKit/chromium/public/WebKitClient.h:90
 +      virtual bool clipboardWriteData(const WebDragData& data) { return false; }
nit: exclude the 'data' param name

WebKit/chromium/public/WebKitClient.h:90
 +      virtual bool clipboardWriteData(const WebDragData& data) { return false; }
please document the meaning of the bool return value.

WebKit/chromium/public/WebDataTransfer.h:38
 +      enum Source {
i'd recommend prefixing the enum values with Source.  that way when you see
WebDataTransfer::SourceClipboard, it is obvious that the value refers to the
source of the data transfer.

WebKit/chromium/public/WebKitClient.h:90
 +      virtual bool clipboardWriteData(const WebDragData& data) { return false; }
shouldn't this method be declared on WebClipboard instead?  that
interface exists to collect all of the clipboard related callbacks
to the embedder.

WebKit/chromium/public/WebKitClient.h:91
 +      virtual bool dataTransferGetData(WebDataTransfer::Source source, const WebString& type, WebString* data, WebString* metadata) { return false; }
the other methods on WebKitClient are not namespaced like this.
names that read more naturally are preferred:  getDataTransferData, getDataTransferFilenames.

WebKit/chromium/public/WebKitClient.h:92
 +      virtual WebVector<WebString> dataTransferGetFilenames(WebDataTransfer::Source source) { return WebVector<WebString>(); }
but, perhaps WebDataTransfer should be a struct with fields data, metadata, and filenames?  then you could just call a function on WebKitClient named getDataTransfer?  is there any reason why you would want the data and metadata but not the filenames?

[Daniel Cheng]

Created attachment 58820 [details]
Patch

(In reply to comment #16)
> (From update of attachment 58739 [details])
> WebCore/platform/chromium/ChromiumBridge.h:96
>  +          static bool dataTransferGetData(PasteboardPrivate::Source source, const String& type, String& data, String& metadata);
> nit: in webkit style, parameter names should be excluded when they don't add any information.
> e.g., calling a parameter name "source" when the type name is "Source" doesn't
> add any information.
> 

Done.

> WebKit/chromium/public/WebKitClient.h:90
>  +      virtual bool clipboardWriteData(const WebDragData& data) { return false; }
> nit: exclude the 'data' param name
> 

Done.

> WebKit/chromium/public/WebKitClient.h:90
>  +      virtual bool clipboardWriteData(const WebDragData& data) { return false; }
> please document the meaning of the bool return value.
> 

Done. I removed the return type, to more closely match the existing clipboard functions.

> WebKit/chromium/public/WebDataTransfer.h:38
>  +      enum Source {
> i'd recommend prefixing the enum values with Source.  that way when you see
> WebDataTransfer::SourceClipboard, it is obvious that the value refers to the
> source of the data transfer.
> 

Done. I updated PasteboardPrivate as well, though I used SourceX to keep the naming between the two consistent.

> WebKit/chromium/public/WebKitClient.h:90
>  +      virtual bool clipboardWriteData(const WebDragData& data) { return false; }
> shouldn't this method be declared on WebClipboard instead?  that
> interface exists to collect all of the clipboard related callbacks
> to the embedder.
> 

Why do we go through all that indirection? I find it hard to follow personally:
ChromiumBridge->WebKitClient->WebClipboard->WebClipboardImpl->renderer glue, which in the end, just calls an IPC. Is this for the test shell?

Isn't it simpler to just say:
ChromiumBridge->WebKitClient->IPC?

> WebKit/chromium/public/WebKitClient.h:91
>  +      virtual bool dataTransferGetData(WebDataTransfer::Source source, const WebString& type, WebString* data, WebString* metadata) { return false; }
> the other methods on WebKitClient are not namespaced like this.
> names that read more naturally are preferred:  getDataTransferData, getDataTransferFilenames.
> 

Done.

> WebKit/chromium/public/WebKitClient.h:92
>  +      virtual WebVector<WebString> dataTransferGetFilenames(WebDataTransfer::Source source) { return WebVector<WebString>(); }
> but, perhaps WebDataTransfer should be a struct with fields data, metadata, and filenames?  then you could just call a function on WebKitClient named getDataTransfer?  is there any reason why you would want the data and metadata but not the filenames?

No one needs the data until the drop actually happens. At that point, the drop target usually only cares about one specific flavor of data. Filenames happens to be a flavor of data that has its own special accessor, since the HTML5 draft doesn't specify a MIME type for it.

[Daniel Cheng]

Created attachment 58821 [details]
Patch

Fix stale ChangeLog entry.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/chromium/PasteboardPrivate.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


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

[Darin Fisher (:fishd, Google)]

(In reply to comment #17)
> > WebKit/chromium/public/WebKitClient.h:90
> >  +      virtual bool clipboardWriteData(const WebDragData& data) { return false; }
> > shouldn't this method be declared on WebClipboard instead?  that
> > interface exists to collect all of the clipboard related callbacks
> > to the embedder.
> > 
> 
> Why do we go through all that indirection? I find it hard to follow personally:
> ChromiumBridge->WebKitClient->WebClipboard->WebClipboardImpl->renderer glue, which in the end, just calls an IPC. Is this for the test shell?
> 
> Isn't it simpler to just say:
> ChromiumBridge->WebKitClient->IPC?

It helps provide some grouping to WebKitClient.  I think it would
become a giant mess to have everything flattened into a single
implementation.


> > WebKit/chromium/public/WebKitClient.h:92
> >  +      virtual WebVector<WebString> dataTransferGetFilenames(WebDataTransfer::Source source) { return WebVector<WebString>(); }
> > but, perhaps WebDataTransfer should be a struct with fields data, metadata, and filenames?  then you could just call a function on WebKitClient named getDataTransfer?  is there any reason why you would want the data and metadata but not the filenames?
> 
> No one needs the data until the drop actually happens. At that point, the drop target usually only cares about one specific flavor of data. Filenames happens to be a flavor of data that has its own special accessor, since the HTML5 draft doesn't specify a MIME type for it.

OK

[Daniel Cheng]

Created attachment 58835 [details]
Patch

(In reply to comment #20)
> (In reply to comment #17)
> > > WebKit/chromium/public/WebKitClient.h:90
> > >  +      virtual bool clipboardWriteData(const WebDragData& data) { return false; }
> > > shouldn't this method be declared on WebClipboard instead?  that
> > > interface exists to collect all of the clipboard related callbacks
> > > to the embedder.
> > > 
> > 
> > Why do we go through all that indirection? I find it hard to follow personally:
> > ChromiumBridge->WebKitClient->WebClipboard->WebClipboardImpl->renderer glue, which in the end, just calls an IPC. Is this for the test shell?
> > 
> > Isn't it simpler to just say:
> > ChromiumBridge->WebKitClient->IPC?
> 
> It helps provide some grouping to WebKitClient.  I think it would
> become a giant mess to have everything flattened into a single
> implementation.
> 

Done.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/chromium/PasteboardPrivate.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


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

[WebKit Review Bot]

Attachment 58821 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3326190

[Daniel Cheng]

Created attachment 58840 [details]
Patch

Remove dependency on ClipboardData header (which is not checked in yet). It's blocked on a separate patch.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/chromium/ChromiumBridge.h:54:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/chromium/PasteboardPrivate.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 9 files


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

[WebKit Review Bot]

Attachment 58835 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3327187

[Daniel Cheng]

> > An alternative approach we could take if we find that performance is an issue is populate the most commonly requested pieces of data (such as the types in the clipboard, for example) in advance rather than passing it by IPC.
> 
> It seems like it would be good to pre-populate the types in the clipboard since I don't think those can change during a drag operation.

I thought about it more, and clipboard operations still require an IPC to retrieve the available types. In the spirit of keeping it consistent (what I'm trying really hard to do with this patch is unify copy-and-paste and drag-and-drop things that can be shared), I think it's best to leave retrieving the types as an IPC. I'll make the WebKit side a bit cleverer and cache the results from the first IPC if no one has any objections.

[Tony Chang]

(In reply to comment #27)
> > > An alternative approach we could take if we find that performance is an issue is populate the most commonly requested pieces of data (such as the types in the clipboard, for example) in advance rather than passing it by IPC.
> > 
> > It seems like it would be good to pre-populate the types in the clipboard since I don't think those can change during a drag operation.
> 
> I thought about it more, and clipboard operations still require an IPC to retrieve the available types. In the spirit of keeping it consistent (what I'm trying really hard to do with this patch is unify copy-and-paste and drag-and-drop things that can be shared), I think it's best to leave retrieving the types as an IPC. I'll make the WebKit side a bit cleverer and cache the results from the first IPC if no one has any objections.

That sounds OK to me.

[WebKit Review Bot]

Attachment 58840 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3290213

[Daniel Cheng]

Created attachment 58846 [details]
Patch

Final-ish version I hope. Will be asking around to see if anyone can figure out what's causing the build error (it works when I patch it into my WebKit branch under Linux Chrome).

[Daniel Cheng]

Created attachment 58847 [details]
Patch

... and fix a WebKit style error. Argh.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/chromium/ChromiumBridge.h:54:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/chromium/PasteboardPrivate.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 9 files


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

[WebKit Review Bot]

Attachment 58846 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3316202

[WebKit Review Bot]

Attachment 58847 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3287214

[Darin Fisher (:fishd, Google)]

Comment on attachment 58847 [details]
Patch

r- since the cr-linux bot failed to compile.

[Daniel Cheng]

(In reply to comment #35)
> (From update of attachment 58847 [details])
> r- since the cr-linux bot failed to compile.

It built for me locally. abarth will look into this later and see if it's a problem with the bot or with my patch. I don't see anything obviously wrong with my patch...

[Daniel Cheng]

Created attachment 58937 [details]
Patch

Just testing. Flagging it R? so the EWS bots try out this patch. Nothing has changed.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/chromium/ChromiumBridge.h:54:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/chromium/PasteboardPrivate.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 9 files


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

[WebKit Review Bot]

Attachment 58937 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3290268

[Daniel Cheng]

(In reply to comment #39)
> Attachment 58937 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/3290268

So I can only repro this if I don't run update-webkit --chromium first to update the makefiles. As long as I run update-webkit --chromium before building, it works.

If there are no other objections, can I get a CQ+? If it does end up breaking the Chrome bot, I will roll it out immediately.

[Daniel Cheng]

(In reply to comment #40)
> (In reply to comment #39)
> > Attachment 58937 [details] [details] did not build on chromium:
> > Build output: http://webkit-commit-queue.appspot.com/results/3290268
> 
> So I can only repro this if I don't run update-webkit --chromium first to update the makefiles. As long as I run update-webkit --chromium before building, it works.
> 
> If there are no other objections, can I get a CQ+? If it does end up breaking the Chrome bot, I will roll it out immediately.

I figured out why my patch fails on the Chrome EWS bot. The Chrome EWS bot does not seem to run GYP after it applies a patch. Other patches that have added new files didn't fail because there were no dependencies from other files on the new files. My patch added a dependency on WebDataTransfer.h from WebKitClient.h, which is why the compile fails. The normal build bots should handle this change correctly.

In the long run, it'd be nice to make the cr-linux bot run GYP when it needs to.

[Darin Fisher (:fishd, Google)]

Comment on attachment 58937 [details]
Patch

WebKit/chromium/public/WebClipboard.h:71
 +      // FIXME: Make this pure virtual after landing Chrome changes.
no need.  since this is an interface implemented by chromium, we should make all of
the methods have default implementations.  that is what we do normally for interfaces
implemented by the embedder.  so instead of the FIXME, you can just change all methods
to have defualt implementations :)
WebKit/chromium/public/WebDataTransfer.h:39
 +          ClipboardSource,
nit: the convention for the API is to put the common name first.  so it should
be SourceClipboard, SourceSelection, SourceDrag instead.

WebKit/chromium/public/WebKitClient.h:90
 +      virtual WebVector<WebString> getDataTransferTypes(WebDataTransfer::Source) { return WebVector<WebString>(); }
it seems like it would be better to either put these methods on WebClipboard
or make WebKitClient have a method that returns the current WebDataTransfer.
that way we are not just dumping more stuff in WebKitClient.

WebKit/chromium/src/ChromiumBridge.cpp:220
 +      WebKit::WebString resultData;
no need for WebKit:: prefix in this file.  there is a using decl up top.

[Daniel Cheng]

Created attachment 59101 [details]
Patch

(In reply to comment #42)
> (From update of attachment 58937 [details])
> WebKit/chromium/public/WebClipboard.h:71
>  +      // FIXME: Make this pure virtual after landing Chrome changes.
> no need.  since this is an interface implemented by chromium, we should make all of
> the methods have default implementations.  that is what we do normally for interfaces
> implemented by the embedder.  so instead of the FIXME, you can just change all methods
> to have defualt implementations :)

Done.

> WebKit/chromium/public/WebDataTransfer.h:39
>  +          ClipboardSource,
> nit: the convention for the API is to put the common name first.  so it should
> be SourceClipboard, SourceSelection, SourceDrag instead.
> 

Done.

> WebKit/chromium/public/WebKitClient.h:90
>  +      virtual WebVector<WebString> getDataTransferTypes(WebDataTransfer::Source) { return WebVector<WebString>(); }
> it seems like it would be better to either put these methods on WebClipboard
> or make WebKitClient have a method that returns the current WebDataTransfer.
> that way we are not just dumping more stuff in WebKitClient.
> 

Done.

> WebKit/chromium/src/ChromiumBridge.cpp:220
>  +      WebKit::WebString resultData;
> no need for WebKit:: prefix in this file.  there is a using decl up top.

Done.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/chromium/ChromiumBridge.h:64:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/chromium/PasteboardPrivate.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 9 files


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

[Daniel Cheng]

Created attachment 59105 [details]
Patch

Update stale ChangeLog.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/chromium/ChromiumBridge.h:64:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/chromium/PasteboardPrivate.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 9 files


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

[WebKit Review Bot]

Attachment 59101 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3318355

[WebKit Review Bot]

Attachment 59105 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3270375

[Darin Fisher (:fishd, Google)]

Comment on attachment 59105 [details]
Patch

WebKit/chromium/public/WebDataTransfer.h:47
 +      virtual WebVector<WebString> types(Source, bool* containsFilenames) = 0;
since this interface is implemented by the embedder, these methods should all
have default implementations.

WebKit/chromium/public/WebKitClient.h:79
 +      virtual WebDataTransfer* dataTransfer() { return 0; }
it would be very helpful if you could include some documentation
about what WebDataTransfer is, and it is not clear to me why it
is a global thing.  should this method be named currentDataTransfer
to suggest that this is the information about a current data transfer
if any exists.

[Daniel Cheng]

Created attachment 59308 [details]
Patch

(In reply to comment #49)
> (From update of attachment 59105 [details])
> WebKit/chromium/public/WebDataTransfer.h:47
>  +      virtual WebVector<WebString> types(Source, bool* containsFilenames) = 0;
> since this interface is implemented by the embedder, these methods should all
> have default implementations.
> 

Chromium patch posted at http://codereview.chromium.org/2830019.

> WebKit/chromium/public/WebKitClient.h:79
>  +      virtual WebDataTransfer* dataTransfer() { return 0; }
> it would be very helpful if you could include some documentation
> about what WebDataTransfer is, and it is not clear to me why it
> is a global thing.  should this method be named currentDataTransfer
> to suggest that this is the information about a current data transfer
> if any exists.

Per our conversation, I've updated the patch and folded all the WebDataTransfer stuff into WebClipboard.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/chromium/ChromiumBridge.h:64:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/chromium/PasteboardPrivate.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 6 files


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

[WebKit Review Bot]

Attachment 59308 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3340583

[Darin Fisher (:fishd, Google)]

Comment on attachment 59308 [details]
Patch

WebCore/platform/chromium/ChromiumBridge.h:106
 +          static HashSet<String> clipboardReadTypes(PasteboardPrivate::ClipboardBuffer, bool* containsFilenames);
clipboardReadTypes might be better named clipboardGetAvailableTypes

WebKit/chromium/public/WebClipboard.h:57
 +          BufferDrag,
It is probably worth saying something about the meaning of BufferDrag here.
Mention that it is a readonly buffer corresponding to the last drag-n-drop
operation or something like that.
WebKit/chromium/public/WebClipboard.h:77
 +      virtual WebVector<WebString> readTypes(Buffer, bool* containsFilenames) = 0;
readTypes -> getAvailableTypes or maybe readAvailableTypes?  what does the
return value of readData indicate?  please specify.

WebKit/chromium/src/ChromiumBridge.cpp:203
 +      WebDragData dragData; // FIXME: Define the conversion from ClipboardData to WebDragData.
this would be a good place to call notImplemented()

WebKit/chromium/src/ChromiumBridge.cpp:221
 +      WebKit::WebString resultData;
no need for the WebKit:: prefix here

[Darin Fisher (:fishd, Google)]

Comment on attachment 59308 [details]
Patch

WebKit/chromium/public/WebClipboard.h:80
 +      virtual WebVector<WebString> readFilenames(Buffer) = 0;
please provide default implementations of these WebClipboard methods.
any method to be implemented by the embedder should have a default
implementation.

[Daniel Cheng]

Created attachment 59409 [details]
Patch

(In reply to comment #53)
> (From update of attachment 59308 [details])
> WebCore/platform/chromium/ChromiumBridge.h:106
>  +          static HashSet<String> clipboardReadTypes(PasteboardPrivate::ClipboardBuffer, bool* containsFilenames);
> clipboardReadTypes might be better named clipboardGetAvailableTypes
> 

Done. See comment below about naming.

> WebKit/chromium/public/WebClipboard.h:57
>  +          BufferDrag,
> It is probably worth saying something about the meaning of BufferDrag here.
> Mention that it is a readonly buffer corresponding to the last drag-n-drop
> operation or something like that.

Done.

> WebKit/chromium/public/WebClipboard.h:77
>  +      virtual WebVector<WebString> readTypes(Buffer, bool* containsFilenames) = 0;
> readTypes -> getAvailableTypes or maybe readAvailableTypes?  what does the
> return value of readData indicate?  please specify.
> 

Done and done. I chose read over get for consistency.

> WebKit/chromium/src/ChromiumBridge.cpp:203
>  +      WebDragData dragData; // FIXME: Define the conversion from ClipboardData to WebDragData.
> this would be a good place to call notImplemented()
> 

Done.

> WebKit/chromium/src/ChromiumBridge.cpp:221
>  +      WebKit::WebString resultData;
> no need for the WebKit:: prefix here

Done.

(In reply to comment #54)
> (From update of attachment 59308 [details])
> WebKit/chromium/public/WebClipboard.h:80
>  +      virtual WebVector<WebString> readFilenames(Buffer) = 0;
> please provide default implementations of these WebClipboard methods.
> any method to be implemented by the embedder should have a default
> implementation.

Done.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/chromium/ChromiumBridge.h:64:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/chromium/PasteboardPrivate.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 6 files


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

[Darin Fisher (:fishd, Google)]

Comment on attachment 59409 [details]
Patch

WebKit/chromium/public/WebClipboard.h:58
 +          BufferDrag,
nit: please add a new line in AssertMatchingEnums.cpp

otherwise R=me

[Daniel Cheng]

Created attachment 59548 [details]
Patch

Added the compile assert to AssertMatchingEnums.cpp

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/chromium/ChromiumBridge.h:64:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/chromium/PasteboardPrivate.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 7 files


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

[Eric Seidel (no email)]

Comment on attachment 59409 [details]
Patch

Cleared Darin Fisher's review+ from obsolete attachment 59409 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 59548 [details]
Patch

Clearing flags on attachment: 59548

Committed r61898: <http://trac.webkit.org/changeset/61898>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.