Introduce CryptAuthClient, a class capable of performing all CryptAuth APIs.

Introduce CryptAuthClient, a class capable of performing all CryptAuth API calls.

BUG=419191
TEST=manual + new test

Committed: https://crrev.com/77b8ce75d17370871db8c1c2fd131d981e468a45
Cr-Commit-Position: refs/heads/master@{#307058}

[Tim Song, 2014-11-18 00:31:44]

tengs@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2014-11-18 22:30:45]

Haven't looked at the tests yet, because they might change substantially if you
choose to follow some of my suggestions.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth.gypi
File components/proximity_auth.gypi (right):

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth.gy...
components/proximity_auth.gypi:87: 'target_name': 'proximity_auth_unittests',
Hmm, do we need this?  Since this isn't built on any of the bots, I think it's
fine to just have a GN target for our development convenience.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
File components/proximity_auth/cryptauth/BUILD.gn (right):

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/BUILD.gn:15: "proto",
Should this be a public dep, to match the export_dependent_settings in the .gyp
file?

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
File components/proximity_auth/cryptauth/cryptauth_client.cc (right):

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:17: // Default rRL of
Google APIs endpoint hosting CryptAuth.
nit: "rRL" -> "URL"

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:18: const char
kDefaultGoogleApisUrl[] = "https://www.googleapis.com";
nit: Perhaps "Url" -> "UrlPrefix"?

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:43: return
google_apis_url.Resolve(std::string(kCryptAuthPath) + request_path +
Optional nit: I don't think the explicit promotion to std::string is necessary,
since request_path is a string.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:61: void
CryptAuthClient::MakeApiCall(
nit: Please move the methods so that they're defined in the .cc file in the same
order as they're declared in the .h file.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:71:
response_is_not_proto);
nit: Why not just let the compiler assert that the expected interface is
available?

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:76:
OnApiCallFailed("Another request in progress");
It looks like this wipes out the error_callback_, and also the flow_, from the
previous request -- that seems like it would cause problems.  In addition to
fixing this, please make sure to add test coverage that would catch a similar
error in the future.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:129: return new
CryptAuthApiCallFlow(request_url);
nit: Seems like this could just be inlined.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:149:
cryptauth::FindEligibleUnlockDevicesResponse>(
nit: I don't think that you have to specify the types explicitly -- I believe
that the compiler can infer them on its own.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
File components/proximity_auth/cryptauth/cryptauth_client.h (right):

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:12: #include
"components/proximity_auth/cryptauth/proto/cryptauth_api.pb.h"
nit: Can you forward-declare the necessary classes, rather than #including the
header?  That would allow us not to make the the proto target a public dep,
which makes the build easier to configure correctly.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:32:
CryptAuthClient(net::URLRequestContextGetter* url_request_context,
It looks like this is stored in a scoped_refptr below, so please pass it as one
as well.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:33:
CryptAuthAccessTokenFetcher* access_token_fetcher);
nit: Please document, including lifetime expectations.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:42: ErrorCallback
error_callback);
nit: Please pass callbacks by const-reference.  (Applies throughout.)

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:49: // the local device.
I wonder if it would be better to just document the calls in the .proto file,
and refer readers of this file to that one.  Having the documentation in just
one place seems like it would lead to a lower likelihood of stale or conflicting
docs.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:73: const std::string
public_key,
nit: Please pass strings by const-reference too (also applies throughout).

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:73: const std::string
public_key,
Since we already reference cryptauth::ToggleEasyUnlockResponse in the callback,
it might be cleaner to simply pass a cryptauth::ToggleEasyUnlockRequest object
to this method, rather than repeating the fields.  Ditto for all of the other
wrapper methods.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:105: virtual
CryptAuthApiCallFlow* CreateFlow(GURL request_url);
nit: Please pass by const-reference.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:115: ErrorCallback
error_callback);
Expanding on a comment above: Why not just expose this method publicly, and
remove all of the specialized methods?

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:133:
scoped_refptr<net::URLRequestContextGetter> url_request_context_;
nit: Please leave blank lines between documented variables and methods. 
(Applies above and below as well.)

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:135:
CryptAuthAccessTokenFetcher* access_token_fetcher_;
nit: Please document lifetime expectations.  Alternately, would it be
appropriate for this class to own the token fetcher, and store it in a
scoped_ptr?

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/sw...
File components/proximity_auth/switches.cc (right):

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/sw...
components/proximity_auth/switches.cc:12: const char kCryptAuthGoogleApisUrl[] =
"cryptauth-google-apis-url";
Is this just for testing use?  Do we need it long-term, or is it something we
need temporarily?  If just temporarily, please annotate with a TODO to remove
it.

