Bug 124659 – [GTK] Support custom types for drag and drop data

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gustavo Noronha (kov)]

[GTK] Support custom types for drag and drop data

[Gustavo Noronha (kov)]

Created attachment 217436 [details]
Patch

[Gustavo Noronha (kov)]

FWIW, I did not send this to the bug mentioned in the expectation entry because that one seems to be a separate issue.

[Martin Robinson]

Comment on attachment 217436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217436&action=review

> Source/WebCore/platform/gtk/PasteboardHelper.cpp:35
> +#include <wtf/gobject/GRefPtr.h>

It would probably be slightly safer to use GRefPtrGtk here, even though it isn't necessary at the moment.

> Source/WebCore/platform/gtk/PasteboardHelper.cpp:193
> +        Vector<String> types = dataObject->unknownTypes();

It probably makes sense for the getter to return a const reference to the HashMap and to use an iterator to walk through the keys and values together. That avoids a bit of work.

> Source/WebCore/platform/gtk/PasteboardHelper.cpp:201
> +        gtk_selection_data_set(selectionData, unknownAtom, sizeof(char), reinterpret_cast<const guchar*>(serializedVariant.get()), strlen(serializedVariant.get()));

sizeof(char) -> 1

> LayoutTests/platform/gtk/TestExpectations:229
>  webkit.org/b/99068 editing/pasteboard/clipboard-customData.html [ Failure ]

Any idea why this is failing?

[Gustavo Noronha (kov)]

(In reply to comment #3)
> (From update of attachment 217436 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217436&action=review
> > LayoutTests/platform/gtk/TestExpectations:229
> >  webkit.org/b/99068 editing/pasteboard/clipboard-customData.html [ Failure ]
> 
> Any idea why this is failing?

From what I investigated Chromium was building a different implementation of pasteboard with a different API in parallel to the cross-port implementation. I think qt managed to implement some of its too. If you look at the  change that added that test, for instance, you'll see only chromium code is being touched, and the test is skipped everywhere else.  That is why this test and the data transfer item tests are skipped.

[Gustavo Noronha (kov)]

Created attachment 217881 [details]
Patch

[Martin Robinson]

Comment on attachment 217881 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=217881&action=review

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:255
> +        HashMap<String, String>::const_iterator end = types.end();
> +        for (HashMap<String, String>::const_iterator it = types.begin(); it != end; ++it)
> +            m_dataObject->setUnknownTypeData(it->key, it->value);

We are using auto in situations like this now, I think.

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:379
> +    HashMap<String, String>::const_iterator end = unknownTypes.end();
> +    for (HashMap<String, String>::const_iterator it = unknownTypes.begin(); it != end; ++it)
> +        types.append(it->key);

Ditto.

> Source/WebCore/platform/gtk/PasteboardHelper.cpp:197
> +        HashMap<String, String>::const_iterator end = types.end();
> +        for (HashMap<String, String>::const_iterator it = types.begin(); it != end; ++it) {
> +            GOwnPtr<gchar> dictItem(g_strdup_printf("{'%s', '%s'}", it->key.utf8().data(), it->value.utf8().data()));
> +            g_variant_builder_add_parsed(&builder, dictItem.get());

Ditto.

> Source/WebKit2/Shared/gtk/ArgumentCodersGtk.cpp:181
> +        HashMap<String, String>::const_iterator end = unknownTypes.end();
> +        for (HashMap<String, String>::const_iterator it = unknownTypes.begin(); it != end; ++it)

auto works here too, I think.

> LayoutTests/platform/gtk/TestExpectations:228
>  # Custom MIME type support in DataTransfer is not yet implemented.
>  webkit.org/b/99068 editing/pasteboard/clipboard-customData.html [ Failure ]

Probably should update the comment now and if the test isn't expected to pass I think it needs to move to a different section of the file.

[Gustavo Noronha (kov)]

Committed r159837: <http://trac.webkit.org/changeset/159837>