[webkit-reviews] review granted: [Bug 191447] Web Inspector: Network: show secure certificate details per-request : [Attachment 354447] 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 191447: Web Inspector: Network: show secure certificate details per-request
https://bugs.webkit.org/show_bug.cgi?id=191447

Attachment 354447: Patch

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




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

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

r=me with comments

> Source/WebCore/platform/network/cf/CertificateInfo.cpp:1
> +/*

Any reason this file isn't titled CertificateInfoCF.cpp? Pretty much all the
other files in this directory have that suffix.

> Source/WebCore/platform/network/cf/CertificateInfo.cpp:123
> +	   const Seconds absoluteReferenceDate(978307200);

Probably deserves a comment. This is the 1970 epoch?

> Source/WebCore/platform/network/cf/CertificateInfo.h:-76
> -#ifndef NDEBUG
> -    void dump() const;
> -#endif

You could still provide a `dump()` implementation in a
platform/network/cocoa/CertificateInfoCocoa.mm. Here is would be, and the
implementation would be identical to what it was before.

    #ifndef NDEBUG
    #if PLATFORM(COCOA)
	void dump() const;
    #endif
    #endif

> Source/WebInspectorUI/UserInterface/Views/ResourceSecurityContentView.js:65
> +	   if (!this._resource.loadedSecurely) {

What if we don't have any network details at this point.

Scenario:
1. Load webkit.org
2. Open inspector
3. Go to Network Tab
4. Select a Resource
5. Select Security
  => We don't have secure timing details, does this mean it says loaded
insecurely!?

We might want to present the "incomplete details" text.