Refactor CryptAuth component to be more testable.

Refactor CryptAuth component to be more testable.

Classes now implement interfaces, so it is easier to fake/mock them.

BUG=420316
TEST=unit tests pass

Committed: https://crrev.com/4cb782b03a941c849f32f189cee996076813452c
Cr-Commit-Position: refs/heads/master@{#324303}

[Tim Song, 2015-04-04 02:49:51]

Patchset #1 (id:1) has been deleted

[Tim Song, 2015-04-06 18:25:47]

Patchset #1 (id:20001) has been deleted

[Tim Song, 2015-04-06 18:26:25]

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

[Ilya Sherman, 2015-04-06 18:53:34]

I really don't see any concrete improvements to the tests as a result of this
CL.  Is there a motivating example for this change?

TBH, adding an interface for every class seems like unnecessary boilerplate to
me.  But, if we really want to separate out public interfaces from the
implementation details, I'll at least push for consistently having impl.h and
impl.cc files, rather than hiding the concrete classes in anonymous namespaces.

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.cc
(right):

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.cc:52: };
I rather preferred having this as a separate file.  Why did you fold it into the
anonymous namespace here?

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.cc:59: }
nit: Please move this either above or below the class declaration+definition.

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/cryptauth_client_factory.h (right):

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/cryptauth_client_factory.h:24: // making
multiple requests in sequence or parallel easier.
I don't see the advantage of creating an interface that's distinct from the
implementation.  What's the motivation here?

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/cryptauth_client_factory.h:29: virtual
scoped_ptr<CryptAuthClient> CreateInstance() = 0;
Why did you make this virtual?

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/cryptauth_client_impl.h (right):

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/cryptauth_client_impl.h:100: bool
call_started_;
nit: I'd add a "has_" prefix, to emphasize that this is a boolean variable.

[Tim Song, 2015-04-06 23:20:34]

Patchset #3 (id:80001) has been deleted

[Tim Song, 2015-04-06 23:21:17]

I moved the implementation classes to their own files. Please take another look.

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.cc
(right):

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.cc:52: };
On 2015/04/06 18:53:33, Ilya Sherman wrote:
> I rather preferred having this as a separate file.  Why did you fold it into
the
> anonymous namespace here?

Done.

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.cc:59: }
On 2015/04/06 18:53:33, Ilya Sherman wrote:
> nit: Please move this either above or below the class declaration+definition.

Moved definitions to another file.

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/cryptauth_client_factory.h (right):

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/cryptauth_client_factory.h:24: // making
multiple requests in sequence or parallel easier.
On 2015/04/06 18:53:33, Ilya Sherman wrote:
> I don't see the advantage of creating an interface that's distinct from the
> implementation.  What's the motivation here?

Discussed in person.

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/cryptauth_client_factory.h:29: virtual
scoped_ptr<CryptAuthClient> CreateInstance() = 0;
On 2015/04/06 18:53:34, Ilya Sherman wrote:
> Why did you make this virtual?

Discussed in person.

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/cryptauth_client_impl.h (right):

https://codereview.chromium.org/1066453002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/cryptauth_client_impl.h:100: bool
call_started_;
On 2015/04/06 18:53:34, Ilya Sherman wrote:
> nit: I'd add a "has_" prefix, to emphasize that this is a boolean variable.

Done.

[Ilya Sherman, 2015-04-06 23:48:02]

Thanks.

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
File components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.h
(right):

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.h:11:
#include "base/memory/scoped_ptr.h"
nit: Not needed?

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
File
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher_unittest.cc
(right):

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher_unittest.cc:8:
#include
"components/proximity_auth/cryptauth/cryptauth_access_token_fetcher_impl.h"
nit: The tested header file is supposed to be at the top of unittest files.

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher_unittest.cc:36:
scoped_ptr<CryptAuthAccessTokenFetcher> fetcher_;
nit: Does this need to be a scoped_ptr, or can we revert this change?

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
File components/proximity_auth/cryptauth/cryptauth_client.h (right):

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_client.h:8: #include
"base/callback.h"
nit: Can this be callback_forward?

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_client.h:84: // making multiple
requests in sequence or parallel easier.
Optional nit: I'd rephrase "a factory helps making multiple requests in sequence
or parallel easier" as something closer to "a factory makes it easier to make
multiple requests in sequence or in parallel".

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
File components/proximity_auth/cryptauth/cryptauth_client_impl.h (right):

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_client_impl.h:119:
OAuth2TokenService* token_service,
Please document lifetime expectations.  Actually, it would be nice to have some
docs for all of the parameters.

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
File components/proximity_auth/cryptauth/cryptauth_client_unittest.cc (right):

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:9: #include
"components/proximity_auth/cryptauth/cryptauth_client_impl.h"
Here, too, this include should be at the top of the file.

