Bug 23642 – [GTK] Drag and drop support

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

As of this writing, the Gtk port lacks drag and drop implementation.

As a guideline, the DND support should work well with the GNOME environment and other applications.

[Zan Dobersek]

Created attachment 27180 [details]
The 'drag' implementation

This only implements the 'drag' of the 'drag and drop'.

With this, user can now click and drag images, links and text to other applications. As far as I've tested, the following (properly) works in the GNOME environment (Ubuntu 8.10):
- dragging a link to desktop (Nautilus) => creates a link,
- dragging an image to desktop => downloads the image,
- dragging links, text or HTML to Pidgin,
- dragging links or text to Firefox's search engine input and navigation input,
- dragging links to Epiphany's navigation input,
- dragging image to OpenOffice's Drawing => copies the image.

Supported targets are text targets, HTML, image targets, uri list targets and the _NETSCAPE_URL target.

Because of the lack of a proper (drag) data container in the Gtk toolkit, a small 'drag clipboard' was implemented. It is easily accessible to both WebCore and WebKit components. The GtkClipboard with GTK_SELECTION_SECONDARY could be used, but such clipboards seem to be storing only one type of data at time. Targets, however, can be very different - dragging an image can result in giving the destination either text, link or image itself.

[Zan Dobersek]

As just seen, this patch also interferes with problems of https://bugs.webkit.org/show_bug.cgi?id=20412

I recommend to first push the patch at #20412 through and then modify this patch properly.

[Gustavo Noronha (kov)]

Comment on attachment 27180 [details]
The 'drag' implementation

> +#elif PLATFORM(GTK)
> +typedef struct _GdkPixbuf GdkPixbuf;
>  #endif

Shouldn't this be struct GdkPixbuf only? It seems to me like the typedef is an implementation detail that should not be propagated to client code.

> +    GdkPixmap* pixmap = gdk_pixmap_new(NULL, width, height, 24);
[...]
> +    GdkPixbuf* pixbuf = gdk_pixbuf_get_from_drawable(NULL, GDK_DRAWABLE(pixmap), NULL, 0, 0, 0, 0, width, height);

I believe you should use 0 on those instead of NULL, since this is WebCore code.

> +typedef struct _GtkSelectionData GtkSelectionData;

Same as for GdkPixbuf?

