Bug 203707 – [Clipboard API] Add some infrastructure to resolve ClipboardItems into pasteboard data for writing

• Component: HTML Editing
• Status: RESOLVED FIXED

[Wenson Hsieh]

Work towards supporting ClipboardItem.write().

[Wenson Hsieh]

Created attachment 382551 [details]
Patch

[Wenson Hsieh]

Created attachment 382552 [details]
Fix GTK/WPE builds

[Wenson Hsieh]

Created attachment 382576 [details]
Unified source build fix

[Wenson Hsieh]

Created attachment 382577 [details]
Another build fix in MediaStreamTrack.cpp

[Wenson Hsieh]

Created attachment 382578 [details]
Another build fix in MediaStreamTrack.cpp

[Ryosuke Niwa]

Comment on attachment 382578 [details]
Another build fix in MediaStreamTrack.cpp

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

> Source/WebCore/ChangeLog:42
> +        ClipboardItemDataCollector and ClipboardItem carry out these steps in parallel, so that for each pair of

"in parallel" is misleading because we don't have multiple threads here. Maybe asynchronously?

> Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.cpp:98
> +void ClipboardItemBindingsDataSource::collectDataForWriting(ClipboardItemDataCollector& collector)

Having dependency to ClipboardItemDataCollector is weird here.
I think this function should take a lambda which takes the result instead.

> Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.cpp:105
> +                collector->cancelDataCollection();

It's somewhat unclear why we'd have to clear other data collection.
I think it's better to have more descriptive helper function like didReceiveBadItem() or didFailToResolveItem().

> Source/WebCore/Modules/async-clipboard/ClipboardItemDataCollector.cpp:68
> +            ++m_pendingLoadResultCount;

This manual incrementing & decrementing seems super fragile.
If we made the change to make collectDataForWriting take a lambda,
we can increment & decrement the count in that single lambda.

> Source/WebCore/Modules/async-clipboard/ClipboardItemDataCollector.h:69
> +            return PtrHash<ClipboardItem*>::hash(key.item.get()) + key.type.hash();

Please use DefaultHash<>::hash(std::pair<>(key.item, key.type)).

