[webkit-reviews] review granted: [Bug 203707] [Clipboard API] Add some infrastructure to resolve ClipboardItems into pasteboard data for writing : [Attachment 383138] v3

[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 203707: [Clipboard API] Add some infrastructure to resolve ClipboardItems
into pasteboard data for writing
https://bugs.webkit.org/show_bug.cgi?id=203707

Attachment 383138: v3

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




--- Comment #15 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 383138
  --> https://bugs.webkit.org/attachment.cgi?id=383138
v3

View in context: https://bugs.webkit.org/attachment.cgi?id=383138&action=review

Very nice!

>
Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.cpp:118
> +	       if (!--m_numberOfPendingClipboardTypes)

Hm... are we sure this completion handler can't be called after
m_itemTypeLoaders.clear() had ran?
Given this keeps item alive in lambda, there might be a race condition here.
Perhaps we might want to either assert or check that item is still in
m_itemTypeLoaders.

>
Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.cpp:182
> +    if (!clipboard) {

Can we add a static helper which gets RefPtr<document> so that we can
consolidate these sanity checks into one?

> Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.h:56
> +	   static Ref<ClipboardItemType> create(const String& type,
CompletionHandler<void()>&& completionHandler)

If this object takes a completion hander, I would call it
ClipboardItemTypeLoader or something.

> Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.h:89
> +    Vector<Ref<ClipboardItemType>> m_itemTypeLoaders;

Especially because this variable is called m_itemTypeLoaders.