Allow TrustStore queries to be asynchronous.

Allow TrustStore queries to be asynchronous.

Also changes FindTrustAnchorsByNormalizedName to FindTrustAnchorsForCert, as
some different implementations may do normalization differently. By passing in
the target cert, the implementation can decide whether to use the
pre-normalized issuer or the raw one.

BUG=635203
Committed: https://crrev.com/9009fb22a7b7a80cb28164d0bef8d47daa2ef81e
Cr-Commit-Position: refs/heads/master@{#414875}

[mattm, 2016-08-23 01:21:48]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-23 01:22:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2016-08-23 01:30:56]

mattm@chromium.org changed reviewers:
+ eroman@chromium.org

[commit-bot: I haz the power, 2016-08-23 04:32:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-23 04:32:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[eroman, 2016-08-23 18:30:11]

Initial comments just for the interface.

Also regarding the CL description, the second sentence reads like an elaboration
of the first (Allow Trust queries to be asynchronous) but really it is a second
thing.

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
File net/cert/internal/trust_store.h (right):

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:122: virtual ~Request() = default;
can this be moved to the .cc file?

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:129:
base::Callback<void(std::unique_ptr<TrustAnchors>)>;
Did you consider a pass-by-value of TrustAnchors?
Should cost about the same when using std::move()

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:131: // Returns the trust anchors that match
|cert|'s issuer name in |*out_matches|
Document that |cert| and |out_matches| must be non-null.

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:135: // |*out_matches|.
Can you mention that |out_matches| is *not* used in the case of async
completion.

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:137: // If |callback| is not null and results
may be available asynchronously,
Note when reading this I assume completion was *either* async or sync, but not
possibly both. Can you clarify that in the documentation?

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:138: // |*out_req| will be filled with a
Request, and |callback| will be called
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.

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:139: // when results are available. The Request
may be destroyed to cancel
Suggest clarifying that |out_req| can be null in the synchronous-only mode. But
must be non-null in async mode.

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:144: TrustAnchors* out_matches,
Suggest renaming this to something more like "synchronous_matches".

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:145: std::unique_ptr<Request>* out_req) const =
0;
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)

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
File net/cert/internal/trust_store_test_helpers.h (right):

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_test_helpers.h:19: base::Closure done_callback,
should this be a const-reference?

[mattm, 2016-08-26 01:57:26]

Description was changed from

==========
Allow TrustStore queries to be asynchronous.

Change FindTrustAnchorsByNormalizedName to FindTrustAnchorsForCert, as
some different implementations may do normalization differently. By passing in
the target cert, the implementation can decide whether to use the
pre-normalized issuer or the raw one.

BUG=635203
==========

to

==========
Allow TrustStore queries to be asynchronous.

Also changes FindTrustAnchorsByNormalizedName to FindTrustAnchorsForCert, as
some different implementations may do normalization differently. By passing in
the target cert, the implementation can decide whether to use the
pre-normalized issuer or the raw one.

BUG=635203
==========

[mattm, 2016-08-26 02:23:25]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 02:24:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
File net/cert/internal/trust_store.h (right):

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:122: virtual ~Request() = default;
On 2016/08/23 18:30:10, eroman wrote:
> can this be moved to the .cc file?

Done.

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:129:
base::Callback<void(std::unique_ptr<TrustAnchors>)>;
On 2016/08/23 18:30:10, eroman wrote:
> Did you consider a pass-by-value of TrustAnchors?
> Should cost about the same when using std::move()

Done.

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:131: // Returns the trust anchors that match
|cert|'s issuer name in |*out_matches|
On 2016/08/23 18:30:10, eroman wrote:
> Document that |cert| and |out_matches| must be non-null.

Done.

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:135: // |*out_matches|.
On 2016/08/23 18:30:10, eroman wrote:
> Can you mention that |out_matches| is *not* used in the case of async
> completion.

