Bug 47244 - [GTK] editing/pasteboard/dataTransfer-setData-getData.html fails on GTK+
Summary: [GTK] editing/pasteboard/dataTransfer-setData-getData.html fails on GTK+
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-05 20:44 PDT by Martin Robinson
Modified: 2010-10-11 15:15 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 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.
Comment 1 Martin Robinson 2010-10-05 20:54:05 PDT
Committed r69174: <http://trac.webkit.org/changeset/69174>
Comment 2 James Simonsen 2010-10-05 21:44:53 PDT
Created attachment 69885 [details]
Patch
Comment 3 James Simonsen 2010-10-05 21:46:20 PDT
Sorry, my patch went to the wrong bug. Is there any way to delete that?
Comment 4 Martin Robinson 2010-10-06 11:20:31 PDT
Created attachment 69966 [details]
Patch for this issue
Comment 5 Tony Chang 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?
Comment 6 Martin Robinson 2010-10-06 12:15:25 PDT
Created attachment 69975 [details]
Patch with Tony's suggestions
Comment 7 Martin Robinson 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.
Comment 8 Tony Chang 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.
Comment 9 Martin Robinson 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>
Comment 10 Martin Robinson 2010-10-11 15:15:28 PDT
Comitted as http://trac.webkit.org/changeset/69530 .