Bug 58448

Summary: [Qt] Implement initial support to DataTransferItems
Product: WebKit Reporter: Igor Trindade Oliveira <igor.oliveira>
Component: PlatformAssignee: Igor Trindade Oliveira <igor.oliveira>
Status: RESOLVED FIXED    
Severity: Enhancement CC: benjamin, commit-queue, dcheng, dimich, kling, webkit-ews
Priority: P3 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 58656    
Bug Blocks:    
Attachments:
Description Flags
Add initial implementation of datatransferitems
none
Patch
none
Patch none

Description Igor Trindade Oliveira 2011-04-13 09:25:12 PDT
DataTransferItem and DataTransferItems are used to retrieve data from drop and paste operations.
Comment 1 Benjamin Poulain 2011-04-13 09:48:29 PDT
That remind me of something. Which spec is that?
Comment 3 Igor Trindade Oliveira 2011-04-14 13:58:14 PDT
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.
Comment 4 Andreas Kling 2011-04-14 14:35:47 PDT
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.
Comment 5 Igor Trindade Oliveira 2011-04-29 11:34:57 PDT
Created attachment 91710 [details]
Patch

Patch
Comment 6 Early Warning System Bot 2011-04-29 11:44:29 PDT
Attachment 91710 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8524237
Comment 7 Igor Trindade Oliveira 2011-04-29 11:57:06 PDT
Created attachment 91712 [details]
Patch

Fix build error.
Comment 8 Eric Seidel (no email) 2011-05-01 10:43:34 PDT
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.
Comment 9 Eric Seidel (no email) 2011-05-01 10:44:19 PDT
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?
Comment 10 Igor Trindade Oliveira 2011-05-02 06:59:40 PDT
(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.
Comment 11 Eric Seidel (no email) 2011-05-03 11:55:29 PDT
Comment on attachment 91712 [details]
Patch

OK.  Thanks for the update.
Comment 12 WebKit Commit Bot 2011-05-03 13:28:40 PDT
The commit-queue encountered the following flaky tests while processing attachment 91712 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: ap@webkit.org)
http/tests/appcache/main-resource-hash.html bug 59902 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2011-05-03 13:30:09 PDT
Comment on attachment 91712 [details]
Patch

Clearing flags on attachment: 91712

Committed r85648: <http://trac.webkit.org/changeset/85648>
Comment 14 WebKit Commit Bot 2011-05-03 13:30:16 PDT
All reviewed patches have been landed.  Closing bug.