Drag and drop isn't currently supported in WebKit2 GTK+ port.
Created attachment 99283 [details] Patch
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.
Created attachment 99289 [details] Updated patch to fix coding style issues
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.
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.
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.
(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)
(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.
(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.
(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.
(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.
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.
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.
Created attachment 106978 [details] Patch using GtkWidgetDragAndDropGlue
Comment on attachment 106978 [details] Patch using GtkWidgetDragAndDropGlue Attachment 106978 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9636195
The build failed because this patch dependson 66890.
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.
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.
Created attachment 110095 [details] Patch
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.
Created attachment 110099 [details] Fix the funky licensing on DragIcon.h
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;
(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.
(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.
Committed r97012: <http://trac.webkit.org/changeset/97012>
Comment on attachment 110099 [details] Fix the funky licensing on DragIcon.h Landed with a comment. Thanks for the review!