Bug 30623 - [GTK] Enable DOM clipboard and drag-and-drop access
: [GTK] Enable DOM clipboard and drag-and-drop access
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
:
: 21599 39240 39322 39459 39465 39843 39844 40307 40333 40788
: 30576
  Show dependency treegraph
 
Reported: 2009-10-21 05:31 PST by
Modified: 2010-08-06 09:06 PST (History)


Attachments
Draft patch for centralizing clipboard code (45.97 KB, patch)
2009-10-21 05:35 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Move target list handling to PasteboardHelperGtk (10.92 KB, patch)
2009-11-03 00:20 PST, Martin Robinson
xan.lopez: review-
Review Patch | Details | Formatted Diff | Diff
Move target list handling to PasteboardHelperGtk (rev2) (10.90 KB, patch)
2009-11-03 10:45 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Add DataObjectGtk and use it for GDK_SELECTION_PRIMARY (18.37 KB, patch)
2009-11-05 00:50 PST, Martin Robinson
zecke: review-
Review Patch | Details | Formatted Diff | Diff
Add DataObjectGtk and use it for GDK_SELECTION_PRIMARY (rev2) (17.97 KB, patch)
2009-11-07 13:43 PST, Martin Robinson
abarth: review-
Review Patch | Details | Formatted Diff | Diff
Add DataObjectGtk and use it for GDK_SELECTION_PRIMARY (rev3) (17.75 KB, patch)
2009-12-16 07:15 PST, Martin Robinson
gns: review-
gns: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Updated patch (fixes style issues and passes tests) (18.35 KB, patch)
2010-01-13 00:59 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch (fix new style issues) (18.29 KB, patch)
2010-01-13 01:18 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Slight reorganization of my earlier work (25.55 KB, patch)
2010-04-19 23:19 PST, Martin Robinson
mrobinson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Make the DataObject have 'live' access to the clipboard (26.32 KB, patch)
2010-04-23 07:59 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Convert current drag-and-drop implementation to use the new infrastructure (27.48 KB, patch)
2010-05-04 08:38 PST, Martin Robinson
mrobinson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Convert current drag-and-drop implementation to use the new infrastructure (style violation fixed) (27.48 KB, patch)
2010-05-04 08:44 PST, Martin Robinson
mrobinson: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Slimmed down version of my previous patch (20.58 KB, patch)
2010-05-05 09:43 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-21 05:31:02 PST
DOM DataTransfer objects should be able to get and set clipboard, drag source and drag destination data.
------- Comment #1 From 2009-10-21 05:35:27 PST -------
Created an attachment (id=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.
------- Comment #2 From 2009-10-23 14:43:43 PST -------
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.
------- Comment #3 From 2009-11-03 00:20:45 PST -------
Created an attachment (id=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.
------- Comment #4 From 2009-11-03 03:08:40 PST -------
(From update of attachment 42364 [details])
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.
------- Comment #5 From 2009-11-03 10:45:05 PST -------
Created an attachment (id=42397) [details]
Move target list handling to PasteboardHelperGtk (rev2)

Thanks for the review. I've attached a patch incorporating your suggestions.
------- Comment #6 From 2009-11-04 02:04:41 PST -------
(From update of attachment 42397 [details])
Thanks for the update. Since Xan already looked, I'll say r+.
------- Comment #7 From 2009-11-04 02:19:49 PST -------
(From update of attachment 42397 [details])
Clearing flags on attachment: 42397

Committed r50507: <http://trac.webkit.org/changeset/50507>
------- Comment #8 From 2009-11-04 02:19:54 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #9 From 2009-11-05 00:50:06 PST -------
Created an attachment (id=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.
------- Comment #10 From 2009-11-05 15:19:06 PST -------
(From update of attachment 42550 [details])
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.
------- Comment #11 From 2009-11-05 15:20:55 PST -------
Sorry. The commit-queue closed this bug when committing the previous patch. I'm reopening it and setting the patch flag back to r=?.
------- Comment #12 From 2009-11-05 15:36:21 PST -------
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.
------- Comment #13 From 2009-11-05 15:40:39 PST -------
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.
------- Comment #14 From 2009-11-07 01:09:47 PST -------
(From update of attachment 42550 [details])

> +        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.
------- Comment #15 From 2009-11-07 13:43:01 PST -------
Created an attachment (id=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 ).
------- Comment #16 From 2009-11-08 01:11:52 PST -------
> 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. :)
------- Comment #17 From 2009-11-30 12:23:04 PST -------
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
------- Comment #18 From 2009-12-14 13:09:16 PST -------
Tomorrow never came, it seems. :(
------- Comment #19 From 2009-12-14 15:49:34 PST -------
(From update of attachment 42703 [details])
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.
------- Comment #20 From 2009-12-14 17:35:56 PST -------
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.
------- Comment #21 From 2009-12-16 07:15:54 PST -------
Created an attachment (id=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.
------- Comment #22 From 2009-12-17 03:36:56 PST -------
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
------- Comment #23 From 2009-12-29 11:01:10 PST -------
(From update of attachment 44975 [details])
 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.
------- Comment #24 From 2010-01-04 17:21:22 PST -------
Looks like the patch makes editing/execCommand/empty-span-removal.html and platform/gtk/editing/pasteboard/middle-button-paste.html fail.
------- Comment #25 From 2010-01-13 00:59:24 PST -------
Created an attachment (id=46433) [details]
Updated patch (fixes style issues and passes tests)

I've attached a patch fixing the style issues and the previously failing tests.
------- Comment #26 From 2010-01-13 01:04:50 PST -------
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
------- Comment #27 From 2010-01-13 01:18:30 PST -------
Created an attachment (id=46435) [details]
Updated patch (fix new style issues)
------- Comment #28 From 2010-01-14 05:47:08 PST -------
(From update of attachment 46435 [details])
Joint r+ from kov and me on this :)
------- Comment #29 From 2010-01-14 06:22:00 PST -------
(From update of attachment 46435 [details])
Clearing flags on attachment: 46435

Committed r53264: <http://trac.webkit.org/changeset/53264>
------- Comment #30 From 2010-01-14 06:22:09 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #31 From 2010-04-19 23:06:44 PST -------
Re-opening this as it's still a work in progress.
------- Comment #32 From 2010-04-19 23:19:55 PST -------
Created an attachment (id=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.
------- Comment #33 From 2010-04-20 07:05:31 PST -------
(From update of attachment 53774 [details])
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!
------- Comment #34 From 2010-04-20 11:53:07 PST -------
Committed r57901: <http://trac.webkit.org/changeset/57901>
------- Comment #35 From 2010-04-20 11:54:40 PST -------
Forgot to pass --no-close to webkit-patch, reopening. Sorry.
------- Comment #36 From 2010-04-21 18:16:48 PST -------
Attachment 53774 [details] was posted by a committer and has review+, assigning to Martin Robinson for commit.
------- Comment #37 From 2010-04-22 06:42:54 PST -------
(From update of attachment 53774 [details])
Clearing flag, since it got committed.
------- Comment #38 From 2010-04-23 07:59:28 PST -------
Created an attachment (id=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.
------- Comment #39 From 2010-04-23 13:36:45 PST -------
*** Bug 26712 has been marked as a duplicate of this bug. ***
------- Comment #40 From 2010-04-26 05:32:27 PST -------
(From update of attachment 54159 [details])
 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.
------- Comment #41 From 2010-04-28 21:18:02 PST -------
(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>
------- Comment #42 From 2010-05-04 08:38:19 PST -------
Created an attachment (id=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.
------- Comment #43 From 2010-05-04 08:40:33 PST -------
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.
------- Comment #44 From 2010-05-04 08:44:45 PST -------
Created an attachment (id=55021) [details]
Convert current drag-and-drop implementation to use the new infrastructure (style violation fixed)
------- Comment #45 From 2010-05-04 16:29:50 PST -------
*** Bug 20588 has been marked as a duplicate of this bug. ***
------- Comment #46 From 2010-05-05 07:58:01 PST -------
(From update of attachment 55021 [details])
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+.
------- Comment #47 From 2010-05-05 08:30:31 PST -------
(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.
------- Comment #48 From 2010-05-05 09:31:15 PST -------
Committed r58822: <http://trac.webkit.org/changeset/58822>
------- Comment #49 From 2010-05-05 09:43:36 PST -------
Created an attachment (id=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).
------- Comment #50 From 2010-05-06 05:27:58 PST -------
(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

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.
------- Comment #51 From 2010-05-06 09:29:02 PST -------
Thanks for the review!

> (From update of attachment 55131 [details] [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!
------- Comment #52 From 2010-05-06 09:33:06 PST -------
Committed r58885: <http://trac.webkit.org/changeset/58885>
------- Comment #53 From 2010-06-17 10:56:23 PST -------
*** Bug 30576 has been marked as a duplicate of this bug. ***