| Summary: | [Cocoa] Use createNSArray in many more places that build NSArray objects from C++ collections | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||||
| Component: | WebKit Misc. | Assignee: | Darin Adler <darin> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | aboxhall, achristensen, andersca, apinheiro, benjamin, cdumez, cfleizach, cmarcelo, dmazzoni, eric.carlson, ews-watchlist, glenn, jbedard, jcraig, jdiggs, jer.noble, mifenton, philipj, samuel_white, sam, sergio, simon.fraser, webkit-bug-importer | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=210997 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Darin Adler
2020-04-18 15:22:10 PDT
Created attachment 396865 [details]
Patch
Created attachment 396875 [details]
Patch
Created attachment 396877 [details]
Patch
I didn’t write a change log yet, but the change looks like it’s working well. Created attachment 396916 [details]
Patch
Created attachment 396953 [details]
Patch
All tests passing. Ready for review. 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 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 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 { }. Tools/Scripts/svn-apply failed to apply attachment 396953 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Committed r260485: <https://trac.webkit.org/changeset/260485> 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). (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. |