[Tim Song, 2014-12-03 01:18:25]

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth.gypi
File components/proximity_auth.gypi (right):

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth.gy...
components/proximity_auth.gypi:87: 'target_name': 'proximity_auth_unittests',
On 2014/11/18 22:30:43, Ilya Sherman wrote:
> Hmm, do we need this?  Since this isn't built on any of the bots, I think it's
> fine to just have a GN target for our development convenience.

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
File components/proximity_auth/cryptauth/BUILD.gn (right):

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/BUILD.gn:15: "proto",
On 2014/11/18 22:30:43, Ilya Sherman wrote:
> Should this be a public dep, to match the export_dependent_settings in the
.gyp
> file?

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
File components/proximity_auth/cryptauth/cryptauth_client.cc (right):

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:17: // Default rRL of
Google APIs endpoint hosting CryptAuth.
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> nit: "rRL" -> "URL"

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:18: const char
kDefaultGoogleApisUrl[] = "https://www.googleapis.com";
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> nit: Perhaps "Url" -> "UrlPrefix"?

Renamed to kDefaultCryptAuthHTTPHost.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:43: return
google_apis_url.Resolve(std::string(kCryptAuthPath) + request_path +
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> Optional nit: I don't think the explicit promotion to std::string is
necessary,
> since request_path is a string.

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:61: void
CryptAuthClient::MakeApiCall(
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> nit: Please move the methods so that they're defined in the .cc file in the
same
> order as they're declared in the .h file.

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:71:
response_is_not_proto);
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> nit: Why not just let the compiler assert that the expected interface is
> available?

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:76:
OnApiCallFailed("Another request in progress");
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> It looks like this wipes out the error_callback_, and also the flow_, from the
> previous request -- that seems like it would cause problems.  In addition to
> fixing this, please make sure to add test coverage that would catch a similar
> error in the future.

Nice catch. I made the interface such that CryptAuthClient only makes one
request and should not be reused. This should help avoid subtle state resetting
bugs.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:129: return new
CryptAuthApiCallFlow(request_url);
On 2014/11/18 22:30:43, Ilya Sherman wrote:
> nit: Seems like this could just be inlined.

I'm overriding this function in the test to inject a mock flow object.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.cc:149:
cryptauth::FindEligibleUnlockDevicesResponse>(
On 2014/11/18 22:30:43, Ilya Sherman wrote:
> nit: I don't think that you have to specify the types explicitly -- I believe
> that the compiler can infer them on its own.

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
File components/proximity_auth/cryptauth/cryptauth_client.h (right):

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:12: #include
"components/proximity_auth/cryptauth/proto/cryptauth_api.pb.h"
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> nit: Can you forward-declare the necessary classes, rather than #including the
> header?  That would allow us not to make the the proto target a public dep,
> which makes the build easier to configure correctly.

You will still have to include cryptauth_api.pb.h for the response messages, so
the proto target will need to be a public dep.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:32:
CryptAuthClient(net::URLRequestContextGetter* url_request_context,
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> It looks like this is stored in a scoped_refptr below, so please pass it as
one
> as well.

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:33:
CryptAuthAccessTokenFetcher* access_token_fetcher);
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> nit: Please document, including lifetime expectations.

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:42: ErrorCallback
error_callback);
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> nit: Please pass callbacks by const-reference.  (Applies throughout.)

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:49: // the local device.
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> I wonder if it would be better to just document the calls in the .proto file,
> and refer readers of this file to that one.  Having the documentation in just
> one place seems like it would lead to a lower likelihood of stale or
conflicting
> docs.

I would prefer the documentation to be here as the proto file is pulled from
google3 and keeping that in sync is problematic by itself. Plus, users will
probably look at this file first rather than the proto.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:73: const std::string
public_key,
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> nit: Please pass strings by const-reference too (also applies throughout).

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:73: const std::string
public_key,
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> Since we already reference cryptauth::ToggleEasyUnlockResponse in the
callback,
> it might be cleaner to simply pass a cryptauth::ToggleEasyUnlockRequest object
> to this method, rather than repeating the fields.  Ditto for all of the other
> wrapper methods.

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:105: virtual
CryptAuthApiCallFlow* CreateFlow(GURL request_url);
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> nit: Please pass by const-reference.

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:115: ErrorCallback
error_callback);
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> Expanding on a comment above: Why not just expose this method publicly, and
> remove all of the specialized methods?

