[webkit-reviews] review granted: [Bug 226123] Add `createCFArray` just like `createNSArray` : [Attachment 429385] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 226123: Add `createCFArray` just like `createNSArray`
https://bugs.webkit.org/show_bug.cgi?id=226123

Attachment 429385: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 429385
  --> https://bugs.webkit.org/attachment.cgi?id=429385
Patch

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

Looks pretty good, but may need some thought and improvement. In general we
have a very high standard for these "refactoring" changes -- they really have
to earn their right to be done in a way that changes with an immediate
functional benefit do not. In the name of making things better, they can easily
make them subtly worse.

> Source/WTF/wtf/cf/LanguageCF.cpp:121
> +	       if (auto cfString = dynamic_cf_cast<CFStringRef>(item))
> +		   return httpStyleLanguageCode(cfString);

I suggest naming the local variable "string", not "cfString". There is nothing
to disambiguate here.

> Source/WTF/wtf/cf/VectorCF.h:45
> +// Specialize the behavior of these functions by overloading the
makeCFArrayElement
> +// functions and makeVectorElement functions. The makeCFArrayElement
function takes
> +// a const& to a collection element and can return either a
RetainPtr<CFTypeRef> or a CFTypeRef
> +// if the value is autoreleased. The makeVectorElement function takes an
ignored
> +// pointer to the vector element type, making argument-dependent lookup
work, and a
> +// CFTypeRef for the array element, and returns an Optional<T> of the the
vector element,
> +// allowing us to filter out array elements that are not of the expected
type.
> +//
> +//	 RetainPtr<CFTypeRef> makeCFArrayElement(const CollectionElementType&
collectionElement);
> +//	     -or-
> +//	 CFTypeRef makeCFArrayElement(const VectorElementType& vectorElement);
> +//
> +//	 Optional<VectorElementType> makeVectorElement(const
VectorElementType*, CFTypeRef arrayElement);

It’s unpleasant to have this be a copied and pasted version of almost
everything in the VectorCocoa.h header. We should strive to make them share
more, rather than having them duplicate everything. There must be some way to
take advantage of the toll-free bridging between the two or use templates.

I’m especially concerned because I consider VectorCocoa.h to be a work in
progress that needs some improvement. It will be annoying to have to update two
clones.

Given that CFTypeRef is not actually a distinct type, it’s actually just const
void*, we need to give makeVectorElement a name that mentions CF. The same is
not true of the Objective-C version, because id is a distinct type.

> Source/WTF/wtf/cf/VectorCF.h:77
> +    auto array = adoptCF(CFArrayCreateMutable(0, std::size(collection),
&kCFTypeArrayCallBacks));

I suggest nullptr rather than 0 for the first argument to CFArrayCreateMutable.

> Source/WTF/wtf/cf/VectorCF.h:85
> +    CFIndex count = CFArrayGetCount(array);

This will crash if array is nullptr. Is that the behavior we want? The NSArray
versions don’t have that behavior.

> Source/WTF/wtf/cf/VectorCF.h:100
> +    CFIndex count = CFArrayGetCount(array);

This will crash if array is nullptr. Is that the behavior we want? The NSArray
versions don’t have that behavior.

> Source/WTF/wtf/text/WTFString.h:428
> +WTF_EXPORT_PRIVATE Optional<String> makeVectorElement(const String*,
CFTypeRef);

Already mentioned above, but because CFTypeRef isn’t really a type, it’s just a
synonym for const void*, this function is dangerous. It means that anyone who
tries to make a vector element from a const void* will use this function that
assumes it’s a CFTypeRef. I think we need to consider whether there is a better
solution that involves evidence that CF is involved. We simply can’t deduce
that from the type const void*. Can fix this by coming up with a different name
for this function.

> Source/WTF/wtf/text/cf/StringCF.cpp:70
> +RetainPtr<CFTypeRef> makeCFArrayElement(const String& vectorElement)
> +{
> +    return vectorElement.createCFString();
> +}

This should be inlined in the header, not exported privately. It’s just a call
to another function. We don’t want to compile a function that just calls
another.

> Source/WTF/wtf/text/cf/StringCF.cpp:77
> +Optional<String> makeVectorElement(const String*, CFTypeRef arrayElement)
> +{
> +    if (auto cfString = dynamic_cf_cast<CFStringRef>(arrayElement))
> +	   return String(cfString);
> +    return WTF::nullopt;
> +}

We should consider putting this in a header too. Pretty sure we’d like it
inlined.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:490
> +    auto captionLanguages = platformCaptionLanguages ?
makeVector<String>(platformCaptionLanguages.get()) : Vector<String>();

This is not an improvement. We now allocate a vector just so we can append it
to another vector. That is costly. Please restructure this code so we don’t
allocate a temporary vector for this. I think that might just mean leaving this
more or less alone. It was more efficient without creating an additional
temporary vector. The correct idiom here would probably be to make appending a
CFArray to a Vector possible, in much the way that we can append a CFStringRef
to a StringBuilder. Allocating a copy is what we’re trying to avoid.

So I think I suggest leaving this code alone until we can use a better idiom
without sacrificing performance.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:513
> +    auto characteristics = platformCharacteristics ?
makeVector<String>(platformCharacteristics.get()) : Vector<String>();

Ditto.

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:454
> +    auto array = createCFArray(value, [] (const auto& item) {
> +	   return adoptCF(CFNumberCreate(0, kCFNumberFloatType, &item));
> +    });

This should be "float item" rather than "const auto& item". It’s a case when we
want to be explicit about the type, because the code just below takes a
pointer, passes it to something that takes a const void* and relies on us to
know we are passing a float.

It would be neat to use "const auto& item" if the code below implicitly
respected the type of what was passed, but that is not so in this case.

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:518
> +    auto array = createCFArray(value, [] (const auto& item) {
> +	   return adoptCF(CFNumberCreate(0, kCFNumberFloatType, &item));
> +    });

Ditto.

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:554
> +	   if (is<PlatformCAAnimationWin>(animation))
> +	       return downcast<PlatformCAAnimationWin>(*animation).m_animation;
> +	   return nullptr;

Usually in WebKit code we prefer early return:

    if (!is<>)
	return nullptr;
    return xxx;

The idea is that the check returns early so the main code moves down the left.
An exception is where we want to guard the input.

> Source/WebKit/NetworkProcess/webrtc/NetworkRTCResolverCocoa.cpp:56
> +	   if (auto data = checked_cf_cast<CFDataRef>(item))
> +	       return IPAddress::fromSockAddrIn6(*reinterpret_cast<const struct
sockaddr_in6*>(CFDataGetBytePtr(data)));

It’s bizarre for us to check the type, CFDataRef, with a release assertion but
then not check that the length is long enough. Not new, but we really should
add that code, if we don’t trust the type passed in. Also unclear why it’s
important to check the type of these items, but not of the array itself. Maybe
the correct fix is to remove checked_cf_cast and use static_cast instead.