Update some documentation for CertVerifyResult::verified_cert

net/cert/cert_verify_result.h

Index: net/cert/cert_verify_result.h
diff --git a/net/cert/cert_verify_result.h b/net/cert/cert_verify_result.h
index 29c0675af3f3141b79d82f9f16170c4b7cbbca40..7946facfbfbe1e86cedb6e77eb3734f7d6eac6a2 100644
--- a/net/cert/cert_verify_result.h
+++ b/net/cert/cert_verify_result.h
@@ -26,14 +26,17 @@ class NET_EXPORT CertVerifyResult {
 
   void Reset();
 
+  // Comparing CertVerifyResult with a nullptr |verified_cert| is invalid.

[Ryan Sleevi, 2017/01/09 23:20:14]

Why? Are you documenting implementation details or interface contract? Feels
like the former.

Alternatively, if the contract is "verified_cert should never be nullptr",
perhaps that's part of the contract for verified_cert, rather than being
documented here?

[eroman, 2017/01/09 23:33:12]

I was documenting the current behavior, which relies on not being called with a
default-initialized (or Reset()) CertVerifyResult.

This looked a bit suspicious when reading through the code, since one generally
expects operator== to work on any instance, but in practice it doesn't get
called in that mode.

Instead of the comment I could modify the implementation to test for nullptr,
however I have not reviewed the callers to see if that policy is appropriate yet
(i.e. if hitting that situation is wrong, crashing may be preferable)

   bool operator==(const CertVerifyResult& other) const;
 
   // The certificate and chain that was constructed during verification.
   // Note that the though the verified certificate will match the originally
   // supplied certificate, the intermediate certificates stored within may
-  // be substantially different. In the event of a verification failure, this
-  // will contain the chain as supplied by the server. This may be NULL if
-  // running within the sandbox.
+  // be substantially different (both their order and their content).
+  // In the event of a verification failure, |verified_cert| may contain a
+  // partially constructed path, or the original chain.

[Ryan Sleevi, 2017/01/09 23:20:14]

s/, / /

[eroman, 2017/01/09 23:33:12]

Done.

+  // In the case of successful verification the trust anchor is the final
+  // intermediate, or the target certificate if there are no intermediates.

[Ryan Sleevi, 2017/01/09 23:20:14]

Documentation wise, there's also some pre-existing subject/verb agreement issues

// The certificate chain that was constructed during verification.
// Note: Although |verified_cert| will match the originally supplied
// certificate to be validated, the results of
// GetIntermediateCertificates() may be substantially different,
// both in order and in content, then the originally supplied
// intermediates.
// In the event of validation failures, this may contain the
// originally supplied certificate chain or a partially
// constructed path, depending on the implementation.
// In the event of validation success, the trust anchor will be
// |verified_cert->GetIntermediateCertificates().back()| if
// there was a certificate chain to the trust anchor, and will
// be |verified_cert->os_cert_handle()| if the certificate was
// the trust anchor.

WDYT? Probably still botched the wording.

[eroman, 2017/01/09 23:33:12]

Done.

   scoped_refptr<X509Certificate> verified_cert;
 
   // Bitmask of CERT_STATUS_* from net/cert/cert_status_flags.h. Note that