RESOLVED FIXED203707
[Clipboard API] Add some infrastructure to resolve ClipboardItems into pasteboard data for writing 
https://bugs.webkit.org/show_bug.cgi?id=203707
Summary [Clipboard API] Add some infrastructure to resolve ClipboardItems into pasteb...
Wenson Hsieh
Reported 2019-10-31 17:24:02 PDT
Work towards supporting ClipboardItem.write().
Attachments
Patch (33.96 KB, patch)
2019-10-31 21:46 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Fix GTK/WPE builds (34.69 KB, patch)
2019-10-31 21:55 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Unified source build fix (35.32 KB, patch)
2019-11-01 07:20 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Another build fix in MediaStreamTrack.cpp (35.34 KB, patch)
2019-11-01 07:26 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Another build fix in MediaStreamTrack.cpp (35.55 KB, patch)
2019-11-01 07:36 PDT, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
v2 (18.23 KB, patch)
2019-11-07 18:56 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
FIX WPE/GTK builds (18.37 KB, patch)
2019-11-07 19:05 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
v3 (18.55 KB, patch)
2019-11-08 10:05 PST, Wenson Hsieh 		rniwa: review+
DetailsFormatted DiffDiff
Patch for landing (19.19 KB, patch)
2019-11-10 14:42 PST, Wenson Hsieh 		commit-queue: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (19.20 KB, patch)
2019-11-10 15:33 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.
Wenson Hsieh
Comment 1 2019-10-31 21:46:00 PDT Comment hidden (obsolete)
Wenson Hsieh
Comment 2 2019-10-31 21:55:36 PDT Comment hidden (obsolete)
Wenson Hsieh
Comment 3 2019-11-01 07:20:57 PDT Comment hidden (obsolete)
Wenson Hsieh
Comment 4 2019-11-01 07:26:53 PDT Comment hidden (obsolete)
Wenson Hsieh
Comment 5 2019-11-01 07:36:33 PDT Comment hidden (obsolete)
Ryosuke Niwa
Comment 6 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.
Wenson Hsieh
Comment 7 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.
Wenson Hsieh
Comment 8 2019-11-07 18:56:25 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 9 2019-11-07 19:05:11 PST Comment hidden (obsolete)
Ryosuke Niwa
Comment 10 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.
Wenson Hsieh
Comment 11 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`.
Ryosuke Niwa
Comment 12 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...
Wenson Hsieh
Comment 13 2019-11-08 10:05:36 PST
Created attachment 383138 [details] v3
Radar WebKit Bug Importer
Comment 14 2019-11-08 10:06:59 PST
<rdar://problem/57025262>
Ryosuke Niwa
Comment 15 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.
Wenson Hsieh
Comment 16 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.
Wenson Hsieh
Comment 17 2019-11-10 14:42:05 PST Comment hidden (obsolete)
WebKit Commit Bot
Comment 18 2019-11-10 15:13:03 PST Comment hidden (obsolete)
Wenson Hsieh
Comment 19 2019-11-10 15:33:28 PST
Created attachment 383250 [details] Patch for landing
WebKit Commit Bot
Comment 20 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>
Note You need to log in before you can comment on or make changes to this bug.