Bug 203707

Summary: [Clipboard API] Add some infrastructure to resolve ClipboardItems into pasteboard data for writing
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: HTML EditingAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, cdumez, commit-queue, dbates, ews-watchlist, megan_gardner, rniwa, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Fix GTK/WPE builds
none
Unified source build fix
none
Another build fix in MediaStreamTrack.cpp
none
Another build fix in MediaStreamTrack.cpp
none
v2
none
FIX WPE/GTK builds
none
v3
rniwa: review+
Patch for landing
commit-queue: commit-queue-
Patch for landing none

Description Wenson Hsieh 2019-10-31 17:24:02 PDT
Work towards supporting ClipboardItem.write().
Comment 1 Wenson Hsieh 2019-10-31 21:46:00 PDT Comment hidden (obsolete)
Comment 2 Wenson Hsieh 2019-10-31 21:55:36 PDT Comment hidden (obsolete)
Comment 3 Wenson Hsieh 2019-11-01 07:20:57 PDT Comment hidden (obsolete)
Comment 4 Wenson Hsieh 2019-11-01 07:26:53 PDT Comment hidden (obsolete)
Comment 5 Wenson Hsieh 2019-11-01 07:36:33 PDT Comment hidden (obsolete)
Comment 6 Ryosuke Niwa 2019-11-05 15:28:42 PST
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.
Comment 7 Wenson Hsieh 2019-11-07 18:38:38 PST
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.
Comment 8 Wenson Hsieh 2019-11-07 18:56:25 PST Comment hidden (obsolete)
Comment 9 Wenson Hsieh 2019-11-07 19:05:11 PST Comment hidden (obsolete)
Comment 10 Ryosuke Niwa 2019-11-07 19:34:31 PST
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.
Comment 11 Wenson Hsieh 2019-11-07 19:45:00 PST
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`.
Comment 12 Ryosuke Niwa 2019-11-07 19:52:19 PST
(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...
Comment 13 Wenson Hsieh 2019-11-08 10:05:36 PST
Created attachment 383138 [details]
v3
Comment 14 Radar WebKit Bug Importer 2019-11-08 10:06:59 PST
<rdar://problem/57025262>
Comment 15 Ryosuke Niwa 2019-11-08 22:58:05 PST
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.
Comment 16 Wenson Hsieh 2019-11-10 14:24:12 PST
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.
Comment 17 Wenson Hsieh 2019-11-10 14:42:05 PST Comment hidden (obsolete)
Comment 18 WebKit Commit Bot 2019-11-10 15:13:03 PST Comment hidden (obsolete)
Comment 19 Wenson Hsieh 2019-11-10 15:33:28 PST
Created attachment 383250 [details]
Patch for landing
Comment 20 WebKit Commit Bot 2019-11-10 16:16:54 PST
Comment on attachment 383250 [details]
Patch for landing

Clearing flags on attachment: 383250

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