Bug 30623 – [GTK] Enable DOM clipboard and drag-and-drop access

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

DOM DataTransfer objects should be able to get and set clipboard, drag source and drag destination data.

[Martin Robinson]

Created attachment 41562 [details]
Draft patch for centralizing clipboard code

I'm attaching a draft patch for the first part of this effort. The patch centralizes the clipboard code (it was split up into one section for GDK_SELECTION_CLIPBOARD and one for GDK_SELECTION_PRIMARY before). It also creates a DataObjectGtk store for clipboard data, which will be used for drag-and-drop data in future patches. I'd love to get feedback on this patch before preparing it for review.

[Martin Robinson]

Xan asked me to write a high-level description of this patch to make it a little easier to review:

The DOM DataTransfer object (for the GTK port this is WebCore/platform/gtk/ClipboardGtk.cpp) needs to be able to read and modify both clipboard contents and drag and drop data. Unlike OS X where both of these operations are exposed via an NSPasteboard, GTK+/GLib uses two different slices of API for drag-and-drop and clipboard access.

This patch introduces platform/gtk/DataObjectGtk.cpp, comparable to platform/chromium/ChromiumDataObject.cpp, which is a layer of indirection between ClipboardGtk and the actual GTK+/GLib APIs. When clipboard reads happen, all data is read from the clipboard and stored in the DataObject. Eventually drag data can be read into the same DataObject.

This first patch does yet not touch ClipboardGtk, but replaces the old logic for reading and writing clipboard data to use DataObjectGtk. This old logic was in two places:
1. PasteboardGtk -- used for all non-DataTransfer clipboard access.
2. EditorClientGtk -- code to handle GDK_SELECTION_PRIMARY clipboard access (activated when some text is selected).

Information about what drag/clipboard data targets has also been moved from WebKit/gtk/webkit/webkitwebview.cpp to WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.

[Martin Robinson]

Created attachment 42364 [details]
Move target list handling to PasteboardHelperGtk

This is the first patch in this series. It moves knowledge of the target list to PasteboardHelperGtk, which will eventually be responsible for managing all clipboard/pasteboard data.

[Xan Lopez]

Comment on attachment 42364 [details]
Move target list handling to PasteboardHelperGtk

This looks good to me, seems to fit nicely as a first step on the overall plan and is a good improvement by itself (IMHO) since it allows the views to share the same targelist.

I just have a couple of nitpick style comments:

targetlist in here:

+private:
+    GtkTargetList* targetList;

should be m_targetList, per style guide.

Also, I'm in doubt about the name for 'fullTargetList'. I see that in future patches you'll add new methods to get the targetlist for DataObject and GdkDragContext, but I wonder if you really need to add 'full' to the parameter-less method to avoid confusions.

Other than that, looks good to me. Marking r- at least for the style issue.

[Martin Robinson]

Created attachment 42397 [details]
Move target list handling to PasteboardHelperGtk (rev2)

Thanks for the review. I've attached a patch incorporating your suggestions.

[Jan Alonzo]

Comment on attachment 42397 [details]
Move target list handling to PasteboardHelperGtk (rev2)

Thanks for the update. Since Xan already looked, I'll say r+.

[WebKit Commit Bot]

Comment on attachment 42397 [details]
Move target list handling to PasteboardHelperGtk (rev2)

Clearing flags on attachment: 42397

Committed r50507: <http://trac.webkit.org/changeset/50507>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

Created attachment 42550 [details]
Add DataObjectGtk and use it for GDK_SELECTION_PRIMARY

This patch adds DataObjectGtk, a container used for holding pasteboard/clipboard and drag-and-drop data. Code in PasteboardHelperGtk can set the clipboard appropriately given a DataObjectGtk. It will eventually be useful for exposing this data in the DOM DataTransfer object (WebCore/platform/gtk/ClipboardGtk.cpp). The next patch in the series will handle setting data on the main pasteboard (GDK_SELECTION_CLIPBOARD) in this way as well.

[Eric Seidel (no email)]

Comment on attachment 42550 [details]
Add DataObjectGtk and use it for GDK_SELECTION_PRIMARY

I'm confused by this patch being up for review on a closed bug?  Should this bug be re-opened? or can the r=? flag on this patch be cleared?

If it should be re-opened please do so, and mark it r=? again.  For now I'm assuming the r=? should be cleared.

[Martin Robinson]

Sorry. The commit-queue closed this bug when committing the previous patch. I'm reopening it and setting the patch flag back to r=?.

[Eric Seidel (no email)]

