arc: Shuffle ArcAuthService's interface

arc: Shuffle ArcAuthService's interface

This change introduces a new method that will provide ARC with all the
information it needs to completely sign in and provision itself,
reducing the number of round trips needed and allowing us to restructure
ARC's code in a much simpler way.

This change also now assumes that OnSignInComplete/OnSignInFailed can be
called at any time (and in fact the former will be called every time ARC
boots).

BUG=657687
TEST=ARC still boots

Committed: https://crrev.com/6bba4da10ab15b7ad7b8c1bda0cf04f1fa20d31f
Cr-Commit-Position: refs/heads/master@{#429909}

[Luis Héctor Chávez, 2016-11-02 21:23:49]

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

[commit-bot: I haz the power, 2016-11-02 21:24:32]

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

[Luis Héctor Chávez, 2016-11-02 21:56:13]

lhchavez@chromium.org changed reviewers:
+ hidehiko@chromium.org, khmel@chromium.org, rickyz@chromium.org

[Luis Héctor Chávez, 2016-11-02 21:56:14]

PTAL

hidehiko: all
rickyz: *.mojom
khmel: FYI

[hidehiko, 2016-11-03 00:27:21]

lgtm.

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

https://codereview.chromium.org/2474663003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:392:
profile_->GetPrefs()->SetBoolean(prefs::kArcSignedIn, true);
Let's move this to L386.

https://codereview.chromium.org/2474663003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:504: RemoveArcData();
Slightly off-topic. Is it ok to remove the data for re-auth case...?

https://codereview.chromium.org/2474663003/diff/20001/components/arc/common/a...
File components/arc/common/auth.mojom (right):

https://codereview.chromium.org/2474663003/diff/20001/components/arc/common/a...
components/arc/common/auth.mojom:80: struct AccountInformation {
(optional): Some arc code uses "Info" abbr. Are you interested in to align it?

[Luis Héctor Chávez, 2016-11-03 20:08:09]

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

https://codereview.chromium.org/2474663003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:392:
profile_->GetPrefs()->SetBoolean(prefs::kArcSignedIn, true);
On 2016/11/03 00:27:21, hidehiko wrote:
> Let's move this to L386.

Done.

https://codereview.chromium.org/2474663003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:504: RemoveArcData();
On 2016/11/03 00:27:21, hidehiko wrote:
> Slightly off-topic. Is it ok to remove the data for re-auth case...?

If re-auth needs provisioning, definitely. Otherwise maybe not.

The good news is that we can maybe remove the sign in timeout case in another
change :D

https://codereview.chromium.org/2474663003/diff/20001/components/arc/common/a...
File components/arc/common/auth.mojom (right):

https://codereview.chromium.org/2474663003/diff/20001/components/arc/common/a...
components/arc/common/auth.mojom:80: struct AccountInformation {
On 2016/11/03 00:27:21, hidehiko wrote:
> (optional): Some arc code uses "Info" abbr. Are you interested in to align it?

$ git grep -P '(struct|class).*Info\b' -- '*.h' | wc -l
1655
$ git grep -P '(struct|class).*Information\b' -- '*.h' | wc -l
45

I guess I should.

[khmel, 2016-11-03 20:16:45]

lgtm

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

https://codereview.chromium.org/2474663003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:118: class
ArcAuthService::AccountInfoNotifier {
nit: May be move to separate class to improve readability? I am OK with this too
also OK with TODO.

https://codereview.chromium.org/2474663003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:413: if
(profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn))
Is it guaranty that timers are not set?

nit: maybe DCHECK(sign_in_time_.is_null());
           DCHECK(arc_sign_in_timer_.is_null()); ?

[rickyz (no longer on Chrome), 2016-11-04 03:07:53]

mojom lgtm

So much legacy and deprecation, didn't ARC++ just launch recently? :-P

[Luis Héctor Chávez, 2016-11-04 15:43:13]

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

https://codereview.chromium.org/2474663003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:118: class
ArcAuthService::AccountInfoNotifier {
On 2016/11/03 20:16:45, khmel wrote:
> nit: May be move to separate class to improve readability? I am OK with this
too
> also OK with TODO.

Actually my goal is to eventually deprecate the old interfaces, so I'll add the
TODO to remove this instead of moving it out.

https://codereview.chromium.org/2474663003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:413: if
(profile_->GetPrefs()->GetBoolean(prefs::kArcSignedIn))
On 2016/11/03 20:16:45, khmel wrote:
> Is it guaranty that timers are not set?
> 
> nit: maybe DCHECK(sign_in_time_.is_null());
>            DCHECK(arc_sign_in_timer_.is_null()); ?

It should be guaranteed, but after more careful consideration, I think it's
better if this function mirrors OnSignInFailedInternal, so I'll use the same

if (!sign_in_time_.is_null()) {
  arc_sign_in_timer_.Stop();
  // update UMA
}

[Luis Héctor Chávez, 2016-11-04 15:43:17]

The CQ bit was checked by lhchavez@chromium.org

[Luis Héctor Chávez, 2016-11-04 15:43:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org, rickyz@chromium.org, khmel@chromium.org
Link to the patchset: https://codereview.chromium.org/2474663003/#ps60001
(title: "Final nits")

[commit-bot: I haz the power, 2016-11-04 15:43:44]

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-04 16:34:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-04 16:41:27]

Description was changed from

==========
arc: Shuffle ArcAuthService's interface

This change introduces a new method that will provide ARC with all the
information it needs to completely sign in and provision itself,
reducing the number of round trips needed and allowing us to restructure
ARC's code in a much simpler way.

This change also now assumes that OnSignInComplete/OnSignInFailed can be
called at any time (and in fact the former will be called every time ARC
boots).

BUG=657687
TEST=ARC still boots
==========

to

==========
arc: Shuffle ArcAuthService's interface

This change introduces a new method that will provide ARC with all the
information it needs to completely sign in and provision itself,
reducing the number of round trips needed and allowing us to restructure
ARC's code in a much simpler way.

This change also now assumes that OnSignInComplete/OnSignInFailed can be
called at any time (and in fact the former will be called every time ARC
boots).

BUG=657687
TEST=ARC still boots

Committed: https://crrev.com/6bba4da10ab15b7ad7b8c1bda0cf04f1fa20d31f
Cr-Commit-Position: refs/heads/master@{#429909}
==========

[commit-bot: I haz the power, 2016-11-04 16:41:28]

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