Summary: | [Qt] Implement initial support to DataTransferItems | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Igor Trindade Oliveira <igor.oliveira> | ||||||||
Component: | Platform | Assignee: | 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
Igor Trindade Oliveira
2011-04-13 09:25:12 PDT
That remind me of something. Which spec is that? http://www.whatwg.org/specs/web-apps/current-work/multipage/dnd.html#the-datatransferitems-interface 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 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. Created attachment 91710 [details]
Patch
Patch
Attachment 91710 [details] did not build on qt: Build output: http://queues.webkit.org/results/8524237 Created attachment 91712 [details]
Patch
Fix build error.
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 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? (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 on attachment 91712 [details]
Patch
OK. Thanks for the update.
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 on attachment 91712 [details] Patch Clearing flags on attachment: 91712 Committed r85648: <http://trac.webkit.org/changeset/85648> All reviewed patches have been landed. Closing bug. |