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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40540


Daniel Cheng <dcheng at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #58739|0                           |1
        is obsolete|                            |
  Attachment #58820|                            |review?
               Flag|                            |




--- Comment #17 from Daniel Cheng <dcheng at chromium.org>  2010-06-15 14:44:03 PST ---
Created an attachment (id=58820)
 --> (https://bugs.webkit.org/attachment.cgi?id=58820)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.