Bug 63706 - [GTK] Implement drag and drop support in WebKit2
Summary: [GTK] Implement drag and drop support in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords: Gtk
Depends on: 66890
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-30 06:06 PDT by Carlos Garcia Campos
Modified: 2011-10-08 10:45 PDT (History)
5 users (show)

See Also:


Attachments
Patch (42.63 KB, patch)
2011-06-30 06:16 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch to fix coding style issues (42.55 KB, patch)
2011-06-30 06:38 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Nuew patch, fix the build (42.55 KB, patch)
2011-06-30 06:44 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (42.57 KB, patch)
2011-07-15 09:21 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch using GtkWidgetDragAndDropGlue (40.82 KB, patch)
2011-09-10 12:40 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch (52.31 KB, patch)
2011-10-06 23:26 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Fix the funky licensing on DragIcon.h (52.81 KB, patch)
2011-10-07 00:25 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-06-30 06:06:52 PDT
Drag and drop isn't currently supported in WebKit2 GTK+ port.
Comment 1 Carlos Garcia Campos 2011-06-30 06:16:24 PDT
Created attachment 99283 [details]
Patch
Comment 2 WebKit Review Bot 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.
Comment 3 Carlos Garcia Campos 2011-06-30 06:38:26 PDT
Created attachment 99289 [details]
Updated patch to fix coding style issues
Comment 4 Carlos Garcia Campos 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.
Comment 5 WebKit Review Bot 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.
Comment 6 Martin Robinson 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.
Comment 7 Carlos Garcia Campos 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)
Comment 8 Martin Robinson 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.
Comment 9 Carlos Garcia Campos 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.
Comment 10 Martin Robinson 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.
Comment 11 Carlos Garcia Campos 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.
Comment 12 Carlos Garcia Campos 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.
Comment 13 WebKit Review Bot 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.
Comment 14 Martin Robinson 2011-09-10 12:40:04 PDT
Created attachment 106978 [details]
Patch using GtkWidgetDragAndDropGlue
Comment 15 Gustavo Noronha (kov) 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
Comment 16 Martin Robinson 2011-09-11 08:28:19 PDT
The build failed because this patch dependson 66890.
Comment 17 Carlos Garcia Campos 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.
Comment 18 Martin Robinson 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.
Comment 19 Martin Robinson 2011-10-06 23:26:34 PDT
Created attachment 110095 [details]
Patch
Comment 20 Carlos Garcia Campos 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.
Comment 21 Martin Robinson 2011-10-07 00:25:57 PDT
Created attachment 110099 [details]
Fix the funky licensing on DragIcon.h
Comment 22 Philippe Normand 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;
Comment 23 Carlos Garcia Campos 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.
Comment 24 Philippe Normand 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.
Comment 25 Martin Robinson 2011-10-08 10:45:01 PDT
Committed r97012: <http://trac.webkit.org/changeset/97012>
Comment 26 Martin Robinson 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!