[webkit-reviews] review granted: [Bug 39843] [GTK] WebKitWebView should support drops : [Attachment 60371] Patch with Gustavo's suggestions

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 39843: [GTK] WebKitWebView should support drops
https://bugs.webkit.org/show_bug.cgi?id=39843

Attachment 60371: Patch with Gustavo's suggestions
https://bugs.webkit.org/attachment.cgi?id=60371&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
>+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