Bug 210702 - [Cocoa] Use createNSArray in many more places that build NSArray objects from C++ collections
Summary: [Cocoa] Use createNSArray in many more places that build NSArray objects from...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-18 15:22 PDT by Darin Adler
Modified: 2020-04-24 16:06 PDT (History)
23 users (show)

See Also:


Attachments
Patch (169.13 KB, patch)
2020-04-18 15:23 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (169.29 KB, patch)
2020-04-18 17:18 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (169.30 KB, patch)
2020-04-18 17:31 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (198.45 KB, patch)
2020-04-19 10:10 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (198.45 KB, patch)
2020-04-20 00:05 PDT, Darin Adler
achristensen: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-04-18 15:22:10 PDT
[Cocoa] Use createNSArray in many more places that build NSArray objects from C++ collections
Comment 1 Darin Adler 2020-04-18 15:23:21 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2020-04-18 17:18:35 PDT Comment hidden (obsolete)
Comment 3 Darin Adler 2020-04-18 17:31:06 PDT Comment hidden (obsolete)
Comment 4 Darin Adler 2020-04-18 18:05:09 PDT Comment hidden (obsolete)
Comment 5 Darin Adler 2020-04-19 10:10:46 PDT Comment hidden (obsolete)
Comment 6 Darin Adler 2020-04-20 00:05:31 PDT
Created attachment 396953 [details]
Patch
Comment 7 Darin Adler 2020-04-20 12:12:56 PDT
All tests passing. Ready for review.
Comment 8 Alex Christensen 2020-04-21 09:22:44 PDT
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.
Comment 9 Darin Adler 2020-04-21 12:17:08 PDT
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.
Comment 10 Darin Adler 2020-04-21 12:24:58 PDT
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 { }.
Comment 11 EWS 2020-04-21 18:30:20 PDT
Tools/Scripts/svn-apply failed to apply attachment 396953 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 12 Darin Adler 2020-04-21 18:51:01 PDT
Committed r260485: <https://trac.webkit.org/changeset/260485>
Comment 13 Radar WebKit Bug Importer 2020-04-21 18:51:18 PDT
<rdar://problem/62145691>
Comment 14 Alex Christensen 2020-04-24 14:49:33 PDT
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).
Comment 15 Darin Adler 2020-04-24 16:06:00 PDT
(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.