> Source/WebCore/Modules/async-clipboard/ClipboardItemDataCollector.h:90
> +    class ClipboardItemBlobLoader : public RefCounted<ClipboardItemBlobLoader>, public FileReaderLoaderClient {

This class should be defined in the cpp file.
You only need a decoration here.

> Source/WebCore/Modules/async-clipboard/ClipboardItemDataCollector.h:102
> +        ClipboardItemBlobLoader(ClipboardItemDataCollector& collector, const LoadResultKey& loadResultKey, FileReaderLoader::ReadType readType)

Again, this loader should probably just take a lambda instead of having interdependency.

> Source/WebCore/Modules/async-clipboard/ClipboardItemDataCollector.h:124
> +        RefPtr<ClipboardItemBlobLoader> loader;

It's very strange that m_loadResults contains loader.

> Source/WebCore/Modules/async-clipboard/ClipboardItemDataCollector.h:136
> +    HashMap<LoadResultKey, LoadResult, LoadResultKeyHash, LoadResultKeyHashTrait> m_loadResults;

This map seems completely unnecessary. We can just keep accumulating results in a list
then we can iterate them all to assign all the results in takePasteboardItemsFromResults.

[Wenson Hsieh]

Comment on attachment 382578 [details]
Another build fix in MediaStreamTrack.cpp

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

>> Source/WebCore/ChangeLog:42
>> +        ClipboardItemDataCollector and ClipboardItem carry out these steps in parallel, so that for each pair of
> 
> "in parallel" is misleading because we don't have multiple threads here. Maybe asynchronously?

Good point — changed the wording accordingly.

>> Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.cpp:98
>> +void ClipboardItemBindingsDataSource::collectDataForWriting(ClipboardItemDataCollector& collector)
> 
> Having dependency to ClipboardItemDataCollector is weird here.
> I think this function should take a lambda which takes the result instead.

Refactored so that collectDataForWriting() takes completion handlers.

>> Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.cpp:105
>> +                collector->cancelDataCollection();
> 
> It's somewhat unclear why we'd have to clear other data collection.
> I think it's better to have more descriptive helper function like didReceiveBadItem() or didFailToResolveItem().

Good point — I find didFailToResolveItem() to be a better name as well.

>> Source/WebCore/Modules/async-clipboard/ClipboardItemDataCollector.cpp:68
>> +            ++m_pendingLoadResultCount;
> 
> This manual incrementing & decrementing seems super fragile.
> If we made the change to make collectDataForWriting take a lambda,
> we can increment & decrement the count in that single lambda.

Done.

>> Source/WebCore/Modules/async-clipboard/ClipboardItemDataCollector.h:69
>> +            return PtrHash<ClipboardItem*>::hash(key.item.get()) + key.type.hash();
> 
> Please use DefaultHash<>::hash(std::pair<>(key.item, key.type)).

Removed this custom hashable key altogether.

>> Source/WebCore/Modules/async-clipboard/ClipboardItemDataCollector.h:90
>> +    class ClipboardItemBlobLoader : public RefCounted<ClipboardItemBlobLoader>, public FileReaderLoaderClient {
> 
> This class should be defined in the cpp file.
> You only need a decoration here.

Removed ClipboardItemBlobLoader.

>> Source/WebCore/Modules/async-clipboard/ClipboardItemDataCollector.h:102
>> +        ClipboardItemBlobLoader(ClipboardItemDataCollector& collector, const LoadResultKey& loadResultKey, FileReaderLoader::ReadType readType)
> 
> Again, this loader should probably just take a lambda instead of having interdependency.

Removed ClipboardItemBlobLoader.

>> Source/WebCore/Modules/async-clipboard/ClipboardItemDataCollector.h:124
>> +        RefPtr<ClipboardItemBlobLoader> loader;
> 
> It's very strange that m_loadResults contains loader.

Refactored this to avoid having a separate wrapper class around the loader.

>> Source/WebCore/Modules/async-clipboard/ClipboardItemDataCollector.h:136
>> +    HashMap<LoadResultKey, LoadResult, LoadResultKeyHash, LoadResultKeyHashTrait> m_loadResults;
> 
> This map seems completely unnecessary. We can just keep accumulating results in a list
> then we can iterate them all to assign all the results in takePasteboardItemsFromResults.

Removed this map.

[Wenson Hsieh]

Created attachment 383102 [details]
v2

[Wenson Hsieh]

Created attachment 383105 [details]
FIX WPE/GTK builds

[Ryosuke Niwa]

Comment on attachment 383105 [details]
FIX WPE/GTK builds

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

> Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.cpp:224
> +    // All clipboard types have been loaded; proceed to convert the loaded data into PasteboardCustomData.

Instead of having this comment, why don't we just rename canFinishCollectingDataForWriting to didFinishLoadingAllTypes?

> Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.cpp:243
> +            // We've already extracted data from this loader.

Having a single callback which iterates over all FileReaderLoader
whenever one type finishes loading seems awfully inefficient & cumbersome.

We don't we create a struct like ClipboardItemType or something for each type,
and then have each one of those objects be a FileReaderLoaderClient.

The struct can then have a String & Ref<SharedBuffer> as members or a single variant member.

[Wenson Hsieh]

Comment on attachment 383105 [details]
FIX WPE/GTK builds

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

>> Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.cpp:224
>> +    // All clipboard types have been loaded; proceed to convert the loaded data into PasteboardCustomData.
> 
> Instead of having this comment, why don't we just rename canFinishCollectingDataForWriting to didFinishLoadingAllTypes?

The name didFinishLoadingAllTypes sounds too much like a delegate or client hook, so I think we should avoid that. How about `bool finishedLoadingAllTypes() const`.

>> Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.cpp:243
>> +            // We've already extracted data from this loader.
> 
> Having a single callback which iterates over all FileReaderLoader
> whenever one type finishes loading seems awfully inefficient & cumbersome.
> 
> We don't we create a struct like ClipboardItemType or something for each type,
> and then have each one of those objects be a FileReaderLoaderClient.
> 
> The struct can then have a String & Ref<SharedBuffer> as members or a single variant member.

This was what I did in my original patch, for this exact reason; I moved away from this to address your concerns about the resulting code being too complicated.

I'll bring it back as `ClipboardItemType`.

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #11)
> Comment on attachment 383105 [details]
> FIX WPE/GTK builds
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383105&action=review
> 
> >> Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.cpp:224
> >> +    // All clipboard types have been loaded; proceed to convert the loaded data into PasteboardCustomData.
> > 
> > Instead of having this comment, why don't we just rename canFinishCollectingDataForWriting to didFinishLoadingAllTypes?
> 
> The name didFinishLoadingAllTypes sounds too much like a delegate or client
> hook, so I think we should avoid that. How about `bool
> finishedLoadingAllTypes() const`.

Sounds good.

> >> Source/WebCore/Modules/async-clipboard/ClipboardItemBindingsDataSource.cpp:243
> >> +            // We've already extracted data from this loader.
> > 
> > Having a single callback which iterates over all FileReaderLoader
> > whenever one type finishes loading seems awfully inefficient & cumbersome.
> > 
> > We don't we create a struct like ClipboardItemType or something for each type,
> > and then have each one of those objects be a FileReaderLoaderClient.
> > 
> > The struct can then have a String & Ref<SharedBuffer> as members or a single variant member.
> 
> This was what I did in my original patch, for this exact reason; I moved
> away from this to address your concerns about the resulting code being too
> complicated.

Hm... maybe we're talking about each other. What I'm saying is that each type can have its own loader. But when it finishes loading, it doesn't need to look itself in a map, etc... Just hold onto that data, and notify ClipboardItem that it has completed loading all types. ClipboardItem can then notify Clipboard when it has gathered all types. All of this could be done via a completion handler instead of looking up a hash map & storing data in a central location, etc...

[Wenson Hsieh]

Created attachment 383138 [details]
v3

[Radar WebKit Bug Importer]

<rdar://problem/57025262>

[Ryosuke Niwa]

Comment on attachment 383138 [details]
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.

[Wenson Hsieh]

Comment on attachment 383138 [details]
v3

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

>> 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.

I /think/ this is safe, because the only thing that hangs on to the item loader is ClipboardItemBindingsDataSource (the Ref to m_item is for the ClipboardItem, rather than the ClipboardItemTypeLoader).

More assertions sounds like a good idea — I added these two:
- Assert m_numberOfPendingClipboardTypes before decrementing it.
- Assert that m_itemTypeLoaders contains the itemTypeLoader above when settling the promise.

I’ll also be writing a layout test in the next patch to exercise this scenario (i.e. calling Clipboard.write(), and then calling it again before the previous write() has finished).

>> 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?

👍🏻 Added a new helper, and used it here.

>> 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.

Sounds good — renamed to ClipboardItemTypeLoader.

[Wenson Hsieh]

Created attachment 383249 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 383249 [details]
Patch for landing

Rejecting attachment 383249 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/WebCore-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -IPAL -IForwardingHeaders -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/libxslt -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/libxml2 -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/WebKitAdditions -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/local/include/WebKitAdditions -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/webrtc -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/local/include/webrtc -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/webrtc/sdk/objc/Framework/Headers -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/local/include/webrtc/sdk/objc/Framework/Headers -I/Volumes/Data/EWS/WebKit/Source/WebCore -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -Wno-unknown-warning-option -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/PrivateFrameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/System.framework/PrivateHeaders -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/WebCorePrefix-hesoptuqnbqggaewbsnjerkudzpe/WebCorePrefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/UnifiedSource29.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/UnifiedSource29.dia -c /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource29.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/UnifiedSource29.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/UnifiedSource23.o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource23.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Failed to run "['Tools/Scripts/build-webkit', '--release']" exit_code: 65
nt-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -Wno-unknown-warning-option -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/PrivateFrameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/System.framework/PrivateHeaders -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/WebCorePrefix-hesoptuqnbqggaewbsnjerkudzpe/WebCorePrefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/UnifiedSource29.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/UnifiedSource29.dia -c /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource29.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/UnifiedSource29.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/UnifiedSource23.o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource23.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: https://webkit-queues.webkit.org/results/13236428

[Wenson Hsieh]

Created attachment 383250 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 383250 [details]
Patch for landing

Clearing flags on attachment: 383250

Committed r252315: <https://trac.webkit.org/changeset/252315>