Bug 58448 – [Qt] Implement initial support to DataTransferItems

• Component: Platform
• Status: RESOLVED FIXED

[Igor Trindade Oliveira]

DataTransferItem and DataTransferItems are used to retrieve data from drop and paste operations.

[Benjamin Poulain]

That remind me of something. Which spec is that?

[Igor Trindade Oliveira]

http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#the-datatransferitems-interface

[Igor Trindade Oliveira]

Created attachment 89640 [details]
Add initial implementation of datatransferitems

Add initial implementation of datatransferitems. It is based on chrome implementation. And right now it is not supporting images.

[Andreas Kling]

Comment on attachment 89640 [details]
Add initial implementation of datatransferitems

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

Interesting stuff. :)

There seems to be a lot of code duplication in the DataTransferItemsQt implementation, maybe we could share the common paths with DataTransferItemsChromium somehow? On a related note, when adding derived code like this, you should leave the original copyright (as well as adding your own if applicable.)

> Source/WebCore/bindings/js/JSDataTransferItemsCustom.cpp:14
> +bool JSDataTransferItems::deleteProperty(ExecState *exec, const Identifier& propertyName)
> +{
> +    return Base::deleteProperty(exec, propertyName);
> +}

This should be avoidable using a conditional in the IDL file. Though what does Chromium do in their custom deleteProperty?

> Source/WebCore/platform/qt/ClipboardQt.cpp:369
> +    RefPtr<DataTransferItemsQt> items = DataTransferItemsQt::create(this, m_frame->document()->scriptExecutionContext());

m_frame->document() is guaranteed to be non-null here?

> Source/WebCore/platform/qt/ClipboardQt.h:90
> +    Frame *m_frame;

Coding style, Frame* m_frame;

> Source/WebCore/platform/qt/DataTransferItemQt.cpp:29
> +#include <QtCore/QDebug>

Remove.

> Source/WebCore/platform/qt/DataTransferItemQt.cpp:104
> +    }

What about other MIME types?

> Source/WebCore/platform/qt/DataTransferItemsQt.cpp:29
> +#include <QtCore/QDebug>

Remove.

> Source/WebCore/platform/qt/DataTransferItemsQt.cpp:55
> +    qDebug() << m_items.size();

Remove.

> Source/WebCore/platform/qt/DataTransferItemsQt.h:55
> +    friend class ClipboardQt;

Unnecessary friend declaration.

[Igor Trindade Oliveira]

Created attachment 91710 [details]
Patch

Patch

[Early Warning System Bot]

Attachment 91710 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8524237

[Igor Trindade Oliveira]

Created attachment 91712 [details]
Patch

Fix build error.

[Eric Seidel (no email)]

Comment on attachment 91712 [details]
Patch

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

Looks reasonable.

> Source/WebCore/platform/qt/DataTransferItemQt.cpp:103
> +        QByteArray rawData = mimeData->data(type());
> +        data = QTextCodec::codecForName("UTF-16")->toUnicode(rawData);

I would have probably just written this as one line, but this is fine too.

[Eric Seidel (no email)]

Comment on attachment 91712 [details]
Patch

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

> Source/WebCore/dom/DataTransferItems.idl:38
> +#if defined(V8_BINDING) && V8_BINDING
>          CustomDeleteProperty,
> +#endif

actually... why is this needed here?  Why haven't other ports needed/wanted this idl change?

[Igor Trindade Oliveira]

(In reply to comment #9)
> (From update of attachment 91712 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91712&action=review
> 
> > Source/WebCore/dom/DataTransferItems.idl:38
> > +#if defined(V8_BINDING) && V8_BINDING
> >          CustomDeleteProperty,
> > +#endif
> 
> actually... why is this needed here?  Why haven't other ports needed/wanted this idl change?

The other ports haven't needed this idl change because just Chromium has implemented DataTransferItems support :). And DataTransferItems are not accessed outside event handler so IMHO i think we do not need to do safe guards for deletions from cross domain access.

[Eric Seidel (no email)]

Comment on attachment 91712 [details]
Patch

OK.  Thanks for the update.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 91712 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: [e-mail])
http/tests/appcache/main-resource-hash.html bug 59902 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 91712 [details]
Patch

Clearing flags on attachment: 91712

Committed r85648: <http://trac.webkit.org/changeset/85648>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.