I prefer making all the possible API calls explicitly defined here, so the user
doesn't need to know the URL path for each API call. Another advantage is that
we can easily add tracing, logs, or metrics for each API calls.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:133:
scoped_refptr<net::URLRequestContextGetter> url_request_context_;
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> nit: Please leave blank lines between documented variables and methods. 
> (Applies above and below as well.)

Done.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_client.h:135:
CryptAuthAccessTokenFetcher* access_token_fetcher_;
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> nit: Please document lifetime expectations.  Alternately, would it be
> appropriate for this class to own the token fetcher, and store it in a
> scoped_ptr?

I haven't implemented the access token fetching yet, but I was originally
thinking that it could be reused among multiple clients.

However, thinking about it some more that might make ownership kind of tricky as
there is no good place to put it. I'll just make it a scoped_ptr for now.

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/sw...
File components/proximity_auth/switches.cc (right):

https://codereview.chromium.org/738593002/diff/1/components/proximity_auth/sw...
components/proximity_auth/switches.cc:12: const char kCryptAuthGoogleApisUrl[] =
"cryptauth-google-apis-url";
On 2014/11/18 22:30:44, Ilya Sherman wrote:
> Is this just for testing use?  Do we need it long-term, or is it something we
> need temporarily?  If just temporarily, please annotate with a TODO to remove
> it.

This flag is used in development so we can point to the sandbox/localhost
version of CryptAuth.

Renamed to kCryptAuthHTTPHost.

[Ilya Sherman, 2014-12-03 03:16:37]

Thanks, Tim.  I think the current state of the code is simple enough without a
public template, so I withdraw that concern :)

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/BUILD.gn (right):

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/BUILD.gn:37: ]
I don't remember for sure: Do we need to forward the public_deps here, i.e. list
:cryptauth as a public dep?

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_client.cc (right):

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.cc:158:
OnApiCallFailed("parse error");
Please add test coverage for this code path.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_client.h (right):

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.h:50: // The
|bluetooth_address| field should contain the Bluetooth address of the
I think it's really weird to refer to |bluetooth_address| here, when it isn't
directly declared or referenced in this file.  I really think it would be better
to just have one copy of the documentation, and for that to live in the .proto
file.  It seems implausible to me that clients of this file would have trouble
finding that documentation, but would still be able to use this code, since the
methods require instantiated protobuf objects.

