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

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Daniel Cheng
<dcheng at chromium.org>'s request 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 58739: Patch
https://bugs.webkit.org/attachment.cgi?id=58739&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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?