[webkit-reviews] review granted: [Bug 204078] [Clipboard API] Add support for Clipboard.write() : [Attachment 383324] Fix macOS 10.13 build

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 204078: [Clipboard API] Add support for Clipboard.write()
https://bugs.webkit.org/show_bug.cgi?id=204078

Attachment 383324: Fix macOS 10.13 build

https://bugs.webkit.org/attachment.cgi?id=383324&action=review




--- Comment #7 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 383324
  --> https://bugs.webkit.org/attachment.cgi?id=383324
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?