Bug 212921 – [IPC] Adopt enum class for IPC::CFType

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[David Kilzer (:ddkilzer)]

Adopt enum class for IPC::CFType.

[Radar WebKit Bug Importer]

<rdar://problem/64134506>

[David Kilzer (:ddkilzer)]

Created attachment 401369 [details]
Patch v1

[Darin Adler]

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.

[Darin Adler]

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!?

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 401383 [details]
Patch v2

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

Created attachment 401398 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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.

[EWS]

Committed r262762: <https://trac.webkit.org/changeset/262762>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401398 [details].