WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
47244
[GTK] editing/pasteboard/dataTransfer-setData-getData.html fails on GTK+
https://bugs.webkit.org/show_bug.cgi?id=47244
Summary
[GTK] editing/pasteboard/dataTransfer-setData-getData.html fails on GTK+
Martin Robinson
Reported
2010-10-05 20:44:20 PDT
The issue here seems to be that setData('URL') and setData('text/uri-list') do not preserve the original string. Patch forthcoming.
Attachments
Patch
(28.80 KB, patch)
2010-10-05 21:44 PDT
,
James Simonsen
no flags
Details
Formatted Diff
Diff
Patch for this issue
(20.73 KB, patch)
2010-10-06 11:20 PDT
,
Martin Robinson
no flags
Details
Formatted Diff
Diff
Patch with Tony's suggestions
(20.77 KB, patch)
2010-10-06 12:15 PDT
,
Martin Robinson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Martin Robinson
Comment 1
2010-10-05 20:54:05 PDT
Committed
r69174
: <
http://trac.webkit.org/changeset/69174
>
James Simonsen
Comment 2
2010-10-05 21:44:53 PDT
Created
attachment 69885
[details]
Patch
James Simonsen
Comment 3
2010-10-05 21:46:20 PDT
Sorry, my patch went to the wrong bug. Is there any way to delete that?
Martin Robinson
Comment 4
2010-10-06 11:20:31 PDT
Created
attachment 69966
[details]
Patch for this issue
Tony Chang
Comment 5
2010-10-06 11:43:30 PDT
Comment on
attachment 69966
[details]
Patch for this issue View in context:
https://bugs.webkit.org/attachment.cgi?id=69966&action=review
(not changing the review flag because I don't feel qualified to r+ the gtk+ specific code)
> WebCore/platform/gtk/DataObjectGtk.cpp:66 > + // This code is originally from: platform/chromium/ChromiumDataObject.cpp.
Can you add a FIXME that says we should try to refactor this code for sharing across platforms?
> WebCore/platform/gtk/DataObjectGtk.cpp:104 > void DataObjectGtk::setURL(const KURL& url, const String& label) > { > + m_url = url;
This seems redundant since setURIList should do this for you.
> WebCore/platform/gtk/PasteboardHelper.cpp:141 > + CString uriList = dataObject->uriList().utf8(); > + gtk_selection_data_set(selectionData, uriListAtom, 8, > + reinterpret_cast<const guchar*>(uriList.data()), strlen(uriList.data()) + 1);
Would urList.length() + 1 be better than calling strlen?
Martin Robinson
Comment 6
2010-10-06 12:15:25 PDT
Created
attachment 69975
[details]
Patch with Tony's suggestions
Martin Robinson
Comment 7
2010-10-06 12:17:25 PDT
(In reply to
comment #5
) Thanks for the comments.
> > WebCore/platform/gtk/DataObjectGtk.cpp:66 > > + // This code is originally from: platform/chromium/ChromiumDataObject.cpp. > Can you add a FIXME that says we should try to refactor this code for sharing across platforms?
Done.
> > WebCore/platform/gtk/DataObjectGtk.cpp:104 > > void DataObjectGtk::setURL(const KURL& url, const String& label) > > { > > + m_url = url; > This seems redundant since setURIList should do this for you.
Instead of using setURIList here, I've switched it to simply do m_uriList = url;, to avoid any unintended side-effects (like settings files on the DataObject).
> > WebCore/platform/gtk/PasteboardHelper.cpp:141 > > + CString uriList = dataObject->uriList().utf8(); > > + gtk_selection_data_set(selectionData, uriListAtom, 8, > > + reinterpret_cast<const guchar*>(uriList.data()), strlen(uriList.data()) + 1); > Would urList.length() + 1 be better than calling strlen?
It definitely is! Fixed.
Tony Chang
Comment 8
2010-10-11 11:32:10 PDT
Comment on
attachment 69975
[details]
Patch with Tony's suggestions I still think it would be nice if someone more familiar with the GTK+ port would review.
Martin Robinson
Comment 9
2010-10-11 15:13:10 PDT
Comment on
attachment 69975
[details]
Patch with Tony's suggestions Clearing flags on attachment: 69975 Committed
r69530
: <
http://trac.webkit.org/changeset/69530
>
Martin Robinson
Comment 10
2010-10-11 15:15:28 PDT
Comitted as
http://trac.webkit.org/changeset/69530
.
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