Bug 202825 – Only use CFNetwork SPI for metrics where needed

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Only use CFNetwork SPI for metrics where needed

[Alex Christensen]

Created attachment 380685 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 380685 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380685&action=review

r=me

The first time we tried this (bug 198762) got rolled out in r247095. Just make sure this test continues to pass!
http/tests/inspector/network/har/har-page.html

> Source/WTF/wtf/Platform.h:1521
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000)
> +#define HAVE_CFNETWORK_METRICS 1
> +#endif

Nit: I believe Keith Rollins was just investigating if we should enable these for tvOS / watchOS. The APIs do exist there, so I suggest we do that. Maybe you can sync up with him on how best to do that, but that doesn't block this.

I'd probably have named this positive for "APIs" since older builds have the "SPIs". So maybe something like HAVE_CFNETWORK_METRICS_APIS_V4? I'd leave that up to you.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:710
> +            networkLoadMetrics.tlsCipher = stringForSSLCipher((SSLCipherSuite)[m.negotiatedTLSCipherSuite unsignedShortValue]);

Shouldn't this be a `tls_ciphersuite_t` not a `SSLCipherSuite`? Or are they interchangeable?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:734
>              for (NSURLSessionTaskTransactionMetrics *transactionMetrics in metrics.transactionMetrics) {
> +#if HAVE(CFNETWORK_METRICS)
> +                requestHeaderBytesSent += transactionMetrics.countOfRequestHeaderBytesSent;
> +                responseHeaderBytesReceived += transactionMetrics.countOfResponseHeaderBytesReceived;
> +                responseBodyBytesReceived += transactionMetrics.countOfResponseBodyBytesReceived;
> +                responseBodyDecodedSize += transactionMetrics.countOfResponseBodyBytesAfterDecoding ? transactionMetrics.countOfResponseBodyBytesAfterDecoding : transactionMetrics.countOfResponseBodyBytesReceived;
> +#else

Not significant but we should probably update NetworkLoadMetrics's values here from uint32_ts to uint64_ts. This was done in the first attempt to use the CFNetwork APIs:
https://bugs.webkit.org/attachment.cgi?id=371875&action=review

```
    uint32_t requestHeaderBytesSent;
    uint32_t responseHeaderBytesReceived;
```

The CFNetwork APIs here are `int64_t` and the SPIs here were `NSInteger`. So it seems a >32bit number could happen.

[Alex Christensen]

Created attachment 380769 [details]
Patch

[Alex Christensen]

Created attachment 380772 [details]
Patch

[Joseph Pecoraro]

*** Bug 198762 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 380772 [details]
Patch

Clearing flags on attachment: 380772

Committed r251021: <https://trac.webkit.org/changeset/251021>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56313694>

[Tim Horton]

Comment on attachment 380772 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380772&action=review

> Source/WTF/wtf/Platform.h:1519
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000)

FWIW "PLATFORM(IOS_FAMILY) && only testing an IPHONE_OS version" is always wrong. This is disabled on all non-iOS iOS-family platforms.