Done.

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:137: // If |callback| is not null and results
may be available asynchronously,
On 2016/08/23 18:30:10, eroman wrote:
> Note when reading this I assume completion was *either* async or sync, but not
> possibly both. Can you clarify that in the documentation?

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

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:138: // |*out_req| will be filled with a
Request, and |callback| will be called
On 2016/08/23 18:30:10, eroman wrote:
> 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.

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.

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:139: // when results are available. The Request
may be destroyed to cancel
On 2016/08/23 18:30:10, eroman wrote:
> Suggest clarifying that |out_req| can be null in the synchronous-only mode.
But
> must be non-null in async mode.

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

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:144: TrustAnchors* out_matches,
On 2016/08/23 18:30:10, eroman wrote:
> Suggest renaming this to something more like "synchronous_matches".

Done.

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store.h:145: std::unique_ptr<Request>* out_req) const =
0;
On 2016/08/23 18:30:10, eroman wrote:
> 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)

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.

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
File net/cert/internal/trust_store_test_helpers.h (right):

https://codereview.chromium.org/2266333002/diff/1/net/cert/internal/trust_sto...
net/cert/internal/trust_store_test_helpers.h:19: base::Closure done_callback,
On 2016/08/23 18:30:10, eroman wrote:
> should this be a const-reference?

Done.

[commit-bot: I haz the power, 2016-08-26 04:52:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-26 04:52:18]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[eroman, 2016-08-26 18:19:31]

lgtm

https://codereview.chromium.org/2266333002/diff/40001/net/cert/internal/path_...
File net/cert/internal/path_builder.cc (right):

https://codereview.chromium.org/2266333002/diff/40001/net/cert/internal/path_...
net/cert/internal/path_builder.cc:317: DVLOG(1) << "CertanchorsIter(" <<
CertDebugString(cert())
typo?

[mattm, 2016-08-26 22:14:18]

https://codereview.chromium.org/2266333002/diff/40001/net/cert/internal/path_...
File net/cert/internal/path_builder.cc (right):

https://codereview.chromium.org/2266333002/diff/40001/net/cert/internal/path_...
net/cert/internal/path_builder.cc:317: DVLOG(1) << "CertanchorsIter(" <<
CertDebugString(cert())
On 2016/08/26 18:19:31, eroman wrote:
> typo?

Done.

[mattm, 2016-08-26 22:14:22]

The CQ bit was checked by mattm@chromium.org

[mattm, 2016-08-26 22:14:23]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2266333002/#ps60001
(title: "typo")

[commit-bot: I haz the power, 2016-08-26 22:14:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-27 00:22:02]

Description was changed from

==========
Allow TrustStore queries to be asynchronous.

Also changes FindTrustAnchorsByNormalizedName to FindTrustAnchorsForCert, as
some different implementations may do normalization differently. By passing in
the target cert, the implementation can decide whether to use the
pre-normalized issuer or the raw one.

BUG=635203
==========

to

==========
Allow TrustStore queries to be asynchronous.

Also changes FindTrustAnchorsByNormalizedName to FindTrustAnchorsForCert, as
some different implementations may do normalization differently. By passing in
the target cert, the implementation can decide whether to use the
pre-normalized issuer or the raw one.

BUG=635203
==========

[commit-bot: I haz the power, 2016-08-27 00:22:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-27 00:25:10]

Description was changed from

==========
Allow TrustStore queries to be asynchronous.

Also changes FindTrustAnchorsByNormalizedName to FindTrustAnchorsForCert, as
some different implementations may do normalization differently. By passing in
the target cert, the implementation can decide whether to use the
pre-normalized issuer or the raw one.

BUG=635203
==========

to

==========
Allow TrustStore queries to be asynchronous.

Also changes FindTrustAnchorsByNormalizedName to FindTrustAnchorsForCert, as
some different implementations may do normalization differently. By passing in
the target cert, the implementation can decide whether to use the
pre-normalized issuer or the raw one.

BUG=635203

Committed: https://crrev.com/9009fb22a7b7a80cb28164d0bef8d47daa2ef81e
Cr-Commit-Position: refs/heads/master@{#414875}
==========

[commit-bot: I haz the power, 2016-08-27 00:25:11]

Patchset 4 (id:??) landed as
https://crrev.com/9009fb22a7b7a80cb28164d0bef8d47daa2ef81e
Cr-Commit-Position: refs/heads/master@{#414875}