Allow TrustStore queries to be asynchronous.

net/cert/internal/trust_store.h

Index: net/cert/internal/trust_store.h
diff --git a/net/cert/internal/trust_store.h b/net/cert/internal/trust_store.h
index e8706e78d1da765f8a9be8fe5ed394b66531ed80..2d7b07c021a40bfe2badfd794d975b8884d92f88 100644
--- a/net/cert/internal/trust_store.h
+++ b/net/cert/internal/trust_store.h
@@ -7,6 +7,7 @@
 
 #include <vector>
 
+#include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "net/base/net_export.h"
 #include "net/cert/internal/parsed_certificate.h"
@@ -114,13 +115,34 @@ using TrustAnchors = std::vector<scoped_refptr<TrustAnchor>>;
 // Interface for finding trust anchors.
 class NET_EXPORT TrustStore {
  public:
+  class NET_EXPORT Request {
+   public:
+    Request() = default;
+    // Destruction of the Request cancels it.
+    virtual ~Request() = default;

[eroman, 2016/08/23 18:30:10]

can this be moved to the .cc file?

[mattm, 2016/08/26 02:24:43]

Done.

+  };
+
   TrustStore();
   virtual ~TrustStore();
 
-  // Returns the trust anchors that match |name| in |*matches|, if any.
-  virtual void FindTrustAnchorsByNormalizedName(
-      const der::Input& normalized_name,
-      TrustAnchors* matches) const = 0;
+  using TrustAnchorCallback =
+      base::Callback<void(std::unique_ptr<TrustAnchors>)>;

[eroman, 2016/08/23 18:30:10]

Did you consider a pass-by-value of TrustAnchors?
Should cost about the same when using std::move()

[mattm, 2016/08/26 02:24:42]

Done.

+
+  // Returns the trust anchors that match |cert|'s issuer name in |*out_matches|

[eroman, 2016/08/23 18:30:10]

Document that |cert| and |out_matches| must be non-null.

[mattm, 2016/08/26 02:24:42]

Done.

+  // and/or through |callback|.
+  //
+  // If results are available synchronously, they will be returned in
+  // |*out_matches|.

[eroman, 2016/08/23 18:30:10]

Can you mention that |out_matches| is *not* used in the case of async
completion.

[mattm, 2016/08/26 02:24:43]

Done.

+  //
+  // If |callback| is not null and results may be available asynchronously,

[eroman, 2016/08/23 18:30:10]

Note when reading this I assume completion was *either* async or sync, but not
possibly both. Can you clarify that in the documentation?

[mattm, 2016/08/26 02:24:43]

The first line does say results are returned in "|*synchronous_matches| and/or
through |callback|". Could that be clearer?

+  // |*out_req| will be filled with a Request, and |callback| will be called

[eroman, 2016/08/23 18:30:10]

Please clarify what "filled" means.

My assumption as a reader here is that this means assignment. However based on
the implementations this is an append operation.

[mattm, 2016/08/26 02:24:42]

out_req is assigned (it's the unique_ptr<Request>).  You probably ment
|synchronous_matches|, which is appended, and I will update the docs for that to
mention it.

+  // when results are available. The Request may be destroyed to cancel

[eroman, 2016/08/23 18:30:10]

Suggest clarifying that |out_req| can be null in the synchronous-only mode. But
must be non-null in async mode.

[mattm, 2016/08/26 02:24:43]

Currently it must always be non-null. But allowing it to be null in
synchronous-only mode seems reasonable. done.

+  // the callback if it has not occurred yet.
+  virtual void FindTrustAnchorsForCert(
+      const ParsedCertificate* cert,
+      const TrustAnchorCallback& callback,
+      TrustAnchors* out_matches,

[eroman, 2016/08/23 18:30:10]

Suggest renaming this to something more like "synchronous_matches".

[mattm, 2016/08/26 02:24:43]

Done.

+      std::unique_ptr<Request>* out_req) const = 0;

[eroman, 2016/08/23 18:30:10]

side-comment: Should the API allow for a probing use-case? (i.e. calling in a
way to get only synchronous results, however know whether there are also async
results to be had)

[mattm, 2016/08/26 02:24:43]

The current pathbuilder impl doesn't need it, so I didn't include that ability.
I think we can just change it later if we come up with a use case that needs it.

 
  private:
   DISALLOW_COPY_AND_ASSIGN(TrustStore);