Bug 66890

Summary: [GTK] Move drag-and-drop code which can be shared with WebKit2 to WebCore
Product: WebKit Reporter: Martin Robinson <mrobinson@webkit.org>
Component: WebKit GtkAssignee: Martin Robinson <mrobinson@webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia@igalia.com, gns@gnome.org, xan.lopez@gmail.com
Priority: P3 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 63706    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description From 2011-08-24 14:01:49 PST
This bug tracks the creation of a WebCore class holding drag-and-drop code which can be shared in common between WebKit and WebKit2.
------- Comment #1 From 2011-08-24 15:56:06 PST -------
Created an attachment (id=105089) [details]
Patch
------- Comment #2 From 2011-09-12 00:14:43 PST -------
(From update of attachment 105089 [details])
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?
------- Comment #3 From 2011-09-14 09:40:45 PST -------
(In reply to comment #2)
> (From update of attachment 105089 [details] [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.
------- Comment #4 From 2011-09-14 09:41:07 PST -------
Created an attachment (id=107343) [details]
Patch
------- Comment #5 From 2011-09-14 17:38:47 PST -------
(From update of attachment 107343 [details])
Attachment 107343 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9660816
------- Comment #6 From 2011-09-14 17:53:15 PST -------
(In reply to comment #5)
> (From update of attachment 107343 [details] [details])
> Attachment 107343 [details] [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.
------- Comment #7 From 2011-09-14 21:54:38 PST -------
Created an attachment (id=107454) [details]
Patch
------- Comment #8 From 2011-09-14 23:53:25 PST -------
(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.
------- Comment #9 From 2011-09-14 23:54:01 PST -------
(From update of attachment 107454 [details])
Looks good to me. Great work!
------- Comment #10 From 2011-09-23 06:21:45 PST -------
(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?
------- Comment #11 From 2011-09-23 06:27:32 PST -------
(In reply to comment #10)
> (From update of attachment 107454 [details] [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.
------- Comment #12 From 2011-09-28 09:54:14 PST -------
(From update of attachment 107454 [details])
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?
------- Comment #13 From 2011-09-28 23:01:59 PST -------
Committed r96299: <http://trac.webkit.org/changeset/96299>