[webkit-reviews] review granted: [Bug 202825] Only use CFNetwork SPI for metrics where needed : [Attachment 380685] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 202825: Only use CFNetwork SPI for metrics where needed
https://bugs.webkit.org/show_bug.cgi?id=202825

Attachment 380685: Patch

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




--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 380685
  --> https://bugs.webkit.org/attachment.cgi?id=380685
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.