[chromium] Implement glue between DataTransferItems and the pasteboard.
Created attachment 85715 [details] Patch
Attachment 85715 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:48: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:116: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 85715 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=85715&action=review > Source/WebCore/ChangeLog:5 > + Need a short description and bug URL (OOPS!) This needs to be filled out. > Source/WebCore/ChangeLog:7 > + No new tests. (OOPS!) We need tests for this change! > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:47 > PassRefPtr<DataTransferItemChromium> DataTransferItemChromium::create(PassRefPtr<Clipboard> owner, > ScriptExecutionContext* context, > + FromPasteboardType, > + const String& type) { This should all be on one line. > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:49 > + if (type == mimeTypeTextPlain || > + type == mimeTypeTextHTML) { We'd usually just put these on one line. > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:51 > + } We don't need these braces. > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:108 > + if (m_type == mimeTypeTextPlain) { > + callback->scheduleCallback(m_context, PlatformBridge::clipboardReadPlainText(PasteboardPrivate::StandardBuffer)); > + } else if (m_type == mimeTypeTextHTML) { No braces for one-line if bodies. Also, prefer early return. > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:120 > + if ((m_owner->policy() != ClipboardReadable && m_owner->policy() != ClipboardWritable) > + || m_kind != kindFile) > + return 0; > + return 0; Why are we returning zero in both cases? That seems wrong... > Source/WebKit/chromium/ChangeLog:5 > + Need a short description and bug URL (OOPS!) This needs to be filled out.
Created attachment 86254 [details] Patch
Comment on attachment 86254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=86254&action=review > Source/WebCore/ChangeLog:9 > + Support retrieving clipboard data in a paste through DataTransferItems. > + There should be a "Test: editing/pasteboard/data-transfer-items.html" line there (normally prepare-ChangeLog does that for you, but you're probably just editing the previous message). > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:112 > } Should there be a NOTREACHED here? > Source/WebCore/platform/chromium/DataTransferItemChromium.h:49 > + enum FromPasteboardType { > + FromPasteboard, > + }; I'm confused as to what this is for. > Source/WebCore/platform/chromium/PlatformBridge.h:102 > + static PassRefPtr<SharedBuffer> clipboardReadImage(PasteboardPrivate::ClipboardBuffer); Where is this implemented?
Created attachment 86347 [details] Patch
(In reply to comment #5) > (From update of attachment 86254 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=86254&action=review > > > Source/WebCore/ChangeLog:9 > > + Support retrieving clipboard data in a paste through DataTransferItems. > > + > > There should be a "Test: editing/pasteboard/data-transfer-items.html" line there (normally prepare-ChangeLog does that for you, but you're probably just editing the previous message). Done. For some reason, it's not emitting that line. I added it manually. > > > Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:112 > > } > > Should there be a NOTREACHED here? > Done. > > Source/WebCore/platform/chromium/DataTransferItemChromium.h:49 > > + enum FromPasteboardType { > > + FromPasteboard, > > + }; > > I'm confused as to what this is for. > It's just a marker to distinguish the overloads of DataTransferItem::create(). > > Source/WebCore/platform/chromium/PlatformBridge.h:102 > > + static PassRefPtr<SharedBuffer> clipboardReadImage(PasteboardPrivate::ClipboardBuffer); > > Where is this implemented? I missed reverting this change. Fixed.
Comment on attachment 86347 [details] Patch (In reply to comment #7) > (In reply to comment #5) > > (From update of attachment 86254 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=86254&action=review > > > > > Source/WebCore/ChangeLog:9 > > > + Support retrieving clipboard data in a paste through DataTransferItems. > > > + > > > > There should be a "Test: editing/pasteboard/data-transfer-items.html" line there (normally prepare-ChangeLog does that for you, but you're probably just editing the previous message). > > Done. For some reason, it's not emitting that line. I added it manually. Hmm, maybe it only adds it for new tests. Anyway, it's nice to have either way. > > > Source/WebCore/platform/chromium/DataTransferItemChromium.h:49 > > > + enum FromPasteboardType { > > > + FromPasteboard, > > > + }; > > > > I'm confused as to what this is for. > > > > It's just a marker to distinguish the overloads of DataTransferItem::create(). It seems like it would be a bit clearer to name the method createFromPasteboard. I would probably also just have a single constructor that takes DataSource as a parameter (so you don't have to keep 2 sets of constructor initializer lists in sync). View in context: https://bugs.webkit.org/attachment.cgi?id=86347&action=review > Source/WebCore/platform/chromium/DataTransferItemsChromium.h:60 > + friend class ClipboardChromium; What is this for?
Created attachment 86385 [details] Patch
(In reply to comment #8) > (From update of attachment 86347 [details]) > (In reply to comment #7) > > (In reply to comment #5) > > > (From update of attachment 86254 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=86254&action=review > > > > > > > Source/WebCore/ChangeLog:9 > > > > + Support retrieving clipboard data in a paste through DataTransferItems. > > > > + > > > > > > There should be a "Test: editing/pasteboard/data-transfer-items.html" line there (normally prepare-ChangeLog does that for you, but you're probably just editing the previous message). > > > > Done. For some reason, it's not emitting that line. I added it manually. > > Hmm, maybe it only adds it for new tests. Anyway, it's nice to have either way. > > > > > Source/WebCore/platform/chromium/DataTransferItemChromium.h:49 > > > > + enum FromPasteboardType { > > > > + FromPasteboard, > > > > + }; > > > > > > I'm confused as to what this is for. > > > > > > > It's just a marker to distinguish the overloads of DataTransferItem::create(). > > It seems like it would be a bit clearer to name the method createFromPasteboard. I would probably also just have a single constructor that takes DataSource as a parameter (so you don't have to keep 2 sets of constructor initializer lists in sync). > > Done. > View in context: https://bugs.webkit.org/attachment.cgi?id=86347&action=review > > > Source/WebCore/platform/chromium/DataTransferItemsChromium.h:60 > > + friend class ClipboardChromium; > > What is this for? addPasteboardItem is only exposed to ClipboardChromium.
Committed r81620: <http://trac.webkit.org/changeset/81620>
The new test is failing on the Chrome bots: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=editing%2Fpasteboard%2Fdata-transfer-items.html