Bug 66890 – [GTK] Move drag-and-drop code which can be shared with WebKit2 to WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

This bug tracks the creation of a WebCore class holding drag-and-drop code which can be shared in common between WebKit and WebKit2.

[Martin Robinson]

Created attachment 105089 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 105089 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105089&action=review

Patch looks good, but the name is a bit confusing to me, something called GtkWidget* makes me think it's a gtk widget. I would name it GtkDragAndDropHelper or something like that

> Source/WebCore/platform/gtk/GtkWidgetDragAndDropGlue.cpp:56
> +    DroppingContextMap::iterator endDroppingContexts = m_droppingContexts.end();
> +    for (HashMap<GdkDragContext*, DroppingContext*>::iterator iter = m_droppingContexts.begin(); iter != endDroppingContexts; ++iter)
> +        delete (iter->second);

I think you can use deleteAllPairSeconds()

> Source/WebCore/platform/gtk/GtkWidgetDragAndDropGlue.cpp:123
> +    g_timeout_add(0, reinterpret_cast<GSourceFunc>(handleDragLeaveLaterCallback), data);

Why not using an idle instead of a timeout with 0?

> Source/WebCore/platform/gtk/GtkWidgetDragAndDropGlue.h:23
> +#include <gtk/gtk.h>

Shouldn't we use forward declarations instead of including gtk.h?

[Martin Robinson]

(In reply to comment #2)
> (From update of attachment 105089 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105089&action=review
> 

Thanks for the comments.

> Patch looks good, but the name is a bit confusing to me, something called GtkWidget* makes me think it's a gtk widget. I would name it GtkDragAndDropHelper or something like that

I've changed the name to GtkDragAndDropHelper.

> > Source/WebCore/platform/gtk/GtkWidgetDragAndDropGlue.cpp:56
> > +    DroppingContextMap::iterator endDroppingContexts = m_droppingContexts.end();
> > +    for (HashMap<GdkDragContext*, DroppingContext*>::iterator iter = m_droppingContexts.begin(); iter != endDroppingContexts; ++iter)
> > +        delete (iter->second);
> I think you can use deleteAllPairSeconds()

I switched to using deleteAllValues here.

> > Source/WebCore/platform/gtk/GtkWidgetDragAndDropGlue.cpp:123
> > +    g_timeout_add(0, reinterpret_cast<GSourceFunc>(handleDragLeaveLaterCallback), data);
> Why not using an idle instead of a timeout with 0?

g_timeout_add defaults to using a higher priority. I want this to happen sooner rather than later.

> > Source/WebCore/platform/gtk/GtkWidgetDragAndDropGlue.h:23
> > +#include <gtk/gtk.h>
> Shouldn't we use forward declarations instead of including gtk.h?

Yes! Thanks for catching that. I just removed this line.

[Martin Robinson]

Created attachment 107343 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 107343 [details]
Patch

Attachment 107343 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9660816

[Martin Robinson]

(In reply to comment #5)
> (From update of attachment 107343 [details])
> Attachment 107343 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/9660816

webkit-patch left out the new file. I'll reupload soon.

[Martin Robinson]

Created attachment 107454 [details]
Patch

[Carlos Garcia Campos]

(In reply to comment #3)
> > > Source/WebCore/platform/gtk/GtkWidgetDragAndDropGlue.cpp:123
> > > +    g_timeout_add(0, reinterpret_cast<GSourceFunc>(handleDragLeaveLaterCallback), data);
> > Why not using an idle instead of a timeout with 0?
> 
> g_timeout_add defaults to using a higher priority. I want this to happen sooner rather than later.
> 

You could use g_idle_add_full() passing G_PRIORITY_DEFAULT, so that it's clear what priority it's using.

[Carlos Garcia Campos]

Comment on attachment 107454 [details]
Patch

Looks good to me. Great work!

[Alejandro G. Castro]

Comment on attachment 107454 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107454&action=review

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1488
> +static void dragExitedCallback(GtkWidget* widget, DragData* dragData, bool dropHappened)

Would it be an option to move this callback inside the helper?

[Carlos Garcia Campos]

(In reply to comment #10)
> (From update of attachment 107454 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107454&action=review
> 
> > Source/WebKit/gtk/webkit/webkitwebview.cpp:1488
> > +static void dragExitedCallback(GtkWidget* widget, DragData* dragData, bool dropHappened)
> 
> Would it be an option to move this callback inside the helper?

No, because in WebKit2 you don't have access to dragController() in the UI process, WebPageProxy is used instead to send a message to the web process.

[Philippe Normand]

Comment on attachment 107454 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107454&action=review

Nice and clean refactoring! Just a little nit to fix in the ChangeLog before landing. Thanks!

> Source/WebCore/ChangeLog:16
> +        * GNUmakefile.list.am: Added the GtkWidgetDragAndDropGlue to the sources list.
> +        * platform/gtk/GtkWidgetDragAndDropGlue.cpp: Added.
> +        * platform/gtk/GtkWidgetDragAndDropGlue.h: Added.

Was this the first name of GtkDragAndDropHelper?

[Martin Robinson]

Committed r96299: <http://trac.webkit.org/changeset/96299>