Bug 212921 - [IPC] Adopt enum class for IPC::CFType
Summary: [IPC] Adopt enum class for IPC::CFType
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks: 211988
  Show dependency treegraph
 
Reported: 2020-06-08 14:12 PDT by David Kilzer (:ddkilzer)
Modified: 2020-06-11 13:38 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1 (11.41 KB, patch)
2020-06-08 14:16 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (15.52 KB, patch)
2020-06-08 15:39 PDT, David Kilzer (:ddkilzer)
darin: review+
Details | Formatted Diff | Diff
Patch for landing (15.48 KB, patch)
2020-06-08 16:30 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2020-06-08 14:12:38 PDT
Adopt enum class for IPC::CFType.
Comment 1 Radar WebKit Bug Importer 2020-06-08 14:13:24 PDT
<rdar://problem/64134506>
Comment 2 David Kilzer (:ddkilzer) 2020-06-08 14:16:35 PDT
Created attachment 401369 [details]
Patch v1
Comment 3 Darin Adler 2020-06-08 14:17:20 PDT
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 4 Darin Adler 2020-06-08 14:17:57 PDT
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 5 David Kilzer (:ddkilzer) 2020-06-08 14:34:32 PDT
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 6 David Kilzer (:ddkilzer) 2020-06-08 14:40:28 PDT
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.
Comment 7 David Kilzer (:ddkilzer) 2020-06-08 15:39:21 PDT
Created attachment 401383 [details]
Patch v2
Comment 8 Darin Adler 2020-06-08 15:46:11 PDT
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.
Comment 9 David Kilzer (:ddkilzer) 2020-06-08 16:30:12 PDT
Created attachment 401398 [details]
Patch for landing
Comment 10 David Kilzer (:ddkilzer) 2020-06-08 16:33:06 PDT
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 11 David Kilzer (:ddkilzer) 2020-06-08 18:10:45 PDT
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.
Comment 12 EWS 2020-06-08 18:26:33 PDT
Committed r262762: <https://trac.webkit.org/changeset/262762>

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