RESOLVED FIXED 63706
[GTK] Implement drag and drop support in WebKit2
https://bugs.webkit.org/show_bug.cgi?id=63706
Summary [GTK] Implement drag and drop support in WebKit2
Carlos Garcia Campos
Reported 2011-06-30 06:06:52 PDT
Drag and drop isn't currently supported in WebKit2 GTK+ port.
Attachments
Patch (42.63 KB, patch)
2011-06-30 06:16 PDT, Carlos Garcia Campos
no flags
Updated patch to fix coding style issues (42.55 KB, patch)
2011-06-30 06:38 PDT, Carlos Garcia Campos
no flags
Nuew patch, fix the build (42.55 KB, patch)
2011-06-30 06:44 PDT, Carlos Garcia Campos
no flags
Updated patch (42.57 KB, patch)
2011-07-15 09:21 PDT, Carlos Garcia Campos
no flags
Patch using GtkWidgetDragAndDropGlue (40.82 KB, patch)
2011-09-10 12:40 PDT, Martin Robinson
no flags
Patch (52.31 KB, patch)
2011-10-06 23:26 PDT, Martin Robinson
no flags
Fix the funky licensing on DragIcon.h (52.81 KB, patch)
2011-10-07 00:25 PDT, Martin Robinson
no flags
Carlos Garcia Campos
Comment 1 2011-06-30 06:16:24 PDT
WebKit Review Bot
Comment 2 2011-06-30 06:19:07 PDT
Attachment 99283 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit2/Shared/gtk/ArgumentCodersGtk.cpp:72: Declaration has space between type name and * in cairo_surface_t *surface [whitespace/declaration] [3] Source/WebKit2/Shared/gtk/ArgumentCodersGtk.cpp:113: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Source/WebKit2/Shared/gtk/ArgumentCodersGtk.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebDragClientGtk.cpp:59: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Carlos Garcia Campos
Comment 3 2011-06-30 06:38:26 PDT
Created attachment 99289 [details] Updated patch to fix coding style issues
Carlos Garcia Campos
Comment 4 2011-06-30 06:44:21 PDT
Created attachment 99291 [details] Nuew patch, fix the build Sorry, previous patch doesn't build, check-webkit-stye suggests to use PassRefPtr, but I don't know why RefPtr is wrong in this case.
WebKit Review Bot
Comment 5 2011-06-30 06:47:13 PDT
Attachment 99291 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit2/Shared/gtk/ArgumentCodersGtk.cpp:113: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Martin Robinson
Comment 6 2011-06-30 08:46:43 PDT
Comment on attachment 99291 [details] Nuew patch, fix the build View in context: https://bugs.webkit.org/attachment.cgi?id=99291&action=review Looks good. Definitely need to use a PassRefPtr below. I'll do a full review later! >> Source/WebKit2/Shared/gtk/ArgumentCodersGtk.cpp:113 >> +static bool decodeDataObject(ArgumentDecoder* decoder, RefPtr<DataObjectGtk>& dataObject) > > The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] It's pretty weird to pass a reference to a RefPtr here. You should just use a PassRefPtr.
Carlos Garcia Campos
Comment 7 2011-06-30 08:53:02 PDT
(In reply to comment #6) > (From update of attachment 99291 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99291&action=review > > Looks good. Definitely need to use a PassRefPtr below. I'll do a full review later! Thanks! > >> Source/WebKit2/Shared/gtk/ArgumentCodersGtk.cpp:113 > >> +static bool decodeDataObject(ArgumentDecoder* decoder, RefPtr<DataObjectGtk>& dataObject) > > > > The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] > > It's pretty weird to pass a reference to a RefPtr here. You should just use a PassRefPtr. See decodeImage in Source/WebKit2/Shared/WebCoreArgumentCoders.cpp: static bool decodeImage(ArgumentDecoder* decoder, RefPtr<Image>& image)
Martin Robinson
Comment 8 2011-06-30 09:09:13 PDT
(In reply to comment #7) > > >> Source/WebKit2/Shared/gtk/ArgumentCodersGtk.cpp:113 > > >> +static bool decodeDataObject(ArgumentDecoder* decoder, RefPtr<DataObjectGtk>& dataObject) > > > > > > The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] > > > > It's pretty weird to pass a reference to a RefPtr here. You should just use a PassRefPtr. > > See decodeImage in Source/WebKit2/Shared/WebCoreArgumentCoders.cpp: > static bool decodeImage(ArgumentDecoder* decoder, RefPtr<Image>& image) WebKit as a project has recently become more strict about RefPtr/PassRefPtr usage. Perhaps there was some reason it wasn't possible in that case, but my instinct says this should just be a PassRefPtr.
Carlos Garcia Campos
Comment 9 2011-06-30 09:11:42 PDT
(In reply to comment #8) > (In reply to comment #7) > > > > >> Source/WebKit2/Shared/gtk/ArgumentCodersGtk.cpp:113 > > > >> +static bool decodeDataObject(ArgumentDecoder* decoder, RefPtr<DataObjectGtk>& dataObject) > > > > > > > > The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] > > > > > > It's pretty weird to pass a reference to a RefPtr here. You should just use a PassRefPtr. > > > > See decodeImage in Source/WebKit2/Shared/WebCoreArgumentCoders.cpp: > > static bool decodeImage(ArgumentDecoder* decoder, RefPtr<Image>& image) > > WebKit as a project has recently become more strict about RefPtr/PassRefPtr usage. Perhaps there was some reason it wasn't possible in that case, but my instinct says this should just be a PassRefPtr. Ok, I'll look at it again to try to use a PassRefPtr instead, thanks.
Martin Robinson
Comment 10 2011-06-30 09:27:54 PDT
(In reply to comment #9) > Ok, I'll look at it again to try to use a PassRefPtr instead, thanks. Apologies! You were correct originally. It seems this argument is actually an out-parameter and the return value expresses success. Please disregard my comments.
Carlos Garcia Campos
Comment 11 2011-06-30 09:35:16 PDT
(In reply to comment #10) > (In reply to comment #9) > > Ok, I'll look at it again to try to use a PassRefPtr instead, thanks. > > Apologies! You were correct originally. It seems this argument is actually an out-parameter and the return value expresses success. Please disregard my comments. No problem, I actually looked for an example before implement it, found decodeImage and copied it assuming it was correct.
Carlos Garcia Campos
Comment 12 2011-07-15 09:21:07 PDT
Created attachment 100991 [details] Updated patch Updated to apply on current git master and fixed the FIXME about sending null handles, no that the null handles patch landed.
WebKit Review Bot
Comment 13 2011-07-15 09:24:34 PDT
Attachment 100991 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebKit2/Shared/gtk/ArgumentCodersGtk.cpp:113: The parameter type should use PassRefPtr instead of RefPtr. [readability/pass_ptr] [5] Total errors found: 1 in 21 files If any of these errors are false positives, please file a bug against check-webkit-style.
Martin Robinson
Comment 14 2011-09-10 12:40:04 PDT
Created attachment 106978 [details] Patch using GtkWidgetDragAndDropGlue
Gustavo Noronha (kov)
Comment 15 2011-09-10 12:51:29 PDT
Comment on attachment 106978 [details] Patch using GtkWidgetDragAndDropGlue Attachment 106978 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9636195
Martin Robinson
Comment 16 2011-09-11 08:28:19 PDT
The build failed because this patch dependson 66890.
Carlos Garcia Campos
Comment 17 2011-09-12 00:17:40 PDT
Comment on attachment 106978 [details] Patch using GtkWidgetDragAndDropGlue View in context: https://bugs.webkit.org/attachment.cgi?id=106978&action=review Looks good to me. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:151 > + priv->dragAndDropGlue.setWidget(GTK_WIDGET(webkitWebViewBase)); Use viewWidget here too, to avoid another GTK_WIDGET cast.
Martin Robinson
Comment 18 2011-10-03 23:15:44 PDT
Comment on attachment 106978 [details] Patch using GtkWidgetDragAndDropGlue Removing review bit. I found a memory leak and the dragging performance is quite bad without the transparent window implementation from WK1. I'll reupload a patch soon hopefully with this logic shared between the two platforms.
Martin Robinson
Comment 19 2011-10-06 23:26:34 PDT
Carlos Garcia Campos
Comment 20 2011-10-06 23:43:06 PDT
Comment on attachment 110095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110095&action=review Looks good to me, great work! > Source/WebCore/platform/gtk/DragIcon.cpp:107 > + gtk_drag_set_icon_widget(context, m_window, > + hotspot.x(), hotspot.y()); I guess this could be just one line.
Martin Robinson
Comment 21 2011-10-07 00:25:57 PDT
Created attachment 110099 [details] Fix the funky licensing on DragIcon.h
Philippe Normand
Comment 22 2011-10-07 01:13:27 PDT
Comment on attachment 110099 [details] Fix the funky licensing on DragIcon.h View in context: https://bugs.webkit.org/attachment.cgi?id=110099&action=review Looks good, please fix this little early return issue and the one earlier spotted by Carlos before landing! > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebDragClientGtk.cpp:60 > + if (bitmap && !bitmap->createHandle(handle)) > + return; I think this should be: if (!bitmap || !bitmap->createHandle(handle)) return;
Carlos Garcia Campos
Comment 23 2011-10-07 01:33:44 PDT
(In reply to comment #22) > (From update of attachment 110099 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110099&action=review > > Looks good, please fix this little early return issue and the one earlier spotted by Carlos before landing! > > > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebDragClientGtk.cpp:60 > > + if (bitmap && !bitmap->createHandle(handle)) > > + return; > > I think this should be: > > if (!bitmap || !bitmap->createHandle(handle)) > return; No, it's correct. !bitmap means there's no icon, is not an error, and we want to send StartDrag with a null handle. This case is handled by the UI process that used the default icon. If there's a bitmap, but we fail to create a handle for it, it's and error and we don't want to start a drag.
Philippe Normand
Comment 24 2011-10-07 01:39:08 PDT
(In reply to comment #23) > (In reply to comment #22) > > (From update of attachment 110099 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=110099&action=review > > > > Looks good, please fix this little early return issue and the one earlier spotted by Carlos before landing! > > > > > Source/WebKit2/WebProcess/WebCoreSupport/gtk/WebDragClientGtk.cpp:60 > > > + if (bitmap && !bitmap->createHandle(handle)) > > > + return; > > > > I think this should be: > > > > if (!bitmap || !bitmap->createHandle(handle)) > > return; > > No, it's correct. !bitmap means there's no icon, is not an error, and we want to send StartDrag with a null handle. This case is handled by the UI process that used the default icon. If there's a bitmap, but we fail to create a handle for it, it's and error and we don't want to start a drag. Ah ok it makes sense now! Thanks Carlos. It'd be good I think to add a comment before the early-return.
Martin Robinson
Comment 25 2011-10-08 10:45:01 PDT
Martin Robinson
Comment 26 2011-10-08 10:45:31 PDT
Comment on attachment 110099 [details] Fix the funky licensing on DragIcon.h Landed with a comment. Thanks for the review!
Note You need to log in before you can comment on or make changes to this bug.