Actually, you closed it after the commit-queue did. :)
https://bugs.webkit.org/show_activity.cgi?id=30623

Our tools are really only designed to work with one-change-per-bug.  bug 28230 is about not closing bugs with r=?, which I'll eventually get to fixing.

[Martin Robinson]

It's true. I had confusedly thought that VERIFIED / LATER meant "the bug has been verified, we'll fix it later" and would prevent the commit queue from closing it in the future.

[Holger Freyther]

Comment on attachment 42550 [details]
Add DataObjectGtk and use it for GDK_SELECTION_PRIMARY


> +        gchar* path = g_filename_from_uri(uri.utf8().data(), NULL, NULL);
> +        if (!path)
> +            continue;

^^^leaks scheme...

Use GOwnPtr here...


> +
> +        files.append(path);
> +        g_free(path);

^^^ Are you sure you want to create the String from ASCII?


> +    GdkPixbuf* m_image;
> +    GdkDragContext* m_dragContext;


Use a GOwnPtr here... this gets rid of all the if () unref if () ref things..



.. I stopped here for now.

[Martin Robinson]

Created attachment 42703 [details]
Add DataObjectGtk and use it for GDK_SELECTION_PRIMARY (rev2)

Thank you for the review. I've attached a patch addressing some of your issues you mentioned. In some places the need to call g_free(...) has been removed by making the uriList a vector of KURLs. In one other, I have switched to using GOwnPtr.

IMO, m_image and m_dragContext should be GRefPtr's instead of GOwnPtr's. If you feel like this patch needs that, I can shift my focus to finally getting that into WebKit ( https://bugs.webkit.org/show_bug.cgi?id=21599 ).

[Holger Freyther]

> IMO, m_image and m_dragContext should be GRefPtr's instead of GOwnPtr's. If you
> feel like this patch needs that, I can shift my focus to finally getting that
> into WebKit ( https://bugs.webkit.org/show_bug.cgi?id=21599 ).

Yeah, GRefPtr sounds better. I will hopefully have a review cycle tomorrow. :)

[Adam Barth]

Attachment 42703 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Done processing WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp
WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.h:50:  Missing spaces around =  [whitespace/operators] [4]
Done processing WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.h
WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.cpp:98:  Use 0 instead of NULL.  [readability/null] [5]
WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.cpp:107:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Done processing WebKit/gtk/WebCoreSupport/PasteboardHelperGtk.cpp
WebCore/platform/gtk/DataObjectGtk.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
Done processing WebCore/platform/gtk/DataObjectGtk.h
WebCore/platform/gtk/DataObjectGtk.cpp:72:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Done processing WebCore/platform/gtk/DataObjectGtk.cpp
Total errors found: 5

[Eric Seidel (no email)]

Tomorrow never came, it seems. :(

[Adam Barth]

Comment on attachment 42703 [details]
Add DataObjectGtk and use it for GDK_SELECTION_PRIMARY (rev2)

This patch has been up for review for a while with style issues.  If you're still interested in this patch, please address the style nits and repost.

[Martin Robinson]

I think I took some of Holger's comments to heart on this one and plan to upload a new version of this patch when 21599 has landed. It would be better to make members GRefPtrs.

[Martin Robinson]

Created attachment 44975 [details]
Add DataObjectGtk and use it for GDK_SELECTION_PRIMARY (rev3)

I've attached an updated patch which corrects the above style issues and uses GRefPtr (yet to land) for reference counted members. I'll add r? when GRefPtr lands.

[WebKit Review Bot]

Attachment 44975 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/gtk/DataObjectGtk.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/gtk/DataObjectGtk.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2

[Gustavo Noronha (kov)]

Comment on attachment 44975 [details]
Add DataObjectGtk and use it for GDK_SELECTION_PRIMARY (rev3)

 10         No new tests. (OOPS!)

Aren't there pasteboard tests we could enable? It looks like in this case we are not really changing the current behavior at all, right?

 28 DataObjectGtk::DataObjectGtk()
 29 {
 30 
 31 }

Empty line.

 94 /*static*/
 95 DataObjectGtk* DataObjectGtk::forClipboard(GtkClipboard* clipboard)

Why the commented out static here?

 85     if (info == WEBKIT_WEB_VIEW_TARGET_INFO_TEXT)
 86         gtk_selection_data_set_text(selectionData, dataObject->text().utf8().data(), -1);
 87 
 88     else if (info == WEBKIT_WEB_VIEW_TARGET_INFO_HTML) {

This empty line makes the if/else if blocks relationship confusing =).

 152 
 153     } else

Unnecessary empty line.

There are also a couple complaints by the style bot. The overall change in code looks pretty good to me, though. I'll r- just so you can fix these nits.

[Gustavo Noronha (kov)]

Looks like the patch makes editing/execCommand/empty-span-removal.html and platform/gtk/editing/pasteboard/middle-button-paste.html fail.

[Martin Robinson]

Created attachment 46433 [details]
Updated patch (fixes style issues and passes tests)

I've attached a patch fixing the style issues and the previously failing tests.

[WebKit Review Bot]

Attachment 46433 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/gtk/DataObjectGtk.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/gtk/DataObjectGtk.cpp:23:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/gtk/DataObjectGtk.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3

[Martin Robinson]

Created attachment 46435 [details]
Updated patch (fix new style issues)

[Xan Lopez]

Comment on attachment 46435 [details]
Updated patch (fix new style issues)

Joint r+ from kov and me on this :)