Maybe we can compromise by having high-level docs duplicated both here and in
the .proto file, but have the low-level details be documented only in the .proto
file?  I saw your concern about keeping the .proto file in sync with the server
copy; but we have to do that anyway in order to use it.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.h:93: // enroll the device
with CryptAuth. See for the SetupEnrollment for the
nit: "See for the" -> "See"

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.h:104: // Creates a
CryptAuthApiCallFlow object and takes ownership of it.
nit: I'm not sure what "and takes ownership of it" means here -- really we are
transferring ownership to the caller, right?

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.h:106: virtual
CryptAuthApiCallFlow* CreateFlow(const GURL& request_url);
Please return a scoped_ptr here.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.h:139: // Fetchs the access
token authorizing the API calls.
nit: "Fetchs" -> "Fetches"

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_client_unittest.cc (right):

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:64:
ErrorCallback error_callback));
Hmm, how come the callbacks aren't passed by const-ref?

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:75: //
scoped_ptr.
Hmm, could you clarify how gmock is involved here?  I know that you can't used
scoped_ptrs in mocked methods, but what's the problem with using one in the
constructor?

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:83:
MOCK_METHOD1(CreateFlow, CryptAuthApiCallFlow*(const GURL& request_url));
I'm wondering if the reason you made CreateFlow return a non-scoped_ptr is so
that you could mock the method.  If so, I'd prefer that you create a "wrapper"
method that's mocked, as I've done in other tests -- the needs of test code
shouldn't reduce the clarity of production code, wherever possible.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:98: // static
assert
I don't follow what this comment means -- could you clarify?

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:110:
serialized_request_("") {}
nit: Please prefer std::string() to "".

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:117:
client_.reset(new NiceMock<MockCryptAuthClient>(access_token_fetcher_,
nit: Any reason not to use a StrictMock?  Are there calls to CreateFlow that
some tests aren't interested in?

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:141:
response_proto->SerializeToString(&serialized_proto);
nit: I think you can use "SerializeAsString" to save a line or two of code.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:241: const
std::string kIneligibilityReason = "You require more vespine gas.";
:)

[Tim Song, 2014-12-05 00:00:36]

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/BUILD.gn (right):

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/BUILD.gn:37: ]
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> I don't remember for sure: Do we need to forward the public_deps here, i.e.
list
> :cryptauth as a public dep?

I can build the proximity_auth_unittests target fine. Maybe there is something
special about source_sets, such that they don't count as a true dependency?

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_client.cc (right):

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.cc:158:
OnApiCallFailed("parse error");
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> Please add test coverage for this code path.

Done.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_client.h (right):

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.h:50: // The
|bluetooth_address| field should contain the Bluetooth address of the
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> I think it's really weird to refer to |bluetooth_address| here, when it isn't
> directly declared or referenced in this file.  I really think it would be
better
> to just have one copy of the documentation, and for that to live in the .proto
> file.  It seems implausible to me that clients of this file would have trouble
> finding that documentation, but would still be able to use this code, since
the
> methods require instantiated protobuf objects.
> 
> Maybe we can compromise by having high-level docs duplicated both here and in
> the .proto file, but have the low-level details be documented only in the
.proto
> file?  I saw your concern about keeping the .proto file in sync with the
server
> copy; but we have to do that anyway in order to use it.

I get what you're saying. I removed all the documentation here, and added a
comment to look at the proto file.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.h:93: // enroll the device
with CryptAuth. See for the SetupEnrollment for the
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> nit: "See for the" -> "See"

Done.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.h:104: // Creates a
CryptAuthApiCallFlow object and takes ownership of it.
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> nit: I'm not sure what "and takes ownership of it" means here -- really we are
> transferring ownership to the caller, right?

Because the function is protected, the caller will always be the object itself.
I made this much clearer by making the return value a scoped_ptr like you
suggested.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.h:106: virtual
CryptAuthApiCallFlow* CreateFlow(const GURL& request_url);
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> Please return a scoped_ptr here.

Done.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.h:139: // Fetchs the access
token authorizing the API calls.
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> nit: "Fetchs" -> "Fetches"

Done.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_client_unittest.cc (right):

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:64:
ErrorCallback error_callback));
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> Hmm, how come the callbacks aren't passed by const-ref?

