[webkit-reviews] review canceled: [Bug 203707] [Clipboard API] Add a helper class to resolve ClipboardItems into pasteboard data for writing : [Attachment 382578] Another build fix in MediaStreamTrack.cpp

[bugzilla-daemon at webkit.org]

Wenson Hsieh <wenson_hsieh at apple.com> has canceled Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 203707: [Clipboard API] Add a helper class to resolve ClipboardItems into
pasteboard data for writing
https://bugs.webkit.org/show_bug.cgi?id=203707

Attachment 382578: Another build fix in MediaStreamTrack.cpp

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




--- Comment #7 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 382578
  --> https://bugs.webkit.org/attachment.cgi?id=382578
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(ClipboardItemDataCollect
or& 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.