[WebKit Commit Bot]

Comment on attachment 46435 [details]
Updated patch (fix new style issues)

Clearing flags on attachment: 46435

Committed r53264: <http://trac.webkit.org/changeset/53264>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

Re-opening this as it's still a work in progress.

[Martin Robinson]

Created attachment 53774 [details]
Slight reorganization of my earlier work

I've attached a reorganization of my earlier work. This new patch moves most of the logic that was in PasteboardHelperGtk into the abstract base class, PastboardHelper. This leads to a much more natural separation of concerns between WebCore and WebKit and puts things into a better place for WebKit2.

[Gustavo Noronha (kov)]

Comment on attachment 53774 [details]
Slight reorganization of my earlier work

77 GtkClipboard* PasteboardHelper::getPrimarySelectionClipboard(Frame* frame) const
 78 {
 79         return gtk_widget_get_clipboard(widgetFromFrame(frame), GDK_SELECTION_PRIMARY);
 80 }

Indentation is off here.

I love this patch. The move to WebCore makes lots of sense, the refactoring rocks, and the usage of GClosure is very smart! We should do it elsewhere as well (like in the GeoLocation stuff). Thanks!

[Martin Robinson]

Committed r57901: <http://trac.webkit.org/changeset/57901>

[Martin Robinson]

Forgot to pass --no-close to webkit-patch, reopening. Sorry.

[Eric Seidel (no email)]

Attachment 53774 [details] was posted by a committer and has review+, assigning to Martin Robinson for commit.

[Gustavo Noronha (kov)]

Comment on attachment 53774 [details]
Slight reorganization of my earlier work

Clearing flag, since it got committed.

[Martin Robinson]

Created attachment 54159 [details]
Make the DataObject have 'live' access to the clipboard

I've added a patch that makes the DataObject access the clipboard when necessary. This is required so that web pages can modify and read the clipboard in real-time.

[Martin Robinson]

*** Bug 26712 has been marked as a duplicate of this bug. ***

[Gustavo Noronha (kov)]

Comment on attachment 54159 [details]
Make the DataObject have 'live' access to the clipboard

 8         Make ClipboardGtk a "live" DataTransfer object, able to modify
 9         modify the clipboard when setData(...) is called.

one modify too many =)

WebCore/platform/gtk/ClipboardGtk.cpp:50
 +  }
I think you should avoid breaking lines before you have the first argument in, in WebKit. It's fairly common having longer lines, so it's not an issue.

WebCore/platform/gtk/DataObjectGtk.cpp:52
 +      replaceNonBreakingSpaceWithSpace(m_text);

I believe this is a good idea, but is this behaviour specified somewhere? I have been biten at least once by non-breaking-spaces after copying/pasting from ephy =/

WebCore/platform/gtk/PasteboardHelper.cpp:101
 +      }

I am looking at these whiles and thinking they would look better as fors, given they're structured exactly as fors, but your take.

WebCore/platform/gtk/PasteboardGtk.cpp:64
 +                                 strlen(clipboardData->markup()));

This seems to be an independant bug fix, that should go in a separate commit. Please do that (r=me on it).

Hrm... I think the contextual review thing should probably add more lines for context. One line is not very useful =). Thanks for the patch, it looks right to me.

[Martin Robinson]

(In reply to comment #40)
 
> WebCore/platform/gtk/DataObjectGtk.cpp:52
>  +      replaceNonBreakingSpaceWithSpace(m_text);
> 
> I believe this is a good idea, but is this behaviour specified somewhere? I
> have been biten at least once by non-breaking-spaces after copying/pasting from
> ephy =/

In this case after seeing non-breaking spaces in pastes and drag-and-drop
data, I copied this logic from the Windows port. :)

