Remove delegates from ArcAuthCodeFetcher and ArcAuthContext.

Remove delegates from ArcAuthCodeFetcher and ArcAuthContext.

This CL replaces many "delegate" implementation by
callbacks.
Also, now ArcAuthContext's preparation is only for ArcAuthCodeFetcher,
so it is done in the class.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.

Committed: https://crrev.com/e4d65b979e7ccd7834db7d5c3037b1df9091f1d1
Cr-Commit-Position: refs/heads/master@{#432859}

[hidehiko, 2016-11-15 13:21:57]

hidehiko@chromium.org changed reviewers:
+ lhchavez@chromium.org

[hidehiko, 2016-11-15 13:21:58]

Another preparation for ArcAuthService and ArcSessionManager separation. PTAL.

[Luis Héctor Chávez, 2016-11-15 23:27:38]

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc (right):

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc:82:
login_token_request_.reset(
optional: can this be 

login_token_request_ = token_service->StartRequest(account_id, scoped, this);

? It seems very roundabout to do a .reset(...release());

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_code_fetcher.h (right):

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_code_fetcher.h:58: ArcAuthContext*
context_;
nit: ArcAuthContext* const context_;

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_context.cc (left):

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_context.cc:94:
delegate_->OnPrepareContextFailed();
Is this intentionally missing? It _seems_ like it was moved to L89. Both look
weird.

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:413: if (!auth_endpoint.empty())
{
IIUC this can only be true in L411. Is it possible to move this block there?
(possibly even without the condition, because it doesn't make sense to set that
flag to an empty value?)

Furthermore, auth_endpoint can also be declared and initialized in L410.

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_service.h (right):

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.h:252: void
OnAuthCodeObtained(const std::string& auth_code);
Please add comments explaining the difference bewteen these three functions,
since they are easily confused.

[hidehiko, 2016-11-16 01:00:34]

Thank you for review. PTAL.

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc (right):

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc:82:
login_token_request_.reset(
On 2016/11/15 23:27:37, Luis Héctor Chávez wrote:
> optional: can this be 
> 
> login_token_request_ = token_service->StartRequest(account_id, scoped, this);
> 
> ? It seems very roundabout to do a .reset(...release());

Oh, right. Done.

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_code_fetcher.h (right):

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_code_fetcher.h:58: ArcAuthContext*
context_;
On 2016/11/15 23:27:37, Luis Héctor Chávez wrote:
> nit: ArcAuthContext* const context_;

Done.

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_context.cc (left):

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_context.cc:94:
delegate_->OnPrepareContextFailed();
On 2016/11/15 23:27:37, Luis Héctor Chávez wrote:
> Is this intentionally missing? It _seems_ like it was moved to L89. Both look
> weird.

Good catch. unittest should capture this. A following CL will add it.

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:413: if (!auth_endpoint.empty())
{
On 2016/11/15 23:27:37, Luis Héctor Chávez wrote:
> IIUC this can only be true in L411. Is it possible to move this block there?
> (possibly even without the condition, because it doesn't make sense to set
that
> flag to an empty value?)
> 
> Furthermore, auth_endpoint can also be declared and initialized in L410.

Done. Can we work on making L411 DCHECK() in a separate CL, just in case? I'd
like to avoid the risk to revert the CL just for it.

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_service.h (right):

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.h:252: void
OnAuthCodeObtained(const std::string& auth_code);
On 2016/11/15 23:27:37, Luis Héctor Chávez wrote:
> Please add comments explaining the difference bewteen these three functions,
> since they are easily confused.

Done.

[Luis Héctor Chávez, 2016-11-16 03:29:58]

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_auth_service.cc (right):

https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:413: if (!auth_endpoint.empty())
{
On 2016/11/16 01:00:34, hidehiko wrote:
> On 2016/11/15 23:27:37, Luis Héctor Chávez wrote:
> > IIUC this can only be true in L411. Is it possible to move this block there?
> > (possibly even without the condition, because it doesn't make sense to set
> that
> > flag to an empty value?)
> > 
> > Furthermore, auth_endpoint can also be declared and initialized in L410.
> 
> Done. Can we work on making L411 DCHECK() in a separate CL, just in case? I'd
> like to avoid the risk to revert the CL just for it.

sgtm

https://codereview.chromium.org/2498363002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_context.cc (right):

https://codereview.chromium.org/2498363002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context.cc:89:
base::ResetAndReturn(&callback_).Run(nullptr);
I'm still a bit confused as to why this was added. IIUC L88 will eventually call
either OnMergeSession{Success,Failure}, but |callback_| will be null by that
point since you already resolved this as an error.

(Also, if you plan to touch this file soon in the future feel free to ignore
this request, but is is possible to reorder the functions in a way that follows
the logical flow?).

[hidehiko, 2016-11-16 04:09:38]

Thank you for review. PTAL.

Note: shuffling the method is done, too.

https://codereview.chromium.org/2498363002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_auth_context.cc (right):

https://codereview.chromium.org/2498363002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_context.cc:89:
base::ResetAndReturn(&callback_).Run(nullptr);
On 2016/11/16 03:29:58, Luis Héctor Chávez wrote:
> I'm still a bit confused as to why this was added. IIUC L88 will eventually
call
> either OnMergeSession{Success,Failure}, but |callback_| will be null by that
> point since you already resolved this as an error.
> 
> (Also, if you plan to touch this file soon in the future feel free to ignore
> this request, but is is possible to reorder the functions in a way that
follows
> the logical flow?).

Oops... So my mistake was not forgetting L80 callback, but wrongly replace it...
And, testing coverage was not enough... Fixed.

[Luis Héctor Chávez, 2016-11-16 04:11:54]

lgtm

[hidehiko, 2016-11-17 11:59:11]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-11-17 11:59:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2498363002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-17 11:59:19]

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-11-17 13:47:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-17 13:50:33]

Description was changed from

==========
Remove delegates from ArcAuthCodeFetcher and ArcAuthContext.

This CL replaces many "delegate" implementation by
callbacks.
Also, now ArcAuthContext's preparation is only for ArcAuthCodeFetcher,
so it is done in the class.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.
==========

to

==========
Remove delegates from ArcAuthCodeFetcher and ArcAuthContext.

This CL replaces many "delegate" implementation by
callbacks.
Also, now ArcAuthContext's preparation is only for ArcAuthCodeFetcher,
so it is done in the class.

BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.

Committed: https://crrev.com/e4d65b979e7ccd7834db7d5c3037b1df9091f1d1
Cr-Commit-Position: refs/heads/master@{#432859}
==========

[commit-bot: I haz the power, 2016-11-17 13:50:34]

Patchset 5 (id:??) landed as
https://crrev.com/e4d65b979e7ccd7834db7d5c3037b1df9091f1d1
Cr-Commit-Position: refs/heads/master@{#432859}