Implement basic support for Clipboard.write().
<rdar://problem/57087756>
Created attachment 383318 [details] Patch
Created attachment 383320 [details] Fix GTK/WPE
Created attachment 383324 [details] Fix macOS 10.13 build
Comment on attachment 383324 [details] Fix macOS 10.13 build Attachment 383324 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13243519 New failing tests: imported/blink/fast/events/panScroll-crash.html
Created attachment 383386 [details] Archive of layout-test-results from ews215 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews215 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment on attachment 383324 [details] Fix macOS 10.13 build View in context: https://bugs.webkit.org/attachment.cgi?id=383324&action=review > Source/WebCore/Modules/async-clipboard/Clipboard.cpp:191 > + auto shouldProceedWithClipboardWrite = [&] { It seems like this can just be a static function which takes frame as an argument instead of a lambda? > Source/WebCore/Modules/async-clipboard/Clipboard.cpp:225 > + for (size_t index = 0; index < items.size(); ++index) { > + items[index]->collectDataForWriting(*this, [index, collector = makeRef(*m_activeItemWriter)] (auto data) { > + collector->setData(WTFMove(data), index); > + }); > + } Why isn't this code in ItemWriter? Also, it's weird to call ItemWriter a collector. Let's use a consistent name (writer) here. > Source/WebCore/Modules/async-clipboard/Clipboard.cpp:246 > +void Clipboard::ItemWriter::setData(Optional<PasteboardCustomData>&& data, size_t index) Please put these code below the constructor in the order they are declared. > Source/WebCore/Modules/async-clipboard/Clipboard.cpp:255 > + if (!--m_pendingItemCount) Like the other code I reviewed, I really don't want to see a decrement statement on its own like this. This will lead to bugs. > Source/WebCore/Modules/async-clipboard/Clipboard.cpp:265 > +Clipboard::ItemWriter::ItemWriter(Clipboard& clipboard, Ref<DeferredPromise>&& promise, size_t numberOfItems) It's very awkward that this constructor the number of items an argument. I think this should just take Vector<RefPtr<ClipboardItem>> as an argument instead. > Source/WebCore/Modules/async-clipboard/Clipboard.cpp:309 > +void Clipboard::ItemWriter::resolveOrReject(ResolveOrReject resolveOrReject) This is an awkward helper function. Why don't we have resolve & reject helper functions instead? Then there is only one place where we resolve so we don't even need resolve. > LayoutTests/editing/async-clipboard/clipboard-change-data-while-writing.html:50 > + await UIHelper.activateElement(copyButton); > + await new Promise(resolve => shouldBecomeEqual("finishedWriting", "true", resolve)); > + > + copyButton.remove(); What is trigging a change to the pasteboard content while we're writing?
Comment on attachment 383324 [details] Fix macOS 10.13 build View in context: https://bugs.webkit.org/attachment.cgi?id=383324&action=review >> Source/WebCore/Modules/async-clipboard/Clipboard.cpp:191 >> + auto shouldProceedWithClipboardWrite = [&] { > > It seems like this can just be a static function which takes frame as an argument instead of a lambda? Turned this into a static function. >> Source/WebCore/Modules/async-clipboard/Clipboard.cpp:225 >> + } > > Why isn't this code in ItemWriter? > Also, it's weird to call ItemWriter a collector. Let's use a consistent name (writer) here. Good catch — it looks like I forgot to rename this local variable when I renamed ClipboardItemDataCollector to Clipboard::ItemWriter. I also moved this logic into a new helper method on ItemWriter, called write, that takes a Vector of ClipboardItems; this also allows us to make setData() private instead of public. >> Source/WebCore/Modules/async-clipboard/Clipboard.cpp:246 >> +void Clipboard::ItemWriter::setData(Optional<PasteboardCustomData>&& data, size_t index) > > Please put these code below the constructor in the order they are declared. Fixed! >> Source/WebCore/Modules/async-clipboard/Clipboard.cpp:255 >> + if (!--m_pendingItemCount) > > Like the other code I reviewed, I really don't want to see a decrement statement on its own like this. This will lead to bugs. I refactored this so the code to initialize m_pendingItemCount and decrement it is all in the same method, next to each other. >> Source/WebCore/Modules/async-clipboard/Clipboard.cpp:265 >> +Clipboard::ItemWriter::ItemWriter(Clipboard& clipboard, Ref<DeferredPromise>&& promise, size_t numberOfItems) > > It's very awkward that this constructor the number of items an argument. > I think this should just take Vector<RefPtr<ClipboardItem>> as an argument instead. I’ve removed the `numberOfItems` parameter. >> Source/WebCore/Modules/async-clipboard/Clipboard.cpp:309 >> +void Clipboard::ItemWriter::resolveOrReject(ResolveOrReject resolveOrReject) > > This is an awkward helper function. Why don't we have resolve & reject helper functions instead? > Then there is only one place where we resolve so we don't even need resolve. Good call — I refactored this so that we only have a reject() helper. I also renamed `finish()` to `didSetAllData()`, to better reflect its purpose. >> LayoutTests/editing/async-clipboard/clipboard-change-data-while-writing.html:50 >> + copyButton.remove(); > > What is trigging a change to the pasteboard content while we're writing? The call to copyText("foo”) above changes the system pasteboard.
Created attachment 383523 [details] Patch for EWS
Comment on attachment 383523 [details] Patch for EWS Clearing flags on attachment: 383523 Committed r252450: <https://trac.webkit.org/changeset/252450>