As you suggested: 
Committed r58468: <http://trac.webkit.org/changeset/58468>
Committed r58470: <http://trac.webkit.org/changeset/58470>

[Martin Robinson]

Created attachment 55020 [details]
Convert current drag-and-drop implementation to use the new infrastructure

I've attached a patch which converts the current drag-and-drop implementation to one which uses the new DataObjectGtk infrastructure.

[WebKit Review Bot]

Attachment 55020 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/gtk/DragDataGtk.cpp:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Created attachment 55021 [details]
Convert current drag-and-drop implementation to use the new infrastructure (style violation fixed)

[Martin Robinson]

*** Bug 20588 has been marked as a duplicate of this bug. ***

[Gustavo Noronha (kov)]

Comment on attachment 55021 [details]
Convert current drag-and-drop implementation to use the new infrastructure (style violation fixed)

55  // FIXME: this should probably be something gdk-specific
56  typedef void* DragDataRef;
 55 #include "DataObjectGtk.h"
 56 typedef RefPtr<WebCore::DataObjectGtk> DragDataRef;

Hmm. This RefPtr makes me nervous. Are you positive this won't mess up with any of the uses of DragDataRef? All other platforms seem to use a normal pointer, and this type is used in constructor/method where a PassRefPtr would be normaly used.

30         * platform/gtk/PasteboardHelper.cpp:
 31         (WebCore::PasteboardHelper::fillSelectionData): Add support for images here.
 32         (WebCore::PasteboardHelper::targetListForDataObject): Add support for images here.

These make sense as a separate change; as far as I can see there's no dependency on the new code, so please land it first, r=me on this one. I believe the rest of the patch looks good, too, but I would first like to discuss this RefPtr pointer a bit before giving it r+.

[Martin Robinson]

(In reply to comment #46)
> Hmm. This RefPtr makes me nervous. Are you positive this won't mess up with any
> of the uses of DragDataRef? All other platforms seem to use a normal pointer,
> and this type is used in constructor/method where a PassRefPtr would be normaly
> used.

If I understand correctly it shouldn't be a problem here (just a little bit less performant
than using a PassRefPtr). I guess we might be okay since the WebView will control both
the lifetime of the DragData and the DataObject itself.

On looking at it further, this part really belongs in the next patch, not this one, so I'll
split this one up into three separate patches here. I should have a new patch soon.

[Martin Robinson]

Committed r58822: <http://trac.webkit.org/changeset/58822>

[Martin Robinson]

Created attachment 55131 [details]
Slimmed down version of my previous patch

This version does not include the changes to DragData.h and DragDataGtk.cpp these changes are not needed until the next patch (support for dropping).

[Gustavo Noronha (kov)]

Comment on attachment 55131 [details]
Slimmed down version of my previous patch

WebKit/gtk/webkit/webkitprivate.h:155
 +          HashMap<GdkDragContext*, RefPtr<WebCore::DataObjectGtk> > draggingDataObjects;
That space between the two >'s looks unintended? =D

WebKit/gtk/WebCoreSupport/DragClientGtk.cpp:87
 +      WEBKIT_WEB_VIEW_GET_PRIVATE(webView)->draggingDataObjects.set(context, dataObject);
I would prefer having webView->priv be used here, or a private function in WebKitWebView to do this instead of using GET_PRIVATE, because it's infamous for not being very efficient heh.

[Martin Robinson]

Thanks for the review!

> (From update of attachment 55131 [details])
> WebKit/gtk/webkit/webkitprivate.h:155
>  +          HashMap<GdkDragContext*, RefPtr<WebCore::DataObjectGtk> >
> draggingDataObjects;
> That space between the two >'s looks unintended? =D

It's actually required by some compilers to differentiate between template declarations and the '>>' operator.

> WebKit/gtk/WebCoreSupport/DragClientGtk.cpp:87
>  +      WEBKIT_WEB_VIEW_GET_PRIVATE(webView)->draggingDataObjects.set(context,
> dataObject);
> I would prefer having webView->priv be used here, or a private function in
> WebKitWebView to do this instead of using GET_PRIVATE, because it's infamous
> for not being very efficient heh.

Fixed!

[Martin Robinson]

Committed r58885: <http://trac.webkit.org/changeset/58885>

[Martin Robinson]

*** Bug 30576 has been marked as a duplicate of this bug. ***