Bug 39843

Summary: [GTK] WebKitWebView should support drops
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 40307    
Bug Blocks: 30623, 39844, 40788    
Attachments:
Description Flags
Add drop support to WebKit GTK+.
none
Updated patch for this issue
none
Patch with Gustavo's suggestions xan.lopez: review+

Martin Robinson
Reported 2010-05-27 08:24:57 PDT
Currently the view only supports dragging content off. It should also support accepting drops i.e. in GTK+ parlance, serving as a drag-and-drop destination.
Attachments
Add drop support to WebKit GTK+. (22.58 KB, patch)
2010-06-09 15:56 PDT, Martin Robinson
no flags
Updated patch for this issue (24.16 KB, patch)
2010-06-16 11:55 PDT, Martin Robinson
no flags
Patch with Gustavo's suggestions (24.87 KB, patch)
2010-07-02 09:27 PDT, Martin Robinson
xan.lopez: review+
Martin Robinson
Comment 1 2010-06-09 15:56:51 PDT
Created attachment 58304 [details] Add drop support to WebKit GTK+.
Martin Robinson
Comment 2 2010-06-16 11:55:07 PDT
Created attachment 58916 [details] Updated patch for this issue
Gustavo Noronha (kov)
Comment 3 2010-07-01 12:33:18 PDT
Comment on attachment 58916 [details] Updated patch for this issue WebCore/platform/gtk/ClipboardUtilitiesGtk.cpp:42 + GdkDragAction dragOperationToGdkDragAction(DragOperation coreAction) I feel this name is way too similar to the other function. How about dragOperationToSingleGdkDragAction? WebCore/platform/gtk/ClipboardUtilitiesGtk.cpp:58 + // to use it all applicable flags are on. I failed to parse this comment. s/use it/check/? WebKit/gtk/webkit/webkitwebview.cpp:1385 + g_idle_add(reinterpret_cast<GSourceFunc>(doDragLeaveLater), priv->droppingContexts->get(context)); I learned to not trust idles. They get starved, and we lose. Can we replace this with a g_timeout_add(0...) call instead? If not, I'd prefer to have a flag that you raise to say these actions are pending, and perform them after handling drag-drop (perhaps register the timeout at that point?). r- for the above, but otherwise yay!
Martin Robinson
Comment 4 2010-07-02 09:27:54 PDT
Created attachment 60371 [details] Patch with Gustavo's suggestions
Martin Robinson
Comment 5 2010-07-02 09:29:31 PDT
(In reply to comment #3) Thanks for the review! I've implemented all of your suggestions. > + GdkDragAction dragOperationToGdkDragAction(DragOperation coreAction) > I feel this name is way too similar to the other function. How about dragOperationToSingleGdkDragAction? Done. > WebCore/platform/gtk/ClipboardUtilitiesGtk.cpp:58 > + // to use it all applicable flags are on. > I failed to parse this comment. s/use it/check/? I changed this to: to use it when all applicable flags are on. > + g_idle_add(reinterpret_cast<GSourceFunc>(doDragLeaveLater), priv->droppingContexts->get(context)); > I learned to not trust idles. They get starved, and we lose. Can > we replace this with a g_timeout_add(0...) call instead? If not, > I'd prefer to have a flag that you raise to say these actions are > pending, and perform them after handling drag-drop (perhaps > register the timeout at that point?). Makes sense. g_timeout_add(0...) seems to work fine here.
Xan Lopez
Comment 6 2010-07-11 03:41:12 PDT
Comment on attachment 60371 [details] Patch with Gustavo's suggestions >+void PasteboardHelper::fillDataObjectFromDropData(GtkSelectionData* data, guint info, DataObjectGtk* dataObject) >+{ >+ if (!data->data) >+ return; >+ >+ if (data->target == textPlainAtom) >+ dataObject->setText(selectionDataToUTF8String(data)); >+ >+ else if (data->target == markupAtom) >+ dataObject->setMarkup(selectionDataToUTF8String(data)); >+ >+ else if (data->target == uriListAtom) { >+ gchar** uris = gtk_selection_data_get_uris(data); >+ if (!uris) >+ return; >+ >+ Vector<KURL> uriList(urisToKURLVector(uris)); >+ dataObject->setURIList(uriList); >+ g_strfreev(uris); >+ I don't think the extra lines are according to the style guide. >+Vector<GdkAtom> PasteboardHelper::dropAtomsForContext(GtkWidget* widget, GdkDragContext* context) >+{ >+ // Always search for these common atoms. >+ Vector<GdkAtom> dropAtoms; >+ dropAtoms.append(textPlainAtom); >+ dropAtoms.append(markupAtom); >+ dropAtoms.append(uriListAtom); >+ dropAtoms.append(netscapeURLAtom); This could possibly be saved and be reused. >+ >+ // For images, try to find the most applicable image type. >+ GRefPtr<GtkTargetList> list(gtk_target_list_new(0, 0)); >+ gtk_target_list_add_image_targets(list.get(), getIdForTargetType(TargetTypeImage), TRUE); >+ GdkAtom atom = gtk_drag_dest_find_target(widget, context, list.get()); >+ if (atom != GDK_NONE) >+ dropAtoms.append(atom); >+ >+ return dropAtoms; >+} >+ >+ // During a drop GTK+ will fire a drag-leave signal right before firing >+ // the drag-drop signal. We want the actions for drag-leave to happen after >+ // those for drag-drop, so schedule them to happen asynchronously here. >+ g_timeout_add(0, reinterpret_cast<GSourceFunc>(doDragLeaveLater), priv->droppingContexts->get(context)); >+} idle or timeout this seems like a bit of hack... :/ The patch looks sensible as far as I understand things anyway, so if Gustavo likes it r=me
Martin Robinson
Comment 7 2010-07-11 09:48:04 PDT
Thanks for the review! > I don't think the extra lines are according to the style guide. Okay, I've removed them. :) > >+Vector<GdkAtom> PasteboardHelper::dropAtomsForContext(GtkWidget* widget, GdkDragContext* context) > >+{ > >+ // Always search for these common atoms. > >+ Vector<GdkAtom> dropAtoms; > >+ dropAtoms.append(textPlainAtom); > >+ dropAtoms.append(markupAtom); > >+ dropAtoms.append(uriListAtom); > >+ dropAtoms.append(netscapeURLAtom); > This could possibly be saved and be reused. Since the last element is dynamic I cannot think of a way to cache this value without actually copying the vector for the return value. It could possibly be two methods, but I'm not sure the speedup is worth the confusing code. I'm willing to revisit this later, if you like. > idle or timeout this seems like a bit of hack... :/ I agree it's a bit dirty. Mozilla and Chromium pull the same trick: http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsWindow.cpp#3792 http://www.google.com/codesearch/p?hl=en#h0RrPvyPu-c/chrome/browser/tab_contents/web_drag_dest_gtk.cc&q=package:%22src.chromium.org/svn/trunk%22%20drag-leave&sa=N&cd=5&ct=rc&l=205
Martin Robinson
Comment 8 2010-07-11 10:37:45 PDT
Note You need to log in before you can comment on or make changes to this bug.