Bug 226123 – Add `createCFArray` just like `createNSArray`

• Status: NEW
• Component: New Bugs

[Devin Rousso]

When using ObjC one can just toll-free bridge to the equivalent `NS*` type and use `createNSArray`, but that assumes that there is an equivalent `NS*` type and that the code is in a place that can use ObjC (e.g. `.mm`).

As an example, this would have been useful in <https://webkit.org/b/226038>.

[Devin Rousso]

Created attachment 429377 [details]
Patch

[Devin Rousso]

Created attachment 429380 [details]
Patch

oops forgot new file

[Devin Rousso]

Created attachment 429385 [details]
Patch

[Darin Adler]

Comment on attachment 429385 [details]
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.

[Radar WebKit Bug Importer]

<rdar://problem/78636823>