Add support for walking and modifying the CachingCertVerifier

net/cert/caching_cert_verifier.h

Index: net/cert/caching_cert_verifier.h
diff --git a/net/cert/caching_cert_verifier.h b/net/cert/caching_cert_verifier.h
index 74d81a0ef62e6de2beff793f09858ed7c1a89a64..21d14aae17d9cbf20583a8dc63d9c467ed4bcd8c 100644
--- a/net/cert/caching_cert_verifier.h
+++ b/net/cert/caching_cert_verifier.h
@@ -20,6 +20,28 @@ class CertTrustAnchorProvider;
 class NET_EXPORT CachingCertVerifier : public CertVerifier,
                                        public CertDatabase::Observer {
  public:
+  // Provides a read-only iterator over items in the cache.
+  //
+  // This iterator is invalidated by any modifications to the cache, such
+  // as by a call to Verify() or to AddEntry().
+  class NET_EXPORT Iterator {
+   public:
+    explicit Iterator(const CachingCertVerifier& verifier);

[eroman, 2016/06/13 22:49:35]

I would have said this is clearer as a pointer rather than reference type given
how this is coupled to the lifetime of |verifier|.... But looks like the
underlying cache iterator using this same interface.

Can you  add a comment explaining that |verifier| needs to outlive |this| ?
(Technically a bit stronger requirement, but easier to explain).

+    ~Iterator();
+
+    bool HasNext() const;

[eroman, 2016/06/13 22:49:35]

Iteration currently exposes expired entries right?

Should a compaction be done before iterating, or is the expectation on the
client to not bother serializing expired entries?

+    void Advance();
+
+    const RequestParams& params() const;
+    int error() const;
+    const CertVerifyResult& verify_result() const;
+    base::Time verification_time() const;
+    base::Time expiration_time() const;
+
+   private:
+    void* iter_;
+  };

[eroman, 2016/06/13 22:49:35]

Disallow copy and assign.

+
   explicit CachingCertVerifier(std::unique_ptr<CertVerifier> verifier);
 
   ~CachingCertVerifier() override;
@@ -40,7 +62,19 @@ class NET_EXPORT CachingCertVerifier : public CertVerifier,
              const BoundNetLog& net_log) override;
   bool SupportsOCSPStapling() override;
 
+  // Opportunistically attempt to add |error| and |verify_result| as the

[eroman, 2016/06/13 22:49:35]

style nit: use descriptive rather than imperative comment. i.e. "attempt" -->
"attempts"

+  // result for |params|, which was obtained at |verification_time| and
+  // expires at |expiration_time|.
+  // This is opportunistic because it is not guaranteed that the entry
+  // will be added (such as if the cache is full or an entry already
+  // exists).

[eroman, 2016/06/13 22:49:35]

nit: Mention how how this relates to the return value.

[eroman, 2016/06/16 00:25:04]

ping? (My request was to comment that returns true if an entry was added)

+  bool AddEntry(const RequestParams& params,
+                int error,
+                const CertVerifyResult& verify_result,
+                base::Time verification_time);
+
  private:
+  friend class Iterator;
   FRIEND_TEST_ALL_PREFIXES(CachingCertVerifierTest, CacheHit);
   FRIEND_TEST_ALL_PREFIXES(CachingCertVerifierTest, DifferentCACerts);
   FRIEND_TEST_ALL_PREFIXES(CachingCertVerifierTest, CertTrustAnchorProvider);