[webkit-reviews] review granted: [Bug 191539] Web Inspector: Network: show secure connection details per-request : [Attachment 357511] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 191539: Web Inspector: Network: show secure connection details per-request
https://bugs.webkit.org/show_bug.cgi?id=191539

Attachment 357511: Patch

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




--- Comment #23 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 357511
  --> https://bugs.webkit.org/attachment.cgi?id=357511
Patch

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

r=me

> Source/JavaScriptCore/inspector/protocol/Network.json:97
> +		   { "name": "securityConnection", "$ref":
"Security.Connection", "optional": true, "description": "Connection information
for the completed request." }

Why not just name this `connection`?

> Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:214
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500) ||
(PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000)

Alexey was just talking about moving these to USE/HAVE macros. For now I think
its okay as is, I actually prefer to see this info at these sites.

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:197
> +	   this._connectionSection.appendKeyValuePair(WI.UIString("Protocol"),
connection.protocol);
> +	   this._connectionSection.appendKeyValuePair(WI.UIString("Cipher"),
connection.cipher);

The backend / protocol allows for only one of these properties. There should
probably be a default value such as an emdash.

    connection.protocol || emDash