This bug tracks the creation of a WebCore class holding drag-and-drop code which can be shared in common between WebKit and WebKit2.
Created attachment 105089 [details] Patch
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?
(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.
Created attachment 107343 [details] Patch
Comment on attachment 107343 [details] Patch Attachment 107343 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9660816
(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.
Created attachment 107454 [details] Patch
(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 on attachment 107454 [details] Patch Looks good to me. Great work!
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?
(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 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?
Committed r96299: <http://trac.webkit.org/changeset/96299>