Done.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:75: //
scoped_ptr.
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> Hmm, could you clarify how gmock is involved here?  I know that you can't used
> scoped_ptrs in mocked methods, but what's the problem with using one in the
> constructor?

It's for the same reason I think. The definition of Nice/StrictMock classes in
gmock-generated-nice-strict.h require const references for the templated
arguments passed to constructors. Because scoped_ptr::Pass() is not const, we
can't enforce correct ownership semantics.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:83:
MOCK_METHOD1(CreateFlow, CryptAuthApiCallFlow*(const GURL& request_url));
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> I'm wondering if the reason you made CreateFlow return a non-scoped_ptr is so
> that you could mock the method.  If so, I'd prefer that you create a "wrapper"
> method that's mocked, as I've done in other tests -- the needs of test code
> shouldn't reduce the clarity of production code, wherever possible.

Done.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:98: // static
assert
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> I don't follow what this comment means -- could you clarify?

Sorry, that comment was accidentally left in.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:110:
serialized_request_("") {}
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> nit: Please prefer std::string() to "".

Done.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:117:
client_.reset(new NiceMock<MockCryptAuthClient>(access_token_fetcher_,
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> nit: Any reason not to use a StrictMock?  Are there calls to CreateFlow that
> some tests aren't interested in?

Done.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:141:
response_proto->SerializeToString(&serialized_proto);
On 2014/12/03 03:16:37, Ilya Sherman wrote:
> nit: I think you can use "SerializeAsString" to save a line or two of code.

Done.

[Ilya Sherman, 2014-12-05 00:14:20]

LGTM, thanks.

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_client_unittest.cc (right):

https://codereview.chromium.org/738593002/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:75: //
scoped_ptr.
On 2014/12/05 00:00:36, Tim Song wrote:
> On 2014/12/03 03:16:37, Ilya Sherman wrote:
> > Hmm, could you clarify how gmock is involved here?  I know that you can't
used
> > scoped_ptrs in mocked methods, but what's the problem with using one in the
> > constructor?
> 
> It's for the same reason I think. The definition of Nice/StrictMock classes in
> gmock-generated-nice-strict.h require const references for the templated
> arguments passed to constructors. Because scoped_ptr::Pass() is not const, we
> can't enforce correct ownership semantics. 

Oh, I see, because we wrap this class in a NiceMock/StrictMock.  Makes sense.

https://codereview.chromium.org/738593002/diff/40001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_client.h (right):

https://codereview.chromium.org/738593002/diff/40001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.h:41: // GetMyDevices
Optional nit: Since the remaining comments are identical to the method names, I
think it's fine to drop them.

https://codereview.chromium.org/738593002/diff/40001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_client_unittest.cc (right):

https://codereview.chromium.org/738593002/diff/40001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:426:
flow_result_callback_.Run("Not a valid serialized resposne message.");
nit: "resposne" -> "response"

[Tim Song, 2014-12-05 01:20:40]

https://codereview.chromium.org/738593002/diff/40001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_client.h (right):

https://codereview.chromium.org/738593002/diff/40001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client.h:41: // GetMyDevices
On 2014/12/05 00:14:20, Ilya Sherman wrote:
> Optional nit: Since the remaining comments are identical to the method names,
I
> think it's fine to drop them.

I like the color of the comments. It makes them stand out. :)

https://codereview.chromium.org/738593002/diff/40001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_client_unittest.cc (right):

https://codereview.chromium.org/738593002/diff/40001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:426:
flow_result_callback_.Run("Not a valid serialized resposne message.");
On 2014/12/05 00:14:20, Ilya Sherman wrote:
> nit: "resposne" -> "response"

Done.

[Tim Song, 2014-12-05 18:41:23]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-12-05 18:42:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/738593002/60001

[commit-bot: I haz the power, 2014-12-05 19:31:49]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-05 19:32:36]

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