[webkit-reviews] review requested: [Bug 40540] [chromium] Add new stubs for querying platform drag-and-drop and copy-and-paste data. : [Attachment 58820] Patch

[bugzilla-daemon at webkit.org]

Daniel Cheng <dcheng at chromium.org> has asked  for review:
Bug 40540: [chromium] Add new stubs for querying platform drag-and-drop and
copy-and-paste data.
https://bugs.webkit.org/show_bug.cgi?id=40540

Attachment 58820: Patch
https://bugs.webkit.org/attachment.cgi?id=58820&action=review

------- Additional Comments from Daniel Cheng <dcheng at chromium.org>
(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.