> + void DragClipboard::resetClipboard()
> + {
> +     if (image())
> +         deleteDragImage(image());
> +     setImage(NULL);

NULL -> 0

> +        GdkPixbuf* scaledImage =  gdk_pixbuf_scale_simple(image,
> +                                                imageSize.width() * scale.width(),
> +                                                imageSize.height() * scale.height(),
> +                                                GDK_INTERP_NEAREST);

This indentation looks wrong to me. Perhaps some tabs need to be converted to spaces here, or something?

>  typedef enum
>  {
> -    WEBKIT_WEB_VIEW_TARGET_INFO_HTML = - 1,
> -    WEBKIT_WEB_VIEW_TARGET_INFO_TEXT = - 2
> +    WEBKIT_WEB_VIEW_TARGET_INFO_HTML = 1,
> +    WEBKIT_WEB_VIEW_TARGET_INFO_TEXT,
> +    WEBKIT_WEB_VIEW_TARGET_INFO_IMAGE,
> +    WEBKIT_WEB_VIEW_TARGET_INFO_URI_LIST,
> +    WEBKIT_WEB_VIEW_TARGET_INFO_NETSCAPE_URL
>  } WebKitWebViewTargetInfo;

This is weird. This probably means breaking the ABI; is this something we should worry about? Why were these negative values used?

I believe your patch does 2 very distinct things, so you should consider splitting it in two changes: one dealing with the clipboard, and another on top of that, dealing with the drag/drop thing.

[Zan Dobersek]

(In reply to comment #3)
> (From update of attachment 27180 [details] [review])
> > +#elif PLATFORM(GTK)
> > +typedef struct _GdkPixbuf GdkPixbuf;
> >  #endif
> 
> Shouldn't this be struct GdkPixbuf only? It seems to me like the typedef is an
> implementation detail that should not be propagated to client code.

This style of typedef statements is used all over the WebCore, that's why it is used in the same way here. But I have no problem with changing it if needed.

> >  typedef enum
> >  {
> > -    WEBKIT_WEB_VIEW_TARGET_INFO_HTML = - 1,
> > -    WEBKIT_WEB_VIEW_TARGET_INFO_TEXT = - 2
> > +    WEBKIT_WEB_VIEW_TARGET_INFO_HTML = 1,
> > +    WEBKIT_WEB_VIEW_TARGET_INFO_TEXT,
> > +    WEBKIT_WEB_VIEW_TARGET_INFO_IMAGE,
> > +    WEBKIT_WEB_VIEW_TARGET_INFO_URI_LIST,
> > +    WEBKIT_WEB_VIEW_TARGET_INFO_NETSCAPE_URL
> >  } WebKitWebViewTargetInfo;
> 
> This is weird. This probably means breaking the ABI; is this something we
> should worry about? Why were these negative values used?

This patch was diffed out before the proper patch in #20412 [1] was reviewed and committed into the trunk. A new patch takes that into account.

> I believe your patch does 2 very distinct things, so you should consider
> splitting it in two changes: one dealing with the clipboard, and another on top
> of that, dealing with the drag/drop thing.

The patch is now split into 3 - one dealing with GdkPixbuf conversion, one with the drag clipboard and one with drag implementations.
The latter two will be attached here, while the first will be attached to #15654 [2].

[1] https://bugs.webkit.org/show_bug.cgi?id=20412
[2] https://bugs.webkit.org/show_bug.cgi?id=15654

[Zan Dobersek]

Created attachment 28123 [details]
The drag clipboard

Implements a new container, able to hold multiple types of data, which is useful for storing drag data.

[Zan Dobersek]

Created attachment 28124 [details]
The clipboard, drag image and other implementations

Implements drag image manipulation functions, WebCore's clipboard and others.
(My god, so unwilling for detailed descriptions :)

[Xan Lopez]

(In reply to comment #5)
> Created an attachment (id=28123) [review]
> The drag clipboard
> 
> Implements a new container, able to hold multiple types of data, which is
> useful for storing drag data.
> 

+    static DragClipboard* clipboard = 0;
+
+    if (!clipboard)
+        clipboard = new DragClipboard;

can simply do:

static DragClipboard* clipboard = new DragClipboard;

[Xan Lopez]

(In reply to comment #6)
> Created an attachment (id=28124) [review]
> The clipboard, drag image and other implementations
> 
> Implements drag image manipulation functions, WebCore's clipboard and others.
> (My god, so unwilling for detailed descriptions :)
> 

-DragImageRef createDragImageFromImage(Image*)
+DragImageRef createDragImageFromImage(Image* image)
 {
-    return 0;
+    return image->getGdkPixbuf();
 }

Mmm, is this supposed to return a new reference or not? The name seems to imply 'yes', the function used 'no', although later checking the GdkPixbuf it seems it does. Maybe it should be 'createGdkPixbuf'? Maybe I should comment in the other bug... :)

+    } else if (isElementImage) {
+        gtk_target_list_add_image_targets(targetList, WEBKIT_WEB_VIEW_TARGET_INFO_IMAGE, false);
+    }

No braces here.

+    Element* targetElement = focusedFrame->document()->elementFromPoint(m_startPos.x(), m_startPos.y());
+    bool isElementLink = false;
+    bool isElementImage = targetElement->renderer()->isImage();

Can elementFromPoint() ever return NULL?

+    ((GdkEventButton*)event)->window = gtk_widget_get_window(GTK_WIDGET(m_webView));
+    ((GdkEventButton*)event)->time = GDK_CURRENT_TIME;

Better convert this to C++ castings, or better, create a GdkEventButton* variable.

+    GdkDragContext* context = gtk_drag_begin(GTK_WIDGET(m_webView),
+                                   targetList,
+                                   dragAction,
+                                   1,
+                                   event);

Indentation is broken.

+        gchar* data = g_strdup((clipboard->html().isEmpty() || clipboard->html().isNull())
+                                        ? clipboard->text().utf8().data() : clipboard->html().utf8().data());
+        gtk_selection_data_set(selection_data,
+                        selection_data->target,
+                        8, (guchar*)data, strlen(data));

More broken indentation. Also in general I fixnd the whole g{char, int, ...} stuff pretty useless, and the glib/gtk+ maintainers agree it was a mistake. I think we should just use char.

+                        8, (guchar*)data, strlen(data));
+

C casting.

Same comments about indentation and C castings and gchar apply in the other switch branches. Also I think you could declare 'char* data' before the switch and avoid the braces for new scope inside the branches.

[Holger Freyther]

Comment on attachment 28123 [details]
The drag clipboard


> +void DragClipboard::resetClipboard()
> +{
> +    if (image())
> +        deleteDragImage(image());

I didn't check the other ports... but this looks wrong.

[Zan Dobersek]

Created attachment 29738 [details]
The clipboard, drag image and other implementations, improved

Removed braces from switch cases, C++ castings now in use, the dragged image is now held where it was grabbed (used to be held at (0,0)).

Target element (on which the start of the drag was performed) can be NULL if it has no renderer (but is that even possible, starting a drag on an element with no renderer?), so we check for its existence and only then decide whether it's a link or even image that we'll be dragging.

[Xan Lopez]

Comment on attachment 29738 [details]
The clipboard, drag image and other implementations, improved

> +    CachedImage* cachedImage = getCachedImage(element);
> +    if (!cachedImage || !cachedImage->image() || !cachedImage->isLoaded())

Isn't it impossible that something returned by cachedImage fails for ->image() ?

> +        return;
> +    GdkPixbuf* pixbuf = cachedImage->image()->getGdkPixbuf();
> +    if (pixbuf)
> +        m_dragClipboard->setImage(pixbuf);;
> +
> +    m_dragClipboard->setText(url.string());
> +    m_dragClipboard->setUrl(url.string());
> +    m_dragClipboard->setUrlLabel(label);
>  }
>  
> -void ClipboardGtk::writeRange(Range*, Frame*)
> +void ClipboardGtk::writeURL(const KURL& url, const String& label, Frame*)
>  {
> -    notImplemented();
> +    if (!m_dragClipboard)
> +        return;
> +
> +    m_dragClipboard->setText(url.string());
> +    m_dragClipboard->setUrl(url.string());
> +    m_dragClipboard->setUrlLabel(label);

Make the first function call the second one perhaps?

> diff --git a/WebCore/platform/gtk/ClipboardGtk.h b/WebCore/platform/gtk/ClipboardGtk.h
> index b8b4ddf..4e7ef38 100644
> --- a/WebCore/platform/gtk/ClipboardGtk.h
> +++ b/WebCore/platform/gtk/ClipboardGtk.h
> @@ -24,13 +24,14 @@
>   * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
>  
> -#ifndef ClipboardGdk_h
> -#define ClipboardGdk_h
> +#ifndef ClipboardGtk_h
> +#define ClipboardGtk_h
>  


> -    return image;
> +    if (image) {
> +        IntSize imageSize = dragImageSize(image);
> +        GdkPixbuf* scaledImage = gdk_pixbuf_scale_simple(image, imageSize.width() * scale.width(), imageSize.height() * scale.height(), GDK_INTERP_NEAREST);

The docs recommend GDK_INTER_BILINEAR here, any reason not to use it?

> +
> +#if GTK_CHECK_VERSION(2,10,0)
> +    GdkAtom textHtml = gdk_atom_intern_static_string("text/html");
> +    GdkAtom netscapeUrl = gdk_atom_intern_static_string("_NETSCAPE_URL");
> +#else
> +    GdkAtom textHtml = gdk_atom_intern("text/html", false);
> +    GdkAtom netscapeUrl = gdk_atom_intern("_NETSCAPE_URL", false);
> +#endif

We require GTK+ 2.10 now, so you can get rid of this.

> +
> +    GdkDragContext* context = gtk_drag_begin(GTK_WIDGET(m_webView),
> +                                                targetList,
> +                                                dragAction,
> +                                                1,
> +                                                event);

Indentation?

> +static void webkit_web_view_drag_data_get(GtkWidget* widget, GdkDragContext* context, GtkSelectionData* selection_data, guint info, guint time_)
> +{
> +    DragClipboard* clipboard = DragClipboard::clipboard();
> +    char* data;
> +
> +    switch (info) {
> +    case WEBKIT_WEB_VIEW_TARGET_INFO_HTML:
> +        data = g_strdup((clipboard->html().isEmpty() || clipboard->html().isNull())
> +                                        ? clipboard->text().utf8().data() : clipboard->html().utf8().data());
> +        gtk_selection_data_set(selection_data,
> +                                selection_data->target,
> +                                8,
> +                                reinterpret_cast<guchar*>(data),
> +                                strlen(data));
> +
> +        g_free(data);
> +        return;
> +    case WEBKIT_WEB_VIEW_TARGET_INFO_TEXT:
> +        data = g_strdup(clipboard->text().utf8().data());
> +        gtk_selection_data_set(selection_data,
> +                                selection_data->target,
> +                                8,
> +                                reinterpret_cast<guchar*>(data),
> +                                strlen(data));
> +        g_free(data);
> +        return;
> +    case WEBKIT_WEB_VIEW_TARGET_INFO_IMAGE:
> +        gtk_selection_data_set_pixbuf(selection_data, clipboard->image());
> +        return;
> +    case WEBKIT_WEB_VIEW_TARGET_INFO_URI_LIST:
> +        data = g_strdup_printf("%s\r\n%s\r\n",
> +                                        clipboard->url().utf8().data(),
> +                                        (clipboard->urlLabel().isEmpty() || clipboard->urlLabel().isNull())
> +                                            ? clipboard->url().utf8().data() : clipboard->urlLabel().utf8().data());
> +        gtk_selection_data_set(selection_data,
> +                                selection_data->target,
> +                                8,
> +                                reinterpret_cast<guchar*>(data),
> +                                strlen(data));
> +
> +        g_free(data);
> +        return;
> +    case WEBKIT_WEB_VIEW_TARGET_INFO_NETSCAPE_URL:
> +        data = g_strdup_printf("%s\n%s",
> +                                        clipboard->url().utf8().data(),
> +                                        (clipboard->urlLabel().isEmpty() || clipboard->urlLabel().isNull())
> +                                            ? clipboard->url().utf8().data() : clipboard->urlLabel().utf8().data());
> +
> +        gtk_selection_data_set(selection_data,
> +                                selection_data->target,
> +                                8,
> +                                reinterpret_cast<guchar*>(data),
> +                                strlen(data));
> +
> +        g_free(data);
> +        return;
> +    }

The indentation seems to be really wrong here in some places.
> +}
> +

[Zan Dobersek]

Created attachment 29786 [details]
The clipboard, drag image and other implementations, improved

Fixes indentation problems, uses writeURL in declareAndWriteDragImage, removes #if sentences in DragClientGtk.cpp.
GDK_INTERP_NEAREST was used there mostly for speed, but BILINEAR seems a better choice indeed.

[Gustavo Noronha (kov)]

Comment on attachment 28123 [details]
The drag clipboard

> +    if (image())
> +        deleteDragImage(image());
> +    setImage(0);

This seems to be wrong indeed. You would at least need to implement the function for GTK+. I believe the best course of action is changing this, though:

#elif PLATFORM(GTK)
    typedef void* DragImageRef;
#endif

I think you should turn this into:

typedef GOwnPtr<GdkPixbuf*> DragImageRef;

Then you would not need the deleteDragImage implementation and call at all. I believe these changes could be part of this patch, since the drag clipboard implementation doesn't touch other code, it wouldn't matter in terms of bisecting. Sounds good?

[Oliver Hunt]

Comment on attachment 29786 [details]
The clipboard, drag image and other implementations, improved


> +static CachedImage* getCachedImage(Element* element)
As far as I can tell this is copied directly from DragController, it seems a better solution would be for this to be a shared function -- possibly on Element, but i'm not sure.  I'd like hyatt's input.

> -void ClipboardGtk::writeURL(const KURL&, const String&, Frame*)
> +void ClipboardGtk::declareAndWriteDragImage(Element* element, const KURL& url, const String& label, Frame*)
I was just looking at the code for this on Mac and i'm wondering if it should not be possible to move all this logic more directly into WebCore, but i don't think that matters for this particular patch

>  
> -void ClipboardGtk::writeRange(Range*, Frame*)
> +void ClipboardGtk::writeURL(const KURL& url, const String& label, Frame*)
> +void ClipboardGtk::writeRange(Range* range, Frame* frame)

I do not believe it is right to arbitrarily write to the dragging pasteboard as the Clipboard object is exposed to JS in some places where it is used (IIRC) for non-dragging related clipboard interaction.

> diff --git a/WebCore/platform/gtk/DragImageGtk.cpp b/WebCore/platform/gtk/DragImageGtk.cpp
> index 4ddae3a..a8bc7d3 100644
> --- a/WebCore/platform/gtk/DragImageGtk.cpp
> +++ b/WebCore/platform/gtk/DragImageGtk.cpp
All the changes here look good.

> diff --git a/WebKit/gtk/ChangeLog b/WebKit/gtk/ChangeLog
Unfortunately the remainder of this patch is to heavily bound in gtk API concepts for me to be able to review meaningfully

[Gustavo Noronha (kov)]

Comment on attachment 29786 [details]
The clipboard, drag image and other implementations, improved

Do I understand it correctly that you no rely on that new clipboard implementation on this patch?

> +        m_dragClipboard->setImage(pixbuf);;

You are leaking pixbuf here, unless m_dragClipboard somehow adopts the initial reference.

> +    GdkEvent* event = gdk_event_new(GDK_BUTTON_PRESS);
> +    reinterpret_cast<GdkEventButton*>(event)->window = gtk_widget_get_window(GTK_WIDGET(m_webView));
> +    reinterpret_cast<GdkEventButton*>(event)->time = GDK_CURRENT_TIME;
> +
> +    GdkDragContext* context = gtk_drag_begin(GTK_WIDGET(m_webView),
> +                                             targetList, dragAction, 1, event);

I think you are leaking this event.

> +    case WEBKIT_WEB_VIEW_TARGET_INFO_HTML:
> +        data = g_strdup((clipboard->html().isEmpty() || clipboard->html().isNull())
> +                        ? clipboard->text().utf8().data() : clipboard->html().utf8().data());
> +        gtk_selection_data_set(selection_data, selection_data->target, 8,
> +                               reinterpret_cast<guchar*>(data), strlen(data));
> +        g_free(data);

No reason to strdup and free here, just use the pointer.

> +    case WEBKIT_WEB_VIEW_TARGET_INFO_TEXT:
> +        data = g_strdup(clipboard->text().utf8().data());
> +        gtk_selection_data_set(selection_data, selection_data->target, 8,
> +                               reinterpret_cast<guchar*>(data), strlen(data));
> +        g_free(data);

Same here.

Looking at Oliver comments, I think you should clarify the usage of DragClipboard here. My understanding is that the clipboard you are using is separate from the one that is exposed to javascript, but I may be wrong. I'll say r- for now, since I think you still need to update the other patch before we move forward here.

[Zan Dobersek]

(In reply to comment #13)
> typedef GOwnPtr<GdkPixbuf*> DragImageRef;
> 

After discussion on IRC, it was decided that 'typedef GdkPixbuf* DragImageRef;' should be used for now.

(In reply to comment #15)
> Looking at Oliver comments, I think you should clarify the usage of
> DragClipboard here. My understanding is that the clipboard you are using is
> separate from the one that is exposed to javascript, but I may be wrong. I'll
> say r- for now, since I think you still need to update the other patch before
> we move forward here.
> 

DragClipboard is (for now) intended to be exclusively used for GUI dragging and should not be exposed to JavaScript.

The bigger patch will be split into more patches. The first will only deal with changes in DragImage class, second will implement ClipboardGtk class functions, third will more or less implement DragClientGtk class in WebCoreSupport part of WebKit, and the fourth will finish off with changes in webkitwebview.cpp The drag clipboard patch should be thrown between the first and second patch.

[Zan Dobersek]

Created attachment 32067 [details]
First patch

DragImageRef, drag image functions implementation.

[Gustavo Noronha (kov)]

Comment on attachment 32067 [details]
First patch

Ok, looks good.

[Zan Dobersek]

Created attachment 32089 [details]
First patch, updated

Adds additional check in deleteDragImage.

[Zan Dobersek]

Created attachment 32091 [details]
DragClipboard patch

Implements DragClipboard data holder.

[Jan Alonzo]

Comment on attachment 32089 [details]
First patch, updated

Gustavo already r+'d most of this patch so r+ with the update.

[Jan Alonzo]

Comment on attachment 32091 [details]
DragClipboard patch

Hi Zan

> +
> +namespace WebCore {
> +class String;

There needs to be an indent here.

Where is this used by the way? I'd like to see a patch that include its usage as well. I'll say r- for now until a patch that uses DragClipboard is included.

[Jan Alonzo]

Comment on attachment 32089 [details]
First patch, updated

Landed in r45414. Marking patch as obsolete and clearing commit flag.

[Jan Alonzo]

Comment on attachment 32067 [details]
First patch

Clearing r+.

[Zan Dobersek]

Created attachment 32135 [details]
Second patch

This is the second patch dissected from the WebKit/WebCore updates patch.

This one finally uses DragClipboard as the proper data holder.

One more patch is on its way, mostly taking care of things in WebKit part (WebCoreSupport updates and proper support for dnd actions in WebKitWebView). The patch should be prepared tomorrow, so the reviewer can also wait for that one and then land these patches in the same day.

[Zan Dobersek]

Created attachment 32305 [details]
Third patch

The last patch with changes in WebKit.
Implements drag_data_get and drag_end functions for WebKitWebView, WebCore::DragClient.

[Zan Dobersek]

Created attachment 32375 [details]
Third patch

Re-upload of the third patch, now with a proper review flag.

[Gustavo Noronha (kov)]

Comment on attachment 32375 [details]
Third patch

> +//    gtk_target_list_unref(targetList);
>  }

I don't like adding commented code. It seems to me like we need to keep track of both targetList and context, to make sure they go away when the drag operation ends. Should they be added to the private struct?

> +namespace WebCore {
> +    class IntPoint;
> +}

Maybe make this class WebCore::IntPoint; to avoid using more lines than necessary? =)

>  namespace WebKit {
> +

I think there are some unecesary empty line additions in the patch. Some, like the one bellow are OK, but I think the others should be removed:

>      public:
> +        DragClient(WebKitWebView*);
> +

> +        private:
> +            WebKitWebView* m_webView;
> +
> +            WebCore::IntPoint m_startPos;

Like this one.

> +static void webkit_web_view_drag_end(GtkWidget* widget, GdkDragContext* context)
> +{
> +    DragClipboard::clipboard()->resetClipboard();
> +}

This is where we want to free context and targetList, I believe.

> +    fprintf(stderr, "webkit_web_view_drag_data_get %d\n", info);

Debugging that should go away?

The rest looks fine. I think if you modify the code to not leak targetList and context we should be good to go with this one =). r- for now.

[Gustavo Noronha (kov)]

Comment on attachment 32135 [details]
Second patch

This one seems to be fine, but I'd like to commit both at the same time, indeed.

[Adam Barth]

Assigning to gns so this doesn't tempt me to land from the commit queue.

[Zan Dobersek]

(In reply to comment #28)
> (From update of attachment 32375 [details])
> > +//    gtk_target_list_unref(targetList);
> >  }
> 
> I don't like adding commented code. It seems to me like we need to keep track
> of both targetList and context, to make sure they go away when the drag
> operation ends. Should they be added to the private struct?

Yeah, the comment slashes are unneeded.
It seems, however, that the target list can be unreffed at this point.
It starts out with ref count 1 when gtk_target_list_new is called. Ref count then gets increased to 2 in gtk_drag_begin_internal[1], so the target list is certain to survive the whole dnd process.
The target list should be dereffed once by our code. At this moment, I don't think it matters whether this is done at current point or at the end of the drag.

> 
> > +static void webkit_web_view_drag_end(GtkWidget* widget, GdkDragContext* context)
> > +{
> > +    DragClipboard::clipboard()->resetClipboard();
> > +}
> 
> This is where we want to free context and targetList, I believe.

Unreffing context at this point fails, as the context doesn't seem to be a GObject. To avoid that, I've added a g_object_ref call on the context when the latter is returned from the gtk_drag_begin function in DragClient::startDrag.

> 
> > +    fprintf(stderr, "webkit_web_view_drag_data_get %d\n", info);
> 
> Debugging that should go away?

Sure.


The patch follows in a moment.

[1] http://git.gnome.org/cgit/gtk+/tree/gtk/gtkdnd.c#n2398

[Zan Dobersek]

Created attachment 33515 [details]
Third patch

Improved!
Removed some debugging leftovers, some ref counting love added.

[Zan Dobersek]

Created attachment 33516 [details]
DragClipboard patch

Fixed indent in the header file.

[Eric Seidel (no email)]

Comment on attachment 33516 [details]
DragClipboard patch

Does GTK really not have a clipboard for dragging?

storage of text/html/url/urllable is probably easier with a HashMap<String, String>.  At least if you plan to add other types in the future.

Looks OK though.

[Jan Alonzo]

(In reply to comment #32)
> Created an attachment (id=33515) [details]
> Third patch
> 
> Improved!
> Removed some debugging leftovers, some ref counting love added.

Can you please attach a manual test?

[Eric Seidel (no email)]

Comment on attachment 33515 [details]
Third patch

What are the ownership semantics of this call?
         gtk_selection_data_set(selection_data, selection_data->target, 8,
 1107                                reinterpret_cast<const guchar*>(clipboard->text().utf8().data()),
 1108                                clipboard->text().length());

I assume the clipboard makes a copy of whatever string we send to it?

Seems we could put all the string acquisition logic in the switch and follow the switch with a single gtk_selection_data_set call.

Also, seems a bit odd to use a switch if you're returning out of each condition.

Why use forcusedFrame instead of the passed in Frame?

What is clipboard used for in startDrag and why does the gtk code not use it?

Need more information in the ChangeLog about why we're deviating from the expected Platform interface by ignoring the parameters in the startDrag call.

[Zan Dobersek]

(In reply to comment #34)
> (From update of attachment 33516 [details])
> Does GTK really not have a clipboard for dragging?
> 

After having another look at the whole GTK's clipboard system, it's acutally possible to create new clipboards that then contain different formats. E.g., one clipboard carries urls, other url labels, third an image ...
This does eliminate the need for the DragClipboard.
In other words, time was being wasted not only by me when writing the patch, but also by you, reviewers.
For that, I sincerely apologize.

New patch(es) coming in near future.

Until that time I'd like to request not to commit any reviewed patches, especially the DragClipboard patch.

[Zan Dobersek]

Created attachment 34658 [details]
Second patch

This patch eliminates use of DragClipboard and now relies upon different GtkClipboard clipboards.
Also includes a small fix in the ClipboardGtk.h header.

[Zan Dobersek]

Created attachment 34659 [details]
Third patch

This patch now does not use DragClipboard, but requests contents of different clipboards. Which clipboard is requested depends on the type of drag data we need to provide.
WebKit::DragClient::startDrag now uses more of the obejcts passed to it when called - previously, we searched for focused frame and checked whether target element is a link to set drag action flags properly, now we use frame passed in by WebCore::DragController and look up to boolean value of linkDrag.

Also attached is a manual test that gives directions to test different drag situations.

Anything else left unresponded from the comments #34-#36 will be responded to in a minute.

[Zan Dobersek]

(In reply to comment #34)
> (From update of attachment 33516 [details])
> Does GTK really not have a clipboard for dragging?

There is no such specific clipboard. The patch now uses 5 different clipboards to hold the data.

(In reply to comment #35)
> Can you please attach a manual test?

Attached in the third patch.

(In reply to comment #36)
> (From update of attachment 33515 [details])
> What are the ownership semantics of this call?
>          gtk_selection_data_set(selection_data, selection_data->target, 8,
>  1107                                reinterpret_cast<const
> guchar*>(clipboard->text().utf8().data()),
>  1108                                clipboard->text().length());
> 
> I assume the clipboard makes a copy of whatever string we send to it?

In latest patch, DragClipboard is not used anymore, so I don't believe this is still relevant ...?

> Seems we could put all the string acquisition logic in the switch and follow
> the switch with a single gtk_selection_data_set call.
> 

Different kind of approach is now used, so I believe this is now out of the question.

> Also, seems a bit odd to use a switch if you're returning out of each
> condition.

Now break is used in switch statements.

> Why use forcusedFrame instead of the passed in Frame?

Fixed that. Besides passed-in frame, linkDrag boolean is now also used for setting drag actions of a link drag.

> What is clipboard used for in startDrag and why does the gtk code not use it?

The code simply doesn't need to use it at this moment.

> Need more information in the ChangeLog about why we're deviating from the
> expected Platform interface by ignoring the parameters in the startDrag call.

Enhanced that a bit.

[Gustavo Noronha (kov)]

Comment on attachment 34658 [details]
Second patch

Looks good to me. I think using separate clipboards is a good idea, indeed =). I'll leave commit-queue as ? because we want to commit this at the same time as the other patch.

[Gustavo Noronha (kov)]

Comment on attachment 34659 [details]
Third patch

> +        gchar* data;
> +        switch (contents_request->info) {
> +        case WEBKIT_WEB_VIEW_TARGET_INFO_URI_LIST:
> +            data = g_strdup_printf("%s\r\n%s\r\n", contents_request->url, url_label);
> +        case WEBKIT_WEB_VIEW_TARGET_INFO_NETSCAPE_URL:
> +            data = g_strdup_printf("%s\n%s", contents_request->url, url_label);
> +        }
> +

You are missing the breaks. Or at least one. =)

> +    switch (contents_request->info) {
> +    case WEBKIT_WEB_VIEW_TARGET_INFO_HTML:
> +    case WEBKIT_WEB_VIEW_TARGET_INFO_TEXT:
> +        {

I don't think we want a { here. I don't see a need to limit the scope, so I am not sure why you chose to do it.

I will say r=me, but we need these fixed before we commit.

[Eric Seidel (no email)]

Comment on attachment 34658 [details]
Second patch

cq- since this patch has to be committed with certain timing and the queue is not smart enough to do that.

[Zan Dobersek]

Created attachment 34795 [details]
Third patch

Adds missing breaks.

As discussed on IRC, brackets in switch statements are used because of new declarations.
While declarations could be set before the whole switch structure, I don't believe that would be transparent enough.

Thanks for the review!

[Gustavo Noronha (kov)]

Comment on attachment 34795 [details]
Third patch

ok!

[Gustavo Noronha (kov)]

Comment on attachment 34658 [details]
Second patch

updating given that the other patch is ready for committing

[Gustavo Noronha (kov)]

Comment on attachment 34659 [details]
Third patch

clearing flags, patch is obsolete

[Eric Seidel (no email)]

Comment on attachment 34658 [details]
Second patch

Clearing flags on attachment: 34658

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/gtk/ClipboardGtk.cpp
	M	WebCore/platform/gtk/ClipboardGtk.h
Committed r47241
	M	WebKit/win/WebDatabaseManager.cpp
	M	WebKit/win/Interfaces/IWebDatabaseManager.idl
	M	WebKit/win/WebDatabaseManager.h
	M	LayoutTests/platform/win/Skipped
	M	LayoutTests/fast/css/text-overflow-ellipsis-bidi.html
	M	LayoutTests/fast/css/text-overflow-ellipsis.html
	M	LayoutTests/fast/css/text-overflow-ellipsis-strict.html
	M	WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp
	M	WebKitTools/DumpRenderTree/win/UIDelegate.cpp
r47240 = edda196b6c694ce20cf8022d2db4500bf3a699f3 (trunk)
	M	WebCore/ChangeLog
	M	WebCore/platform/gtk/ClipboardGtk.cpp
	M	WebCore/platform/gtk/ClipboardGtk.h
r47241 = 26f1f11e66ef6503a8f08947b3c4504df19f29a0 (trunk)
First, rewinding head to replay your work on top of it...
Nothing to do.
http://trac.webkit.org/changeset/47241

[Eric Seidel (no email)]

Comment on attachment 34795 [details]
Third patch

Clearing flags on attachment: 34795

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKit/gtk/ChangeLog
	M	WebKit/gtk/WebCoreSupport/DragClientGtk.cpp
	M	WebKit/gtk/WebCoreSupport/DragClientGtk.h
	M	WebKit/gtk/webkit/webkitwebview.cpp
	M	WebKit/gtk/webkit/webkitwebview.h
Committed r47243
	M	JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore_debug.def
	M	JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.def
	M	JavaScriptCore/ChangeLog
r47242 = b77f6dfdb5842fe724f9ba9e18f5fa1455d08f50 (trunk)
	M	WebKit/gtk/WebCoreSupport/DragClientGtk.cpp
	M	WebKit/gtk/WebCoreSupport/DragClientGtk.h
	M	WebKit/gtk/webkit/webkitwebview.h
	M	WebKit/gtk/webkit/webkitwebview.cpp
	M	WebKit/gtk/ChangeLog
r47243 = 905cfb9100c6fe22aa3f2d7ba6833604ee93895a (trunk)
First, rewinding head to replay your work on top of it...
Nothing to do.
http://trac.webkit.org/changeset/47243

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Zan Dobersek]

Created attachment 40202 [details]
Manual test for drag source actions

Add the manual test that avoided being diffed when creating the second, WebCore patch.

[Eric Seidel (no email)]

Comment on attachment 40202 [details]
Manual test for drag source actions

Please create a new bug for new patches.  This is plenty big already (and besides, it's closed). :)