[Tim Song, 2015-04-07 02:22:57]

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
File components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.h
(right):

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.h:11:
#include "base/memory/scoped_ptr.h"
On 2015/04/06 23:48:01, Ilya Sherman wrote:
> nit: Not needed?

Done.

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
File
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher_unittest.cc
(right):

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher_unittest.cc:8:
#include
"components/proximity_auth/cryptauth/cryptauth_access_token_fetcher_impl.h"
On 2015/04/06 23:48:02, Ilya Sherman wrote:
> nit: The tested header file is supposed to be at the top of unittest files.

Done.

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher_unittest.cc:36:
scoped_ptr<CryptAuthAccessTokenFetcher> fetcher_;
On 2015/04/06 23:48:02, Ilya Sherman wrote:
> nit: Does this need to be a scoped_ptr, or can we revert this change?

Done.

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
File components/proximity_auth/cryptauth/cryptauth_client.h (right):

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_client.h:8: #include
"base/callback.h"
On 2015/04/06 23:48:02, Ilya Sherman wrote:
> nit: Can this be callback_forward?

Done.

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_client.h:84: // making multiple
requests in sequence or parallel easier.
On 2015/04/06 23:48:02, Ilya Sherman wrote:
> Optional nit: I'd rephrase "a factory helps making multiple requests in
sequence
> or parallel easier" as something closer to "a factory makes it easier to make
> multiple requests in sequence or in parallel".

Done.

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
File components/proximity_auth/cryptauth/cryptauth_client_impl.h (right):

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_client_impl.h:119:
OAuth2TokenService* token_service,
On 2015/04/06 23:48:02, Ilya Sherman wrote:
> Please document lifetime expectations.  Actually, it would be nice to have
some
> docs for all of the parameters.

Done.

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
File components/proximity_auth/cryptauth/cryptauth_client_unittest.cc (right):

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:9: #include
"components/proximity_auth/cryptauth/cryptauth_client_impl.h"
On 2015/04/06 23:48:02, Ilya Sherman wrote:
> Here, too, this include should be at the top of the file.

Done.

However, this gave presubmit warnings since the name of the *_unittest.cc class
isn't the same as the *.h class.

[Ilya Sherman, 2015-04-07 22:04:50]

LGTM, thanks.  In terms of the remaining comment, I'd prefer to change the
presubmit check to allow this pattern; but if the presubmit check's owners are
opposed, then I guess let's move the header back to the middle of the include
list.  We definitely shouldn't introduce new presubmit warnings for this =)

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
File components/proximity_auth/cryptauth/cryptauth_client_unittest.cc (right):

https://codereview.chromium.org/1066453002/diff/100001/components/proximity_a...
components/proximity_auth/cryptauth/cryptauth_client_unittest.cc:9: #include
"components/proximity_auth/cryptauth/cryptauth_client_impl.h"
On 2015/04/07 02:22:57, Tim Song wrote:
> On 2015/04/06 23:48:02, Ilya Sherman wrote:
> > Here, too, this include should be at the top of the file.
> 
> Done.
> 
> However, this gave presubmit warnings since the name of the *_unittest.cc
class
> isn't the same as the *.h class.

Hmm.  I think it's reasonable to update the presubmit check to allow "foo_impl"
to be tested by "foo_unittest".  WDYT?

[Tim Song, 2015-04-08 02:30:00]

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

[Tim Song, 2015-04-08 02:30:01]

+jochen@ for PRESUBMIT.py

What do you think of allowing a file with name file_*test.cc to allow including
file_impl.h as the special first include?

[jochen (gone - plz use gerrit), 2015-04-08 15:20:49]

On 2015/04/08 at 02:30:01, tengs wrote:
> +jochen@ for PRESUBMIT.py
> 
> What do you think of allowing a file with name file_*test.cc to allow
including file_impl.h as the special first include?

it's more common to call the test _impl_unittest.cc

[Tim Song, 2015-04-08 17:49:20]

Patchset #6 (id:160001) has been deleted

[Tim Song, 2015-04-08 17:51:09]

tengs@chromium.org changed reviewers:
- jochen@chromium.org

[Tim Song, 2015-04-08 17:51:09]

Okay, I renamed the tests and removed the PRESUBMIT.py changes.

[Tim Song, 2015-04-08 21:23:48]

The CQ bit was checked by tengs@chromium.org

[Tim Song, 2015-04-08 21:23:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1066453002/#ps180001
(title: "rename tests")

[commit-bot: I haz the power, 2015-04-08 21:24:13]

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

[commit-bot: I haz the power, 2015-04-08 23:07:49]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2015-04-08 23:08:52]

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