[webkit-reviews] review denied: [Bug 39240] [GTK] gtk_drag_begin should be initiated with a motion event : [Attachment 56273] Patch

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 39240: [GTK] gtk_drag_begin should be initiated with a motion event
https://bugs.webkit.org/show_bug.cgi?id=39240

Attachment 56273: Patch
https://bugs.webkit.org/attachment.cgi?id=56273&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
>diff --git a/WebKit/gtk/ChangeLog b/WebKit/gtk/ChangeLog
>index
e166b52a5cd7016ad0bf34adb2a69e4144a6abb8..b619c550eb61166801b588cfe5ba84aed70d3
542 100644
>--- a/WebKit/gtk/ChangeLog
>+++ b/WebKit/gtk/ChangeLog
>@@ -1,3 +1,18 @@
>+2010-05-17  Martin Robinson  <mrobinson at igalia.com>
>+
>+	  Reviewed by NOBODY (OOPS!).
>+
>+	  [GTK] gtk_drag_begin should be initiated with a motion event
>+	  https://bugs.webkit.org/show_bug.cgi?id=39240
>+
>+	  * WebCoreSupport/DragClientGtk.cpp:
>+	  (WebKit::DragClient::startDrag): Use a motion event to initiate the
drag.
>+	  * webkit/webkitprivate.cpp: Abstract this logic from
webkitwebview.cpp into a helper method.
>+	  (WebKit::clientPositionToRootWindowPosition): Added.
>+	  * webkit/webkitprivate.h: Added declaration for
WebKit::clientPositionToRootWindowPosition.
>+	  * webkit/webkitwebview.cpp: 
>+	  (webkit_web_view_popup_menu_handler): Use the new helper method.

OK, now you *are* missing the explanation in the ChangeLog :)

>-    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;
>+    GdkWindow* gdkWindow = gtk_widget_get_window(GTK_WIDGET(m_webView));
>+    IntPoint rootPosition(clientPositionToRootWindowPosition(gdkWindow,
eventPos));
>+
>+    GdkEvent* event = gdk_event_new(GDK_MOTION_NOTIFY);
>+    event->motion.window = gdkWindow;
>+    event->motion.time = GDK_CURRENT_TIME;
>+    event->motion.x_root = rootPosition.x();
>+    event->motion.y_root = rootPosition.y();
> 

Are you leaking the event and not adding a ref to the window? I remember some
patch fixing this, so maybe I'm reviewing stuff in the wrong order, sigh.