[webkit-reviews] review granted: [Bug 202851] [Clipboard API] Support writing multiple PasteboardCustomData with SharedBuffers to the pasteboard : [Attachment 380850] Address review feedback

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 202851: [Clipboard API] Support writing multiple PasteboardCustomData with
SharedBuffers to the pasteboard
https://bugs.webkit.org/show_bug.cgi?id=202851

Attachment 380850: Address review feedback

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




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 380850
  --> https://bugs.webkit.org/attachment.cgi?id=380850
Address review feedback

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

> Source/WebCore/platform/PasteboardCustomData.h:38
> +struct PasteboardCustomDataEntry {

Might want this to be a nested class instead of a separate top level class.

> Source/WebCore/platform/PasteboardCustomData.h:42
> +    WEBCORE_EXPORT PasteboardCustomDataEntry& operator=(const
PasteboardCustomDataEntry& otherData);

Same thing can apply for assignment operator as for copy constructor. Might
want a move overload since that can be more efficient than copying, depending
on how this is used.

> Source/WebCore/platform/SharedBuffer.h:194
> +    WTF::Persistence::Decoder decoder() const;

It’s great to have this function available. Not so good that SharedBuffer.h now
includes PersistentCoders.h. Could we use a forward declaration instead of
including PersistentCoders.h or have this be a free function somewhere instead
of a member function?

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:301
> +	   auto *itemDictionaries = [NSMutableArray
arrayWithCapacity:itemLists.count];

For non-ARC code, I believe it’s more efficient to use alloc/init and RetainPtr
than to use a method like this one that returns an auto-released object.
Although this is slightly more elegant, and matches the idiom in some other
existing code.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:315
> +    auto *itemProviders = [NSMutableArray
arrayWithCapacity:itemLists.count];

For non-ARC code, I believe it’s more efficient to use alloc/init and RetainPtr
than to use a method like this one that returns an auto-released object.
Although this is slightly more elegant, and matches the idiom in some other
existing code.

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:603
> +    NSMutableArray *registrationLists = [NSMutableArray
arrayWithCapacity:itemData.size()];

For non-ARC code, I believe it’s more efficient to use alloc/init and RetainPtr
than to use a method like this one that returns an auto-released object.
Although this is slightly more elegant, and matches the idiom in some other
existing code.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1679
> +	   ASSERT_NOT_REACHED();

Seems strange to assert in this one place. Other decode failures are equally
worth asserting. I would omit this. Or could use a single 3-state value rather
than two booleans for hasString and hasBuffer.