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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Dec 7 11:49:18 PST 2018

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

Attachment 356614: Patch


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

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

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

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

More information about the webkit-reviews mailing list