Bug 210702 – [Cocoa] Use createNSArray in many more places that build NSArray objects from C++ collections

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Darin Adler]

[Cocoa] Use createNSArray in many more places that build NSArray objects from C++ collections

[Darin Adler]

Created attachment 396865 [details]
Patch

[Darin Adler]

Created attachment 396875 [details]
Patch

[Darin Adler]

Created attachment 396877 [details]
Patch

[Darin Adler]

I didn’t write a change log yet, but the change looks like it’s working well.

[Darin Adler]

Created attachment 396916 [details]
Patch

[Darin Adler]

Created attachment 396953 [details]
Patch

[Darin Adler]

All tests passing. Ready for review.

[Alex Christensen]

Comment on attachment 396953 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h:-29
> -#ifndef WebAccessibilityObjectWrapperBase_h

Are we adding #pragma once in objc headers?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1739
> +        return nil;

I've been using nullptr instead of nil for RetainPtr because it's a C++ type.  I guess once we transition to ARC we'll use nil, right?

> Source/WebKitLegacy/mac/History/WebBackForwardList.mm:155
> +constexpr auto WebBackForwardListDictionaryEntriesKey = @"entries";

How do you feel about auto for objc types?  I've tried to avoid it.

[Darin Adler]

Comment on attachment 396953 [details]
Patch

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

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h:-29
>> -#ifndef WebAccessibilityObjectWrapperBase_h
> 
> Are we adding #pragma once in objc headers?

No, in fact we made a patch to remove #pragma once from them. They aren’t needed in Objective-C headers.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1739
>> +        return nil;
> 
> I've been using nullptr instead of nil for RetainPtr because it's a C++ type.  I guess once we transition to ARC we'll use nil, right?

I don’t care at all which we use, but in case you are wondering how I choice nil, in my mind, the nil isn’t for RetainPtr, it’s for the pointer stored in RetainPtr. And yes, if we move to ARC we won’t need RetainPtr for Objective-C pointers any more, just for CF pointers.

>> Source/WebKitLegacy/mac/History/WebBackForwardList.mm:155
>> +constexpr auto WebBackForwardListDictionaryEntriesKey = @"entries";
> 
> How do you feel about auto for objc types?  I've tried to avoid it.

I like it. I understand that others may not share my tastes.

[Darin Adler]

Comment on attachment 396953 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1739
>>> +        return nil;
>> 
>> I've been using nullptr instead of nil for RetainPtr because it's a C++ type.  I guess once we transition to ARC we'll use nil, right?
> 
> I don’t care at all which we use, but in case you are wondering how I choice nil, in my mind, the nil isn’t for RetainPtr, it’s for the pointer stored in RetainPtr. And yes, if we move to ARC we won’t need RetainPtr for Objective-C pointers any more, just for CF pointers.

I think it really depends on how abstract the code we are writing is. If it’s a RetainPtr and the underlying type is unknown, like in a template, then I would use nullptr. Or if it might not even be a RetainPtr, then I might use { }.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 396953 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Darin Adler]

Committed r260485: <https://trac.webkit.org/changeset/260485>

[Radar WebKit Bug Importer]

<rdar://problem/62145691>

[Alex Christensen]

Comment on attachment 396953 [details]
Patch

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

> Source/WebKit/Shared/ApplePay/cocoa/WebPaymentCoordinatorProxyCocoa.mm:190
> +    [result setThumbnailURLs:createNSArray(linkIconURLs).get()];

This caused a regression rdar://problem/62275343 because createNSArray(const Vector<URL>&) returns NSArray<NSString> because URL has operator const String&() const.  We want a makeNSArrayElement that takes a URL (easy), and we want to get rid of that operator String (hard).

[Darin Adler]

(In reply to Alex Christensen from comment #14)
> This caused a regression rdar://problem/62275343 because createNSArray(const
> Vector<URL>&) returns NSArray<NSString>

So sorry about the breakage!

Really frustrating that we have no regression tests for this.

> We want a makeNSArrayElement that takes a URL (easy)

OK. Or just use a lambda here.

> and we want to get rid of that operator String (hard).

I don’t agree that we want to do this. I can imagine we might do it, but I see arguments against it too.