Bug 44914 – [chromium] Add an interface for platform copy/paste drag/drop data objects

• Component: HTML Editing
• Status: RESOLVED FIXED

[Daniel Cheng]

This is useful because we do not plan on copying all the data over in WebView::dragTargetDragEnter anymore. Instead of manually nesting many if's and else's in ClipboardChromium or ChromiumDataObject, we use virtual dispatch.

ReadableDataObject and WritableDataObject will implement this interface:
ReadableDataObject is used to retrieve clipboard and drag/drop data via IPCs; WritableDataObject is (theoretically) to buffer writes until WebViewHost::startDragging or the (currently non-existent) Pasteboard equivalent is called.

[Daniel Cheng]

Created attachment 65985 [details]
Patch

[Tony Chang]

Comment on attachment 65985 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +        [chromium] Add an interface for platform copy/paste drag/drop data objects
> +        https://bugs.webkit.org/show_bug.cgi?id=44914

Nit: Please add a sentence explaining the general goal (ie., something about so we can eventually support arbitrary data types).

> diff --git a/WebCore/platform/chromium/ChromiumDataObjectNew.h b/WebCore/platform/chromium/ChromiumDataObjectNew.h
> +    virtual String getData(const String& type, bool& succeeded) const = 0;
> +    virtual bool setData(const String& type, const String& data) = 0;

It seems like these methods should use the same style.  Two possibilities: both return a bool (on success) or just remove the bool param complete from getdata (do we need to differentiate between empty and fail?).  The other getters below don't seem to use a bool.

[Daniel Cheng]

Created attachment 65989 [details]
Patch

(In reply to comment #2)
> (From update of attachment 65985 [details])
> > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> > +        [chromium] Add an interface for platform copy/paste drag/drop data objects
> > +        https://bugs.webkit.org/show_bug.cgi?id=44914
> 
> Nit: Please add a sentence explaining the general goal (ie., something about so we can eventually support arbitrary data types).

Done. Updating the other patch as well.

> 
> > diff --git a/WebCore/platform/chromium/ChromiumDataObjectNew.h b/WebCore/platform/chromium/ChromiumDataObjectNew.h
> > +    virtual String getData(const String& type, bool& succeeded) const = 0;
> > +    virtual bool setData(const String& type, const String& data) = 0;
> 
> It seems like these methods should use the same style.  Two possibilities: both return a bool (on success) or just remove the bool param complete from getdata (do we need to differentiate between empty and fail?).  The other getters below don't seem to use a bool.

This is inherited from Clipboard's getData() / setData() signature. I agree it's not ideal, but I don't feel comfortable changing Clipboard.

[Tony Chang]

(In reply to comment #3)
> This is inherited from Clipboard's getData() / setData() signature. I agree it's not ideal, but I don't feel comfortable changing Clipboard.

Please file a bug for this.  Bonus points for actually fixing.  Refactoring is important work!

[WebKit Commit Bot]

Comment on attachment 65989 [details]
Patch

Clearing flags on attachment: 65989

Committed r66483: <http://trac.webkit.org/changeset/66483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.