[webkit-reviews] review granted: [Bug 191458] Web Inspector: Network: add button to show system certificate dialog : [Attachment 354872] 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 191458: Web Inspector: Network: add button to show system certificate
dialog
https://bugs.webkit.org/show_bug.cgi?id=191458

Attachment 354872: Patch

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




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

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

r=me!

> Source/WebCore/platform/network/cf/CertificateInfo.h:103
> +namespace WTF {
> +
> +namespace Persistence {

Style: Normally we don't have the extra newline between these (and the closing
braces). I see you added them, you may want to remove them.

> Source/WebCore/platform/network/cf/CertificateInfo.h:128
> +
> +

Style: Extra newline, carried from the original but we can clean it up.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:54
> +	   return InspectorFrontendHost.supportsShowCertificate &&
InspectorFrontendHost.showCertificate && window.NetworkAgent &&
NetworkAgent.getSerializedCertificate;

This isn't actually calling supportsShowCertificate, just checking if it exists
which will always be true.

Further it looks like we don't have the IFH stub anymore, so we don't need to
feature check IFH functions and can assume it always exists.

I think this should be:

    static supportsShowCertificate()
    {
	return InspectorFrontendHost.supportsShowCertificate() &&
window.NetworkAgent && NetworkAgent.getSerializedCertificate;
    }

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1074
> +	       throw errorString; // Don't expose the error.

The comment doesn't seem useful.

> Source/WebKit/UIProcess/RemoteWebInspectorProxy.h:117
> +    void platformShowCertificate(const WebCore::CertificateInfo&);

To get the Windows ports to build you need to include a stub in
WebInspectorProxyWPE.cpp and WebInspectorProxyWin.cpp for this.