Bug 204078 – [Clipboard API] Add support for Clipboard.write()

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

Implement basic support for Clipboard.write().

[Radar WebKit Bug Importer]

<rdar://problem/57087756>

[Wenson Hsieh]

Created attachment 383318 [details]
Patch

[Wenson Hsieh]

Created attachment 383320 [details]
Fix GTK/WPE

[Wenson Hsieh]

Created attachment 383324 [details]
Fix macOS 10.13 build

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ryosuke Niwa]

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?

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 383523 [details]
Patch for EWS

[WebKit Commit Bot]

Comment on attachment 383523 [details]
Patch for EWS

Clearing flags on attachment: 383523

Committed r252450: <https://trac.webkit.org/changeset/252450>