Bug 39843 – [GTK] WebKitWebView should support drops

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 58304 [details]
Add drop support to WebKit GTK+.

[Martin Robinson]

Created attachment 58916 [details]
Updated patch for this issue

[Gustavo Noronha (kov)]

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]

Created attachment 60371 [details]
Patch with Gustavo's suggestions

[Martin Robinson]

(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 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]

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]

Committed r63058: <http://trac.webkit.org/changeset/63058>