|
Description
Martin Robinson
2009-10-21 05:31:02 PDT
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.
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. 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.
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.
Created attachment 42397 [details]
Move target list handling to PasteboardHelperGtk (rev2)
Thanks for the review. I've attached a patch incorporating your suggestions.
Comment on attachment 42397 [details]
Move target list handling to PasteboardHelperGtk (rev2)
Thanks for the update. Since Xan already looked, I'll say r+.
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> All reviewed patches have been landed. Closing bug. 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.
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.
Sorry. The commit-queue closed this bug when committing the previous patch. I'm reopening it and setting the patch flag back to r=?. 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. 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 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. 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 ).
> 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. :)
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
Tomorrow never came, it seems. :( 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.
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. 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.
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 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.
Looks like the patch makes editing/execCommand/empty-span-removal.html and platform/gtk/editing/pasteboard/middle-button-paste.html fail. 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.
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
Created attachment 46435 [details]
Updated patch (fix new style issues)
Comment on attachment 46435 [details]
Updated patch (fix new style issues)
Joint r+ from kov and me on this :)
Comment on attachment 46435 [details] Updated patch (fix new style issues) Clearing flags on attachment: 46435 Committed r53264: <http://trac.webkit.org/changeset/53264> All reviewed patches have been landed. Closing bug. Re-opening this as it's still a work in progress. 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.
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!
Committed r57901: <http://trac.webkit.org/changeset/57901> Forgot to pass --no-close to webkit-patch, reopening. Sorry. Attachment 53774 [details] was posted by a committer and has review+, assigning to Martin Robinson for commit.
Comment on attachment 53774 [details]
Slight reorganization of my earlier work
Clearing flag, since it got committed.
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.
*** Bug 26712 has been marked as a duplicate of this bug. *** 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.
(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> 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.
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.
Created attachment 55021 [details]
Convert current drag-and-drop implementation to use the new infrastructure (style violation fixed)
*** Bug 20588 has been marked as a duplicate of this bug. *** 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+.
(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. Committed r58822: <http://trac.webkit.org/changeset/58822> 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).
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.
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! Committed r58885: <http://trac.webkit.org/changeset/58885> *** Bug 30576 has been marked as a duplicate of this bug. *** |