[webkit-reviews] review denied: [Bug 23642] [GTK] Drag and drop support : [Attachment 29786] The clipboard, drag image and other implementations, improved
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 25 18:40:52 PDT 2009
Gustavo Noronha (kov) <gns at gnome.org> has denied Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 23642: [GTK] Drag and drop support
https://bugs.webkit.org/show_bug.cgi?id=23642
Attachment 29786: The clipboard, drag image and other implementations, improved
https://bugs.webkit.org/attachment.cgi?id=29786&action=review
------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
Do I understand it correctly that you no rely on that new clipboard
implementation on this patch?
> + m_dragClipboard->setImage(pixbuf);;
You are leaking pixbuf here, unless m_dragClipboard somehow adopts the initial
reference.
> + 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;
> +
> + GdkDragContext* context = gtk_drag_begin(GTK_WIDGET(m_webView),
> + targetList, dragAction, 1,
event);
I think you are leaking this event.
> + case WEBKIT_WEB_VIEW_TARGET_INFO_HTML:
> + data = g_strdup((clipboard->html().isEmpty() ||
clipboard->html().isNull())
> + ? clipboard->text().utf8().data() :
clipboard->html().utf8().data());
> + gtk_selection_data_set(selection_data, selection_data->target, 8,
> + reinterpret_cast<guchar*>(data),
strlen(data));
> + g_free(data);
No reason to strdup and free here, just use the pointer.
> + case WEBKIT_WEB_VIEW_TARGET_INFO_TEXT:
> + data = g_strdup(clipboard->text().utf8().data());
> + gtk_selection_data_set(selection_data, selection_data->target, 8,
> + reinterpret_cast<guchar*>(data),
strlen(data));
> + g_free(data);
Same here.
Looking at Oliver comments, I think you should clarify the usage of
DragClipboard here. My understanding is that the clipboard you are using is
separate from the one that is exposed to javascript, but I may be wrong. I'll
say r- for now, since I think you still need to update the other patch before
we move forward here.
More information about the webkit-reviews
mailing list