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

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied 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 356614: Patch

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




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

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

Please add a screenshot for the UI (even though it should be pretty
straightforward).

I do think this shouldn't hang on the same Security object, that starts getting
weird because it gets updated in different places for different reasons.

The rest of this patch looks great though.

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

I'd just call this "connection" and $ref "Security.Connection" directly. I
didn't think you'd go this route of using the same object. This would also
avoid the `updateSecurity` weird merging that happens later.

Nit: Does the word "Resolved" in the description add anything?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:137
> +    // STRINGIFY_CIPHER(SSL_NULL_WITH_NULL_NULL);

Would be nice to include a pointer to the value that this is shared with. (Or
just putting it along side that value).