[webkit-reviews] review granted: [Bug 202851] [Clipboard API] Support writing multiple PasteboardCustomData with SharedBuffers to the pasteboard : [Attachment 380770] Try to fix GTK/WPE builds

[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 380770: Try to fix GTK/WPE builds

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 380770
  --> https://bugs.webkit.org/attachment.cgi?id=380770
Try to fix GTK/WPE builds

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

Some coding style comments. A lot are not about new things.

> Source/WebCore/platform/PasteboardCustomData.cpp:48
> +PasteboardCustomData::PasteboardCustomData(const PasteboardCustomData& data)

Can’t this be "= default"?

> Source/WebCore/platform/PasteboardCustomData.cpp:59
> +    const static unsigned currentCustomDataSerializationVersion = 1;

Maybe constexpr instead?

> Source/WebCore/platform/PasteboardCustomData.cpp:71
> +    const static unsigned maxSupportedDataSerializationVersionNumber = 1;

Ditto.

> Source/WebCore/platform/PasteboardCustomData.cpp:74
> +    WTF::Persistence::Decoder decoder { reinterpret_cast<const
uint8_t*>(buffer.data()), buffer.size() };

So annoying to have the reinterpret_cast here for an idiom that is not at all
specific to pasteboards. Can we find somewhere to put an overload so this
Decoder/SharedBuffer interaction can be done cleanly?

> Source/WebCore/platform/PasteboardCustomData.cpp:86
> +    if (!decoder.decode(version) || version >
maxSupportedDataSerializationVersionNumber)
> +	   return { };
> +
> +    if (!decoder.decode(result.m_origin))
> +	   return { };
> +
> +    if (!decoder.decode(result.m_sameOriginCustomStringData))
> +	   return { };
> +
> +    if (!decoder.decode(result.m_orderedTypes))
> +	   return { };

This makes me think our decoder interface is not great. I would much prefer a
decoder that would "go numb" once there was an error, and stop doing any work
at all; subsequent calls to decode just wouldn’t do anything. Then at the end
the caller would check for the error once. Anyway, not new here.

> Source/WebCore/platform/PasteboardCustomData.cpp:97
> +static void updateTypes(Vector<String>& types, String type, bool moveToEnd)
> +{
> +    if (moveToEnd)
> +	   types.removeFirst(type);
> +    ASSERT(!types.contains(type));
> +    types.append(type);
> +}

The fact that we maintain a vector of types makes it seem wasteful that the we
also have three maps. We are going to pay the O(n^2) cost if there are a ton of
types so all the hashing in the maps is wasteful: we could instead keep all the
data in this same vector. Or if we need the O(n log n) performance then this
would need to be a ListHashSet (or whatever) instead of a Vector.

Inconvenient that the internal data structures are exposed in the getters of
this class so we can't change this around without changing a lot of call sites.

> Source/WebCore/platform/PasteboardCustomData.cpp:130
> +    if (!m_platformBinaryData.remove(type) &&
!m_platformStringData.remove(type) &&
!m_sameOriginCustomStringData.remove(type))
> +	   return;

What guarantees this type will appear in at most one of these three maps?

It looks like the functions above can put the same type in both
m_platformStringData and m_sameOriginCustomStringData, for example.

By the way, if these maps were truly mutually exclusive, then I’d suggest we
consider a single map with a more complex value type instead. Although it might
make the code more complex at various call sites.

> Source/WebCore/platform/PasteboardCustomData.h:39
> +    WEBCORE_EXPORT PasteboardCustomData(const String&, Vector<String>&&,
HashMap<String, String>&& platformStringData, HashMap<String, String>&&
customData, HashMap<String, RefPtr<SharedBuffer>>&&);

Since we're using rvalue references for everything else, why not use it for the
first argument too? Can save a tiny bit of reference count churn.

Also, I think the first argument needs a name. It’s not obvious that it’s an
origin.

> Source/WebCore/platform/PasteboardCustomData.h:40
> +    WEBCORE_EXPORT PasteboardCustomData(const PasteboardCustomData&);

Seems we should include both move and copy constructors rather than only copy,
since it can result in more efficient code for some common idioms. That’s one
thing we get "for free" if we don’t define either the move or copy constructor,
but need to do extra work to preserve. I think the struct we had before
supported move. If copy is all that’s needed at the moment, sometimes I will
delete the move constructor rather than implementing it, promising myself "I
will implement it if anyone needs it". That makes me feel better that we won’t
end up using copy-and-destroy when we could use a cheaper move.

> Source/WebCore/platform/PasteboardCustomData.h:73
> +    HashMap<String, RefPtr<SharedBuffer>> m_platformBinaryData;

Seems the value type here could be Ref<> instead of RefPtr<>.

> Source/WebCore/platform/StaticPasteboard.cpp:88
> +    return WTFMove(m_customData);

This doesn't guarantee the data is cleared. Might want to use std::exchange
instead to guarantee it’s cleared afterward (see below about changing the
return type).

> Source/WebCore/platform/StaticPasteboard.h:42
> -    PasteboardCustomData takeCustomData();
> +    PasteboardCustomData&& takeCustomData();

Seems like the old signature is better for a function that definitively does a
"take". Returning an rvalue reference is OK if it’s a function where some
callers want to call it but not actually take the data, perhaps read it but
leave it in place, but if callers are always taking then I think it’s better to
return the object, not an rvalue reference. Harder to misuse by accident.

> Source/WebCore/platform/ios/AbstractPasteboard.h:52
>  #if __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000

Is this conditional still needed? Do we support those old versions of iOS?

Also, why isn’t this an "SPI" header?

> Source/WebCore/platform/ios/PlatformPasteboardIOS.mm:747
> +    return [(id<AbstractPasteboard>)m_pasteboard.get() changeCount];

Seems peculiar that this typecast is both needed, but also guaranteed to be
safe and doesn’t require a protocol conformance check.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:870
> +    auto stagedRegistrationInfoLists =
WTFMove(_stagedRegistrationInfoLists);
> +    return stagedRegistrationInfoLists.autorelease();

I don’t understand why the WTFMove and the local variable are needed. I would
expect that this code would do the same thing:

    return _stagedRegistrationInfoLists.autorelease();

If RetainPtr::autorelease does not nil the pointer out (I think it does) then
maybe it’s std::exchange we want, not WTFMove. One sign of that is that it
would still be a one-liner:

    return std::exchange(_stagedRegistrationInfoLists, nil).autorelease();

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:484
> +	   auto platformStringIterator = data.platformStringData().find(type);
> +	   if (platformStringIterator != data.platformStringData().end()) {
> +	       [item setString:platformStringIterator->value
forType:platformType];
> +	       continue;
> +	   }

This can be done simply with HashMap::get and it might be nicer:

    auto value = data.platformStringData().get(type);
    if (!value.isNull()) {
	[item setString:value forType:platformType];
	continue;
    }

The find/end/->value dance is needed for types where copying the value out of
the map is expensive or there is no null value.

You’ll also notice that I favor shorter variable names; my philosophy is to
include adjectives only if they are needed for disambiguation or clarity.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:490
> +	   auto platformDataIterator = data.platformBinaryData().find(type);
> +	   if (platformDataIterator != data.platformBinaryData().end()) {
> +	       if (auto platformData =
platformDataIterator->value->createNSData())
> +		   [item setData:platformData.get() forType:platformType];
> +	   }

Same thing is possible here:

    if (auto buffer = data.platformBinaryData().get(type)) {
	if (auto platformData = buffer->createNSData())
	    [item setData: platformData.get() forType():platformType];
    }

I think the non-iterator code is easier to read.

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:501
> +	   if (auto item = createPasteboardItem(data))
> +	       [platformItems addObject:item.get()];

Is "silently omit" the right thing to do when createPasteboardItem returns nil?
Not obvious to me what this case is.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1645
> +    encoder << static_cast<uint64_t>(data.platformBinaryData().size());

Why is this cast needed? Seems that we could just use the appropriate type in
the decode function. I guess size_t? Annoying to have to use this idiom.