WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1
2011-06-30 06:16:24 PDT
Created
attachment 99283
[details]
Patch
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
Created
attachment 110095
[details]
Patch
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
Committed
r97012
: <
http://trac.webkit.org/changeset/97012
>
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.
Top of Page
Format For Printing
XML
Clone This Bug