[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
bugzilla-daemon at webkit.org
Mon Jun 14 21:57:23 PDT 2010
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?
More information about the webkit-reviews
mailing list