Bug 66890

Summary: [GTK] Move drag-and-drop code which can be shared with WebKit2 to WebCore
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: WebKit GtkAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, gns, xan.lopez
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 Martin Robinson 2011-08-24 14:01:49 PDT
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 Martin Robinson 2011-08-24 15:56:06 PDT
Created attachment 105089 [details]
Patch
Comment 2 Carlos Garcia Campos 2011-09-12 00:14:43 PDT
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?
Comment 3 Martin Robinson 2011-09-14 09:40:45 PDT
(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.
Comment 4 Martin Robinson 2011-09-14 09:41:07 PDT
Created attachment 107343 [details]
Patch
Comment 5 Gustavo Noronha (kov) 2011-09-14 17:38:47 PDT
Comment on attachment 107343 [details]
Patch

Attachment 107343 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9660816
Comment 6 Martin Robinson 2011-09-14 17:53:15 PDT
(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.
Comment 7 Martin Robinson 2011-09-14 21:54:38 PDT
Created attachment 107454 [details]
Patch
Comment 8 Carlos Garcia Campos 2011-09-14 23:53:25 PDT
(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 Carlos Garcia Campos 2011-09-14 23:54:01 PDT
Comment on attachment 107454 [details]
Patch

Looks good to me. Great work!
Comment 10 Alejandro G. Castro 2011-09-23 06:21:45 PDT
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?
Comment 11 Carlos Garcia Campos 2011-09-23 06:27:32 PDT
(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.
Comment 12 Philippe Normand 2011-09-28 09:54:14 PDT
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?
Comment 13 Martin Robinson 2011-09-28 23:01:59 PDT
Committed r96299: <http://trac.webkit.org/changeset/96299>