Bug 207067 – Add KVO SPI WKWebView._negotiatedLegacyTLS

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

[Alex Christensen]

Add KVO SPI WKWebView._negotiatedLegacyTLS

[Alex Christensen]

Created attachment 389403 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/59072968>

[Alex Christensen]

Created attachment 389412 [details]
patch

[Alex Christensen]

Created attachment 389413 [details]
patch with SPI documentation.

[Andy Estes]

Comment on attachment 389413 [details]
patch with SPI documentation.

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:820
> +        auto tlsVersion = (tls_protocol_version_t)metrics.negotiatedTLSProtocolVersion.unsignedShortValue;

reinterpret_cast?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:821
> +        if (tlsVersion == tls_protocol_version_TLSv10 || tlsVersion == tls_protocol_version_TLSv11)

`tlsVersion < tls_protocol_version_TLSv12`?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:828
> +            if (tlsVersion == kTLSProtocol11 || tlsVersion == kTLSProtocol1)

Ditto?

> Source/WebKit/UIProcess/PageLoadState.h:226
> +        double estimatedProgress { 0.0 };

Our style is to just use 0 here.

[Alex Christensen]

(In reply to Andy Estes from comment #5)
> Comment on attachment 389413 [details]
> patch with SPI documentation.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389413&action=review
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:820
> > +        auto tlsVersion = (tls_protocol_version_t)metrics.negotiatedTLSProtocolVersion.unsignedShortValue;
> 
> reinterpret_cast?
Sounds good.

> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:821
> > +        if (tlsVersion == tls_protocol_version_TLSv10 || tlsVersion == tls_protocol_version_TLSv11)
> 
> `tlsVersion < tls_protocol_version_TLSv12`?
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:828
> > +            if (tlsVersion == kTLSProtocol11 || tlsVersion == kTLSProtocol1)
> 
> Ditto?
Strangely enough, we only want to do that with these two particular protocols here.

> > Source/WebKit/UIProcess/PageLoadState.h:226
> > +        double estimatedProgress { 0.0 };
> 
> Our style is to just use 0 here.
I think you're right, even though I think we should change our style in this wonderfully statically-typed language with typed literals.  I'll change to 0.

[Alex Christensen]

Created attachment 389416 [details]
Patch

[Alex Christensen]

Also of note: we use c-style casts heavily in WebKit's ObjC++ code.  Maybe we should go through and change them all to c++-style casts.

[WebKit Commit Bot]

Comment on attachment 389416 [details]
Patch

Clearing flags on attachment: 389416

Committed r255522: <https://trac.webkit.org/changeset/255522>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

(In reply to Alex Christensen from comment #8)
> Also of note: we use c-style casts heavily in WebKit's ObjC++ code.  Maybe
> we should go through and change them all to c++-style casts.

I think our rule (or at least the rule I follow) is to use a C-style cast for Objective-C and CF types (e.g., (__bridge CFSStringRef)@"hello") and C++-style casts otherwise. Or perhaps more generally, use C++ casts for the code that caused you to create the .mm file and C-style casts otherwise.

[Alex Christensen]

https://trac.webkit.org/changeset/255527/webkit was a build fix after this.