Bug 63706 – [GTK] Implement drag and drop support in WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Drag and drop isn't currently supported in WebKit2 GTK+ port.

[Carlos Garcia Campos]

Created attachment 99283 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 99289 [details]
Updated patch to fix coding style issues

[Carlos Garcia Campos]

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]

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 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]

(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]

(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]

(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]

(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]

(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]

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]

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]

Created attachment 106978 [details]
Patch using GtkWidgetDragAndDropGlue

[Gustavo Noronha (kov)]

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]

The build failed because this patch dependson 66890.

[Carlos Garcia Campos]

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 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]

Created attachment 110095 [details]
Patch

[Carlos Garcia Campos]

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]

Created attachment 110099 [details]
Fix the funky licensing on DragIcon.h

[Philippe Normand]

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]

(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]

(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]

Committed r97012: <http://trac.webkit.org/changeset/97012>

[Martin Robinson]

Comment on attachment 110099 [details]
Fix the funky licensing on DragIcon.h

Landed with a comment. Thanks for the review!