Bug 212921

<rdar://problem/64134506>

Created attachment 401369 [details] Patch v1

Comment on attachment 401369 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=401369&action=review > Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:54 > +enum class CFType : uint8_t { We should take off all the CF prefixes for the enumeration values.

Comment on attachment 401369 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=401369&action=review >> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:54 >> +enum class CFType : uint8_t { > > We should take off all the CF prefixes for the enumeration values. Except that it seems we have null and CFNull!?

Comment on attachment 401369 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=401369&action=review >>> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:54 >>> +enum class CFType : uint8_t { >> >> We should take off all the CF prefixes for the enumeration values. > > Except that it seems we have null and CFNull!? Seems like it's to differentiate between a CFNull object and a nullptr.

Comment on attachment 401369 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=401369&action=review >>>> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:54 >>>> +enum class CFType : uint8_t { >>> >>> We should take off all the CF prefixes for the enumeration values. >> >> Except that it seems we have null and CFNull!? > > Seems like it's to differentiate between a CFNull object and a nullptr. Actually, I would propose keeping the "CF" prefix because the rule is to remove "Ref" from the actual type name to turn it into an enum (which doesn't work for types outside of CoreFoundation): IPC::CFType::CFString, IPC::CFType::CFURL, IPC::CFType::SecCertificate, IPC::CFType::SecIdentity, I would change CFType::Null to CFType::Nullptr, though, to differentiate that from CFNull[Ref[, though.

Created attachment 401383 [details] Patch v2

Comment on attachment 401383 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=401383&action=review > Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:55 > CFArray, I understand your rationale for wanting to leave the CF prefix on these, but I don’t agree that it’s better to have that mechanical rule and I would remove it if it was me doing the work. > Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:181 > + case CFType::Unknown: Seems like someone should later refactor to eliminate this value. Using Optional, most likely. It’s funny to validate that these are good values, but include one bad value. > Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:210 > + RetainPtr<CFDictionaryRef> dictionary = createSerializableRepresentation(requestToSerialize.get(), IPC::tokenNullptrTypeRef()); I’d use auto here. > Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:234 > + RetainPtr<NSURLRequest> nsURLRequest = createNSURLRequestFromSerializableRepresentation(dictionary.get(), IPC::tokenNullptrTypeRef()); And here. > Source/WebKit/mac/WebKit2.order:919 > -__ZN7CoreIPC16tokenNullTypeRefEv > +__ZN7CoreIPC19tokenNullptrTypeRefEv Normally we don’t try to keep these up to date. Not even sure why we still have them checked in.

Created attachment 401398 [details] Patch for landing

Comment on attachment 401383 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=401383&action=review >> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:210 >> + RetainPtr<CFDictionaryRef> dictionary = createSerializableRepresentation(requestToSerialize.get(), IPC::tokenNullptrTypeRef()); > > I’d use auto here. Fixed. >> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:234 >> + RetainPtr<NSURLRequest> nsURLRequest = createNSURLRequestFromSerializableRepresentation(dictionary.get(), IPC::tokenNullptrTypeRef()); > > And here. Fixed. >> Source/WebKit/mac/WebKit2.order:919 >> +__ZN7CoreIPC19tokenNullptrTypeRefEv > > Normally we don’t try to keep these up to date. Not even sure why we still have them checked in. Keith Miller tried to remove it in r253218, but it broke the build, so the patch was reverted (*.order restored) in r253234.

Comment on attachment 401398 [details] Patch for landing Adding cq+ since the only differences between this patch and Patch v2 are the reviewer added to ChangeLog and a couple of `auto` variables.

Committed r262762: <https://trac.webkit.org/changeset/262762> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401398 [details].