arc: Restore broken auth code requst on demand.

arc: Restore broken auth code request on demand.

This feature is required when Android needs re-authorization
or in new OOBE Arc Terms integration.

BUG=b/32124104
TEST=Manually on device during the testing OOBE Arc Terms

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

[khmel, 2016-10-12 23:46:28]

khmel@chromium.org changed reviewers:
+ hidehiko@chromium.org, xiyuan@chromium.org

[khmel, 2016-10-12 23:46:28]

Hi Xiyuan and Hidehiko,

PTAL

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

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:821:
IDS_ARC_SIGN_IN_SERVICE_UNAVAILABLE_ERROR));
This error has nothing with "Couldn't reach Google Play. Try again now. " and
may be very confusing while analyzing user reports. Moreover it is hard to image
how it is possible reach this code. If user re-enable Arc, this service passes
through Stopped state and this means stopping the bridge. I would probably
change this to NOTREACHED().

[xiyuan, 2016-10-13 00:34:10]

lgtm

requst -> request in cl description

[khmel, 2016-10-13 15:16:20]

Description was changed from

==========
arc: Restore broken auth code requst on demand.

This feature is required when Android needs re-authorization
or in new OOBE Arc Terms integration.

BUG=b/32124104
TEST=Manually on device during the testing OOBE Arc Terms
==========

to

==========
arc: Restore broken auth code request on demand.

This feature is required when Android needs re-authorization
or in new OOBE Arc Terms integration.

BUG=b/32124104
TEST=Manually on device during the testing OOBE Arc Terms
==========

[khmel, 2016-10-13 15:16:25]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2016-10-13 15:16:42]

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

[hidehiko, 2016-10-13 15:22:00]

Please wait for submit.

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

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:818: void
ArcAuthService::StartUI() {
StartUI is called only from four places.
- GetAuthCode(AndAccountType).
- OnOptInPreferenceChanged.
- StartLso.

My understanding is;
- GetAuthCode(AndAccountType) is for re-sign in for some reason.
- OnOptInPreferenceChanged is the first ARC start up procedure.

However, StartLso sounds wierd because it is called only from ArcSupportHost's
onAgree event, which means the UI (= extension) should be already started at the
moment.
Thus, it should be somehow cleaned up and get rid of StartUI(). Then, the check
you added at L821 is unnecessary, because that if-statment can be moved to the
OnOptInPreferenceChanged.

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:911: if (state_ == State::ACTIVE
&& !IsAuthCodeRequest())
My understanding is that; ArcAuthService shows an extension UI if;
- this is the first time the user starts ARC on the device, or
- For some reason (what's the case, BTW?), re-sign in is requested from ARC.

Is there any other cases?
If these are *only* two, the condition looks slightly weird to me because;
- For the first case, ARC should still be in stopped state.
- For the second case, ARC should be running, but may be stopped due to, e.g.,
crash.

Then, what's the case if ARC is running, but not in the re-sign in flow?

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:914: if (!IsAuthCodeRequest() &&
In this case, kArcSignedIn should be set to false already, shouldn't it?
IIUC, IsAuthCodeRequest() case is the re-sign in request from the ARC. It sounds
the real sign-in condition and persistent kArcSignedIn preference is
conflicting?

[hidehiko, 2016-10-13 15:22:27]

Sorry, but let me uncheck the commit, now...

[hidehiko, 2016-10-13 15:22:31]

The CQ bit was unchecked by hidehiko@chromium.org

[khmel, 2016-10-13 15:30:19]

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

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:818: void
ArcAuthService::StartUI() {
On 2016/10/13 15:22:00, hidehiko wrote:
> StartUI is called only from four places.
> - GetAuthCode(AndAccountType).
> - OnOptInPreferenceChanged.
> - StartLso.
> 
> My understanding is;
> - GetAuthCode(AndAccountType) is for re-sign in for some reason.
> - OnOptInPreferenceChanged is the first ARC start up procedure.
> 
> However, StartLso sounds wierd because it is called only from ArcSupportHost's
> onAgree event, which means the UI (= extension) should be already started at
the
> moment.
> Thus, it should be somehow cleaned up and get rid of StartUI(). Then, the
check
> you added at L821 is unnecessary, because that if-statment can be moved to the
> OnOptInPreferenceChanged.

Checks I added is real condition that prevents Android side for re-authenticate.
Re-auth request is called when Android is running and state is ACTIVE

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:911: if (state_ == State::ACTIVE
&& !IsAuthCodeRequest())
On 2016/10/13 15:22:00, hidehiko wrote:
> My understanding is that; ArcAuthService shows an extension UI if;
> - this is the first time the user starts ARC on the device, or
> - For some reason (what's the case, BTW?), re-sign in is requested from ARC.
> 
> Is there any other cases?
> If these are *only* two, the condition looks slightly weird to me because;
> - For the first case, ARC should still be in stopped state.
> - For the second case, ARC should be running, but may be stopped due to, e.g.,
> crash.
> 
> Then, what's the case if ARC is running, but not in the re-sign in flow?

It can be requested when Arc is running on second or next boot and Android needs
re-auth or authenticate. In the last case, state_ is active.

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:914: if (!IsAuthCodeRequest() &&
On 2016/10/13 15:22:00, hidehiko wrote:
> In this case, kArcSignedIn should be set to false already, shouldn't it?
> IIUC, IsAuthCodeRequest() case is the re-sign in request from the ARC. It
sounds
> the real sign-in condition and persistent kArcSignedIn preference is
> conflicting?

If AuthCodeRequest is set then kArcSignedIn may be on or off, there is no
requirements here for it. GetAuthCode can be called at any time, during first
boot or on consequence boots. There is also one more flag added here to support
the case OOBE integration.

[hidehiko, 2016-10-13 15:44:32]

Thank you for quick reply!

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

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:818: void
ArcAuthService::StartUI() {
On 2016/10/13 15:30:19, khmel wrote:
> On 2016/10/13 15:22:00, hidehiko wrote:
> > StartUI is called only from four places.
> > - GetAuthCode(AndAccountType).
> > - OnOptInPreferenceChanged.
> > - StartLso.
> > 
> > My understanding is;
> > - GetAuthCode(AndAccountType) is for re-sign in for some reason.
> > - OnOptInPreferenceChanged is the first ARC start up procedure.
> > 
> > However, StartLso sounds wierd because it is called only from
ArcSupportHost's
> > onAgree event, which means the UI (= extension) should be already started at
> the
> > moment.
> > Thus, it should be somehow cleaned up and get rid of StartUI(). Then, the
> check
> > you added at L821 is unnecessary, because that if-statment can be moved to
the
> > OnOptInPreferenceChanged.
> 
> Checks I added is real condition that prevents Android side for
re-authenticate.
> Re-auth request is called when Android is running and state is ACTIVE 

I meant;
- StartLso() should not use StartUI() conceptually because when StartLso() is
called the UI (= extension) is already started.
- Then, IsAuthCodeRequest() check is unnecessary to be here (or I'd say it looks
no-longer the part of StartUI() considering re-auth case), so you should move
the check to the caller (OnOptInPreferenceChanged()).

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:911: if (state_ == State::ACTIVE
&& !IsAuthCodeRequest())
On 2016/10/13 15:30:19, khmel wrote:
> On 2016/10/13 15:22:00, hidehiko wrote:
> > My understanding is that; ArcAuthService shows an extension UI if;
> > - this is the first time the user starts ARC on the device, or
> > - For some reason (what's the case, BTW?), re-sign in is requested from ARC.
> > 
> > Is there any other cases?
> > If these are *only* two, the condition looks slightly weird to me because;
> > - For the first case, ARC should still be in stopped state.
> > - For the second case, ARC should be running, but may be stopped due to,
e.g.,
> > crash.
> > 
> > Then, what's the case if ARC is running, but not in the re-sign in flow?
> 
> It can be requested when Arc is running on second or next boot and Android
needs
> re-auth or authenticate. In the last case, state_ is active.

I understand |state_| can be ACTIVE in re-auth case.
My question is, what is the case where |state_| is ACTIVE but *not* in re-auth
case?

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:914: if (!IsAuthCodeRequest() &&
On 2016/10/13 15:30:19, khmel wrote:
> On 2016/10/13 15:22:00, hidehiko wrote:
> > In this case, kArcSignedIn should be set to false already, shouldn't it?
> > IIUC, IsAuthCodeRequest() case is the re-sign in request from the ARC. It
> sounds
> > the real sign-in condition and persistent kArcSignedIn preference is
> > conflicting?
> 
> If AuthCodeRequest is set then kArcSignedIn may be on or off, there is no
> requirements here for it. GetAuthCode can be called at any time, during first
> boot or on consequence boots. There is also one more flag added here to
support
> the case OOBE integration.

Isn't kArcSignedIn represents the ARC container sign-in status?
If yes, and re-auth request means that the container is failed to sign-in, the
preference should be set to false to keep the state in sync, at the first of
re-auth request.

[khmel, 2016-10-13 15:53:21]

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

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:818: void
ArcAuthService::StartUI() {
On 2016/10/13 15:44:32, hidehiko wrote:
> On 2016/10/13 15:30:19, khmel wrote:
> > On 2016/10/13 15:22:00, hidehiko wrote:
> > > StartUI is called only from four places.
> > > - GetAuthCode(AndAccountType).
> > > - OnOptInPreferenceChanged.
> > > - StartLso.
> > > 
> > > My understanding is;
> > > - GetAuthCode(AndAccountType) is for re-sign in for some reason.
> > > - OnOptInPreferenceChanged is the first ARC start up procedure.
> > > 
> > > However, StartLso sounds wierd because it is called only from
> ArcSupportHost's
> > > onAgree event, which means the UI (= extension) should be already started
at
> > the
> > > moment.
> > > Thus, it should be somehow cleaned up and get rid of StartUI(). Then, the
> > check
> > > you added at L821 is unnecessary, because that if-statment can be moved to
> the
> > > OnOptInPreferenceChanged.
> > 
> > Checks I added is real condition that prevents Android side for
> re-authenticate.
> > Re-auth request is called when Android is running and state is ACTIVE 
> 
> I meant;
> - StartLso() should not use StartUI() conceptually because when StartLso() is
> called the UI (= extension) is already started.
> - Then, IsAuthCodeRequest() check is unnecessary to be here (or I'd say it
looks
> no-longer the part of StartUI() considering re-auth case), so you should move
> the check to the caller (OnOptInPreferenceChanged()).

This is real case that blocks working re-auth process. I don't touch StartLso
code at all. StartUI is called from GetAuthCode. At this point UI is not shown
and I see this error.

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:911: if (state_ == State::ACTIVE
&& !IsAuthCodeRequest())
On 2016/10/13 15:44:31, hidehiko wrote:
> On 2016/10/13 15:30:19, khmel wrote:
> > On 2016/10/13 15:22:00, hidehiko wrote:
> > > My understanding is that; ArcAuthService shows an extension UI if;
> > > - this is the first time the user starts ARC on the device, or
> > > - For some reason (what's the case, BTW?), re-sign in is requested from
ARC.
> > > 
> > > Is there any other cases?
> > > If these are *only* two, the condition looks slightly weird to me because;
> > > - For the first case, ARC should still be in stopped state.
> > > - For the second case, ARC should be running, but may be stopped due to,
> e.g.,
> > > crash.
> > > 
> > > Then, what's the case if ARC is running, but not in the re-sign in flow?
> > 
> > It can be requested when Arc is running on second or next boot and Android
> needs
> > re-auth or authenticate. In the last case, state_ is active.
> 
> I understand |state_| can be ACTIVE in re-auth case.
> My question is, what is the case where |state_| is ACTIVE but *not* in re-auth
> case?

I don't see such case. I only see case with re-auth request. Note,
StartArcIfSigned probably not the best function name in this case.

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:914: if (!IsAuthCodeRequest() &&
On 2016/10/13 15:44:32, hidehiko wrote:
> On 2016/10/13 15:30:19, khmel wrote:
> > On 2016/10/13 15:22:00, hidehiko wrote:
> > > In this case, kArcSignedIn should be set to false already, shouldn't it?
> > > IIUC, IsAuthCodeRequest() case is the re-sign in request from the ARC. It
> > sounds
> > > the real sign-in condition and persistent kArcSignedIn preference is
> > > conflicting?
> > 
> > If AuthCodeRequest is set then kArcSignedIn may be on or off, there is no
> > requirements here for it. GetAuthCode can be called at any time, during
first
> > boot or on consequence boots. There is also one more flag added here to
> support
> > the case OOBE integration.
> 
> Isn't kArcSignedIn represents the ARC container sign-in status?
> If yes, and re-auth request means that the container is failed to sign-in, the
> preference should be set to false to keep the state in sync, at the first of
> re-auth request.

Re-auth may occur at any moment, with or without SignedIn status. Please note
that we might have flag skip auth. In this case SignedIn status would be set
without actual signing on Android side. You may consider SignedIn state as one
successful run of Android

[Yusuke Sato, 2016-10-15 01:16:31]

yusukes@chromium.org changed reviewers:
+ yusukes@chromium.org

[Yusuke Sato, 2016-10-15 01:16:32]

drive-by:

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

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:818: void
ArcAuthService::StartUI() {
On 2016/10/13 15:53:21, khmel wrote:
> On 2016/10/13 15:44:32, hidehiko wrote:
> > On 2016/10/13 15:30:19, khmel wrote:
> > > On 2016/10/13 15:22:00, hidehiko wrote:
> > > > StartUI is called only from four places.
> > > > - GetAuthCode(AndAccountType).
> > > > - OnOptInPreferenceChanged.
> > > > - StartLso.
> > > > 
> > > > My understanding is;
> > > > - GetAuthCode(AndAccountType) is for re-sign in for some reason.
> > > > - OnOptInPreferenceChanged is the first ARC start up procedure.
> > > > 
> > > > However, StartLso sounds wierd because it is called only from
> > ArcSupportHost's
> > > > onAgree event, which means the UI (= extension) should be already
started
> at
> > > the
> > > > moment.
> > > > Thus, it should be somehow cleaned up and get rid of StartUI(). Then,
the
> > > check
> > > > you added at L821 is unnecessary, because that if-statment can be moved
to
> > the
> > > > OnOptInPreferenceChanged.
> > > 
> > > Checks I added is real condition that prevents Android side for
> > re-authenticate.
> > > Re-auth request is called when Android is running and state is ACTIVE 
> > 
> > I meant;
> > - StartLso() should not use StartUI() conceptually because when StartLso()
is
> > called the UI (= extension) is already started.
> > - Then, IsAuthCodeRequest() check is unnecessary to be here (or I'd say it
> looks
> > no-longer the part of StartUI() considering re-auth case), so you should
move
> > the check to the caller (OnOptInPreferenceChanged()).
> 
> This is real case that blocks working re-auth process. I don't touch StartLso
> code at all. StartUI is called from GetAuthCode. At this point UI is not shown
> and I see this error.

I'm wondering what's your request now based on the info Yury gave in the
previous comment. Split StartUI() into two (line 819-827 and 828-836), revert
line 821, and let GetAuthCode[AndAccountType] only call the latter, maybe?

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:911: if (state_ == State::ACTIVE
&& !IsAuthCodeRequest())
On 2016/10/13 15:53:21, khmel wrote:
> On 2016/10/13 15:44:31, hidehiko wrote:
> > On 2016/10/13 15:30:19, khmel wrote:
> > > On 2016/10/13 15:22:00, hidehiko wrote:
> > > > My understanding is that; ArcAuthService shows an extension UI if;
> > > > - this is the first time the user starts ARC on the device, or
> > > > - For some reason (what's the case, BTW?), re-sign in is requested from
> ARC.
> > > > 
> > > > Is there any other cases?
> > > > If these are *only* two, the condition looks slightly weird to me
because;
> > > > - For the first case, ARC should still be in stopped state.
> > > > - For the second case, ARC should be running, but may be stopped due to,
> > e.g.,
> > > > crash.
> > > > 
> > > > Then, what's the case if ARC is running, but not in the re-sign in flow?
> > > 
> > > It can be requested when Arc is running on second or next boot and Android
> > needs
> > > re-auth or authenticate. In the last case, state_ is active.
> > 
> > I understand |state_| can be ACTIVE in re-auth case.
> > My question is, what is the case where |state_| is ACTIVE but *not* in
re-auth
> > case?
> 
> I don't see such case. I only see case with re-auth request. Note,
> StartArcIfSigned probably not the best function name in this case.

Then we can revert line 911 to the original one?

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:914: if (!IsAuthCodeRequest() &&
On 2016/10/13 15:53:21, khmel wrote:
> On 2016/10/13 15:44:32, hidehiko wrote:
> > On 2016/10/13 15:30:19, khmel wrote:
> > > On 2016/10/13 15:22:00, hidehiko wrote:
> > > > In this case, kArcSignedIn should be set to false already, shouldn't it?
> > > > IIUC, IsAuthCodeRequest() case is the re-sign in request from the ARC.
It
> > > sounds
> > > > the real sign-in condition and persistent kArcSignedIn preference is
> > > > conflicting?
> > > 
> > > If AuthCodeRequest is set then kArcSignedIn may be on or off, there is no
> > > requirements here for it. GetAuthCode can be called at any time, during
> first
> > > boot or on consequence boots. There is also one more flag added here to
> > support
> > > the case OOBE integration.
> > 
> > Isn't kArcSignedIn represents the ARC container sign-in status?
> > If yes, and re-auth request means that the container is failed to sign-in,
the
> > preference should be set to false to keep the state in sync, at the first of
> > re-auth request.
> 
> Re-auth may occur at any moment, with or without SignedIn status. Please note
> that we might have flag skip auth. In this case SignedIn status would be set
> without actual signing on Android side. You may consider SignedIn state as one
> successful run of Android

Ok, so we don't have to fix kArcSignedIn behavior at least in this CL, correct?
Assuming so, I guess hidehiko's overall concern on this CL is that changing
functions' behavior depending on the auth_*callback_ state (null or not) is
brittle and may incur a maintenance burden. I kind of agree with that.

Given that we can remove all other IsAuthCodeRequest() now (I hope), is there a
not-too-complicated-and-M55-merge-able way to get rid of the remaining
IsAuthCodeRequest() call at line 914?

My (somewhat uninformed) guess is that the function is called only with either
the following paths:

 GetAuthCode*()
   ...snip...
     OnContextReady()
       CheckAndroidManagement() with |background_mode| == false
         StartArcIfSignedIn()

 -or-

 GetAuthCode*()
   ...snip...
     OnContextReady()
       CheckAndroidManagement() with |background_mode| == false
         ArcAndroidManagementChecker()'s ctor
           ...snip...
             StartArcIfSignedIn()

correct? And |background_mode| indicates if it's re-auth or not. If so, can we
propagate |background_mode| to StartArcIfSignedIn() so that the function can
know whether or not it's called during re-auth? WDYT? Does this address your
concern?

[hidehiko, 2016-10-17 09:23:31]

+cc: Luis, as he and I'm cleaning up ARC start up procedure.

Thank you very much for jumping in, Yusuke.
Your review helps us a lot.

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

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:818: void
ArcAuthService::StartUI() {
On 2016/10/15 01:16:32, Yusuke Sato wrote:
> On 2016/10/13 15:53:21, khmel wrote:
> > On 2016/10/13 15:44:32, hidehiko wrote:
> > > On 2016/10/13 15:30:19, khmel wrote:
> > > > On 2016/10/13 15:22:00, hidehiko wrote:
> > > > > StartUI is called only from four places.
> > > > > - GetAuthCode(AndAccountType).
> > > > > - OnOptInPreferenceChanged.
> > > > > - StartLso.
> > > > > 
> > > > > My understanding is;
> > > > > - GetAuthCode(AndAccountType) is for re-sign in for some reason.
> > > > > - OnOptInPreferenceChanged is the first ARC start up procedure.
> > > > > 
> > > > > However, StartLso sounds wierd because it is called only from
> > > ArcSupportHost's
> > > > > onAgree event, which means the UI (= extension) should be already
> started
> > at
> > > > the
> > > > > moment.
> > > > > Thus, it should be somehow cleaned up and get rid of StartUI(). Then,
> the
> > > > check
> > > > > you added at L821 is unnecessary, because that if-statment can be
moved
> to
> > > the
> > > > > OnOptInPreferenceChanged.
> > > > 
> > > > Checks I added is real condition that prevents Android side for
> > > re-authenticate.
> > > > Re-auth request is called when Android is running and state is ACTIVE 
> > > 
> > > I meant;
> > > - StartLso() should not use StartUI() conceptually because when StartLso()
> is
> > > called the UI (= extension) is already started.
> > > - Then, IsAuthCodeRequest() check is unnecessary to be here (or I'd say it
> > looks
> > > no-longer the part of StartUI() considering re-auth case), so you should
> move
> > > the check to the caller (OnOptInPreferenceChanged()).
> > 
> > This is real case that blocks working re-auth process. I don't touch
StartLso
> > code at all. StartUI is called from GetAuthCode. At this point UI is not
shown
> > and I see this error.
> 
> I'm wondering what's your request now based on the info Yury gave in the
> previous comment. Split StartUI() into two (line 819-827 and 828-836), revert
> line 821, and let GetAuthCode[AndAccountType] only call the latter, maybe?

Or, maybe introducing two functions.

void StartSupportExtension() {  // or any better name?
  if (arc is stopped()) {
    // show error.
    return;
  }
  SetState(State::FETCHING_CODE);
  ShowUI(UIPage::TERMS_PROGRESS, ...);
}

void PrepareAuthContext() {  // Any better name... ArcAuthContext looks not a
context, actually???
  SetState(State::FETCHING_CODE);  // Is this and starting extension in really
the same state...?
  context_->PrepareContext();
}

And call necessary ones for each.
// Note: More TODOs are there on those functions still, but I dropped them from
this comment, because it looks out of this CL's focus, I think.

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:911: if (state_ == State::ACTIVE
&& !IsAuthCodeRequest())
On 2016/10/15 01:16:32, Yusuke Sato wrote:
> On 2016/10/13 15:53:21, khmel wrote:
> > On 2016/10/13 15:44:31, hidehiko wrote:
> > > On 2016/10/13 15:30:19, khmel wrote:
> > > > On 2016/10/13 15:22:00, hidehiko wrote:
> > > > > My understanding is that; ArcAuthService shows an extension UI if;
> > > > > - this is the first time the user starts ARC on the device, or
> > > > > - For some reason (what's the case, BTW?), re-sign in is requested
from
> > ARC.
> > > > > 
> > > > > Is there any other cases?
> > > > > If these are *only* two, the condition looks slightly weird to me
> because;
> > > > > - For the first case, ARC should still be in stopped state.
> > > > > - For the second case, ARC should be running, but may be stopped due
to,
> > > e.g.,
> > > > > crash.
> > > > > 
> > > > > Then, what's the case if ARC is running, but not in the re-sign in
flow?
> > > > 
> > > > It can be requested when Arc is running on second or next boot and
Android
> > > needs
> > > > re-auth or authenticate. In the last case, state_ is active.
> > > 
> > > I understand |state_| can be ACTIVE in re-auth case.
> > > My question is, what is the case where |state_| is ACTIVE but *not* in
> re-auth
> > > case?
> > 
> > I don't see such case. I only see case with re-auth request. Note,
> > StartArcIfSigned probably not the best function name in this case.
> 
> Then we can revert line 911 to the original one?

Reverting will cause a problem, because the new code actually would like not to
return if IsAuthCodeRequest() == true even if state_ == ACTIVE.
Maybe remove entirely instead, and D?CHECK etc. with descriptive comments, if
there is no case that state_ == ACTIVE && !IsAuthCodeRequest()?

https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:914: if (!IsAuthCodeRequest() &&
On 2016/10/15 01:16:32, Yusuke Sato wrote:
> On 2016/10/13 15:53:21, khmel wrote:
> > On 2016/10/13 15:44:32, hidehiko wrote:
> > > On 2016/10/13 15:30:19, khmel wrote:
> > > > On 2016/10/13 15:22:00, hidehiko wrote:
> > > > > In this case, kArcSignedIn should be set to false already, shouldn't
it?
> > > > > IIUC, IsAuthCodeRequest() case is the re-sign in request from the ARC.
> It
> > > > sounds
> > > > > the real sign-in condition and persistent kArcSignedIn preference is
> > > > > conflicting?
> > > > 
> > > > If AuthCodeRequest is set then kArcSignedIn may be on or off, there is
no
> > > > requirements here for it. GetAuthCode can be called at any time, during
> > first
> > > > boot or on consequence boots. There is also one more flag added here to
> > > support
> > > > the case OOBE integration.
> > > 
> > > Isn't kArcSignedIn represents the ARC container sign-in status?
> > > If yes, and re-auth request means that the container is failed to sign-in,
> the
> > > preference should be set to false to keep the state in sync, at the first
of
> > > re-auth request.
> > 
> > Re-auth may occur at any moment, with or without SignedIn status. Please
note
> > that we might have flag skip auth. In this case SignedIn status would be set
> > without actual signing on Android side. You may consider SignedIn state as
one
> > successful run of Android
> 
> Ok, so we don't have to fix kArcSignedIn behavior at least in this CL,
correct?
> Assuming so, I guess hidehiko's overall concern on this CL is that changing
> functions' behavior depending on the auth_*callback_ state (null or not) is
> brittle and may incur a maintenance burden. I kind of agree with that.
> 
> Given that we can remove all other IsAuthCodeRequest() now (I hope), is there
a
> not-too-complicated-and-M55-merge-able way to get rid of the remaining
> IsAuthCodeRequest() call at line 914?
> 
> My (somewhat uninformed) guess is that the function is called only with either
> the following paths:
> 
>  GetAuthCode*()
>    ...snip...
>      OnContextReady()
>        CheckAndroidManagement() with |background_mode| == false
>          StartArcIfSignedIn()
> 
>  -or-
> 
>  GetAuthCode*()
>    ...snip...
>      OnContextReady()
>        CheckAndroidManagement() with |background_mode| == false
>          ArcAndroidManagementChecker()'s ctor
>            ...snip...
>              StartArcIfSignedIn()
> 
> correct? And |background_mode| indicates if it's re-auth or not. If so, can we
> propagate |background_mode| to StartArcIfSignedIn() so that the function can
> know whether or not it's called during re-auth? WDYT? Does this address your
> concern?
> 

https://cs.chromium.org/chromium/src/chrome/common/pref_names.cc?q=kArcSigned...

explicitly says it is corresponding to the Android's sign in state. So, if the
semantics of the value is changed, it should be updated. Though, I would suggest
not to change the semantics, because the value is persistent and old values can
be inherited, so the Chrome update would cause a trouble I think.

If the semantics is kept as is, we need to set the value to false at the
beginning of GetAuthCode*()?

BTW, I couldn't track it. Did the semantics change happen already? Or it is
under plan?

[khmel, 2016-10-18 01:26:04]

Truncated..
> > > This is real case that blocks working re-auth process. I don't touch
> StartLso
> > > code at all. StartUI is called from GetAuthCode. At this point UI is not
> shown
> > > and I see this error.
> > 
> > I'm wondering what's your request now based on the info Yury gave in the
> > previous comment. Split StartUI() into two (line 819-827 and 828-836),
revert
> > line 821, and let GetAuthCode[AndAccountType] only call the latter, maybe?
> 
> Or, maybe introducing two functions.
> 
> void StartSupportExtension() {  // or any better name?
>   if (arc is stopped()) {
>     // show error.
>     return;
>   }
>   SetState(State::FETCHING_CODE);
>   ShowUI(UIPage::TERMS_PROGRESS, ...);
> }
> 
> void PrepareAuthContext() {  // Any better name... ArcAuthContext looks not a
> context, actually???
>   SetState(State::FETCHING_CODE);  // Is this and starting extension in really
> the same state...?
>   context_->PrepareContext();
> }

ArcAuthContext is actually HTTP context used to support GAIA communication in
OptIn UI.
I added function PrepareContextForAuthCodeRequest and do all needed actions for
Auth code on demand there.
 
> 
> And call necessary ones for each.
> // Note: More TODOs are there on those functions still, but I dropped them
from
> this comment, because it looks out of this CL's focus, I think.
> 
>
https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
> chrome/browser/chromeos/arc/arc_auth_service.cc:911: if (state_ ==
State::ACTIVE
> && !IsAuthCodeRequest())
> On 2016/10/15 01:16:32, Yusuke Sato wrote:
> > On 2016/10/13 15:53:21, khmel wrote:
> > > On 2016/10/13 15:44:31, hidehiko wrote:
> > > > On 2016/10/13 15:30:19, khmel wrote:
> > > > > On 2016/10/13 15:22:00, hidehiko wrote:
> > > > > > My understanding is that; ArcAuthService shows an extension UI if;
> > > > > > - this is the first time the user starts ARC on the device, or
> > > > > > - For some reason (what's the case, BTW?), re-sign in is requested
> from
> > > ARC.
> > > > > > 
> > > > > > Is there any other cases?
> > > > > > If these are *only* two, the condition looks slightly weird to me
> > because;
> > > > > > - For the first case, ARC should still be in stopped state.
> > > > > > - For the second case, ARC should be running, but may be stopped due
> to,
> > > > e.g.,
> > > > > > crash.
> > > > > > 
> > > > > > Then, what's the case if ARC is running, but not in the re-sign in
> flow?
> > > > > 
> > > > > It can be requested when Arc is running on second or next boot and
> Android
> > > > needs
> > > > > re-auth or authenticate. In the last case, state_ is active.
> > > > 
> > > > I understand |state_| can be ACTIVE in re-auth case.
> > > > My question is, what is the case where |state_| is ACTIVE but *not* in
> > re-auth
> > > > case?
> > > 
> > > I don't see such case. I only see case with re-auth request. Note,
> > > StartArcIfSigned probably not the best function name in this case.
> > 
> > Then we can revert line 911 to the original one?
> 
> Reverting will cause a problem, because the new code actually would like not
to
> return if IsAuthCodeRequest() == true even if state_ == ACTIVE.
> Maybe remove entirely instead, and D?CHECK etc. with descriptive comments, if
> there is no case that state_ == ACTIVE && !IsAuthCodeRequest()?
I think there is the case when quickly re-enable Arc and this code triggers.
However we have separate issue for this.

> 
>
https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
> chrome/browser/chromeos/arc/arc_auth_service.cc:914: if (!IsAuthCodeRequest()
&&
> On 2016/10/15 01:16:32, Yusuke Sato wrote:
> > On 2016/10/13 15:53:21, khmel wrote:
> > > On 2016/10/13 15:44:32, hidehiko wrote:
> > > > On 2016/10/13 15:30:19, khmel wrote:
> > > > > On 2016/10/13 15:22:00, hidehiko wrote:
> > > > > > In this case, kArcSignedIn should be set to false already, shouldn't
> it?
> > > > > > IIUC, IsAuthCodeRequest() case is the re-sign in request from the
ARC.
> > It
> > > > > sounds
> > > > > > the real sign-in condition and persistent kArcSignedIn preference is
> > > > > > conflicting?
> > > > > 
> > > > > If AuthCodeRequest is set then kArcSignedIn may be on or off, there is
> no
> > > > > requirements here for it. GetAuthCode can be called at any time,
during
> > > first
> > > > > boot or on consequence boots. There is also one more flag added here
to
> > > > support
> > > > > the case OOBE integration.
> > > > 
> > > > Isn't kArcSignedIn represents the ARC container sign-in status?
> > > > If yes, and re-auth request means that the container is failed to
sign-in,
> > the
> > > > preference should be set to false to keep the state in sync, at the
first
> of
> > > > re-auth request.
> > > 
> > > Re-auth may occur at any moment, with or without SignedIn status. Please
> note
> > > that we might have flag skip auth. In this case SignedIn status would be
set
> > > without actual signing on Android side. You may consider SignedIn state as
> one
> > > successful run of Android
> > 
> > Ok, so we don't have to fix kArcSignedIn behavior at least in this CL,
> correct?
> > Assuming so, I guess hidehiko's overall concern on this CL is that changing
> > functions' behavior depending on the auth_*callback_ state (null or not) is
> > brittle and may incur a maintenance burden. I kind of agree with that.
> > 
> > Given that we can remove all other IsAuthCodeRequest() now (I hope), is
there
> a
> > not-too-complicated-and-M55-merge-able way to get rid of the remaining
> > IsAuthCodeRequest() call at line 914?
> > 
> > My (somewhat uninformed) guess is that the function is called only with
either
> > the following paths:
> > 
> >  GetAuthCode*()
> >    ...snip...
> >      OnContextReady()
> >        CheckAndroidManagement() with |background_mode| == false
> >          StartArcIfSignedIn()
> > 
> >  -or-
> > 
> >  GetAuthCode*()
> >    ...snip...
> >      OnContextReady()
> >        CheckAndroidManagement() with |background_mode| == false
> >          ArcAndroidManagementChecker()'s ctor
> >            ...snip...
> >              StartArcIfSignedIn()
> > 
> > correct? And |background_mode| indicates if it's re-auth or not. If so, can
we
> > propagate |background_mode| to StartArcIfSignedIn() so that the function can
> > know whether or not it's called during re-auth? WDYT? Does this address your
> > concern?
background_mode does not mean auth code request. If it indicates auth code
request but only as 'side effect'. It is related to android management check and
should not be used for auth code detection. You may also refer to
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_auth_ser...
where auth_code_callback dictates how to handle auth code.


> > 
> 
>
https://cs.chromium.org/chromium/src/chrome/common/pref_names.cc?q=kArcSigned...
> 
> explicitly says it is corresponding to the Android's sign in state. So, if the
> semantics of the value is changed, it should be updated. Though, I would
suggest
> not to change the semantics, because the value is persistent and old values
can
> be inherited, so the Chrome update would cause a trouble I think.
ArcSignedIn is very old flag. We may have discussion if this name good or not
(probably). Currently it indicates that user passed OptIn flow and we no need to
show OptIn flow on next start. That is the purpose of this flag. As I understand
your text above, you don't request to do anything in this CL.

> 
> If the semantics is kept as is, we need to set the value to false at the
> beginning of insists*()?
I don't think so, GetAuthCode and ArcSignedIn state have different
meaning/purpose. There is only one conclusion we may have. If ArcSignedIn state
is OFF that means we definitely need auth code. GetAuthCode on demand can be
used:
1. To handle account password revoke (this work has not been started yes IIRC)
2. Provide auth code on demand for OOBE integration flow.
3. For any other state on Android side that leads device appears in non-signed
state.

If you think that resetting of ArcSignedIn state in GetAuthCode still makes
sense can we please do it in separate CL? Introducing this would affect UX,
because resetting this flag will lead to re-shown GetStarted page in some cases
(for example user signs out during this), which may need PM review. I think this
out out scope of this CL that restores broken GetAuthCode flow.

> 
> BTW, I couldn't track it. Did the semantics change happen already? Or it is
> under plan?
I don't have any information about changing the semantic (as I understand it) in
past of plans to do in the future.

Thanks Hidehiko and Yusuke for your comments.
I updated the code trying to count your comments and make code more readable.

PTAL.

[hidehiko, 2016-10-18 09:38:28]

Note: to reply each inlined comment, could you use the web-UI on each diff page,
rather than using "reply" text box next time? It helps me to reply your comment
easily.

I replied inline below, in this turn.

On 2016/10/18 01:26:04, khmel wrote:
> Truncated..
> > > > This is real case that blocks working re-auth process. I don't touch
> > StartLso
> > > > code at all. StartUI is called from GetAuthCode. At this point UI is not
> > shown
> > > > and I see this error.
> > > 
> > > I'm wondering what's your request now based on the info Yury gave in the
> > > previous comment. Split StartUI() into two (line 819-827 and 828-836),
> revert
> > > line 821, and let GetAuthCode[AndAccountType] only call the latter, maybe?
> > 
> > Or, maybe introducing two functions.
> > 
> > void StartSupportExtension() {  // or any better name?
> >   if (arc is stopped()) {
> >     // show error.
> >     return;
> >   }
> >   SetState(State::FETCHING_CODE);
> >   ShowUI(UIPage::TERMS_PROGRESS, ...);
> > }
> > 
> > void PrepareAuthContext() {  // Any better name... ArcAuthContext looks not
a
> > context, actually???
> >   SetState(State::FETCHING_CODE);  // Is this and starting extension in
really
> > the same state...?
> >   context_->PrepareContext();
> > }
> 
> ArcAuthContext is actually HTTP context used to support GAIA communication in
> OptIn UI.
> I added function PrepareContextForAuthCodeRequest and do all needed actions
for
> Auth code on demand there.

I see. It is HTTPContext. Thank you for explanation.

>  
> > 
> > And call necessary ones for each.
> > // Note: More TODOs are there on those functions still, but I dropped them
> from
> > this comment, because it looks out of this CL's focus, I think.
> > 
> >
>
https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
> > chrome/browser/chromeos/arc/arc_auth_service.cc:911: if (state_ ==
> State::ACTIVE
> > && !IsAuthCodeRequest())
> > On 2016/10/15 01:16:32, Yusuke Sato wrote:
> > > On 2016/10/13 15:53:21, khmel wrote:
> > > > On 2016/10/13 15:44:31, hidehiko wrote:
> > > > > On 2016/10/13 15:30:19, khmel wrote:
> > > > > > On 2016/10/13 15:22:00, hidehiko wrote:
> > > > > > > My understanding is that; ArcAuthService shows an extension UI if;
> > > > > > > - this is the first time the user starts ARC on the device, or
> > > > > > > - For some reason (what's the case, BTW?), re-sign in is requested
> > from
> > > > ARC.
> > > > > > > 
> > > > > > > Is there any other cases?
> > > > > > > If these are *only* two, the condition looks slightly weird to me
> > > because;
> > > > > > > - For the first case, ARC should still be in stopped state.
> > > > > > > - For the second case, ARC should be running, but may be stopped
due
> > to,
> > > > > e.g.,
> > > > > > > crash.
> > > > > > > 
> > > > > > > Then, what's the case if ARC is running, but not in the re-sign in
> > flow?
> > > > > > 
> > > > > > It can be requested when Arc is running on second or next boot and
> > Android
> > > > > needs
> > > > > > re-auth or authenticate. In the last case, state_ is active.
> > > > > 
> > > > > I understand |state_| can be ACTIVE in re-auth case.
> > > > > My question is, what is the case where |state_| is ACTIVE but *not* in
> > > re-auth
> > > > > case?
> > > > 
> > > > I don't see such case. I only see case with re-auth request. Note,
> > > > StartArcIfSigned probably not the best function name in this case.
> > > 
> > > Then we can revert line 911 to the original one?
> > 
> > Reverting will cause a problem, because the new code actually would like not
> to
> > return if IsAuthCodeRequest() == true even if state_ == ACTIVE.
> > Maybe remove entirely instead, and D?CHECK etc. with descriptive comments,
if
> > there is no case that state_ == ACTIVE && !IsAuthCodeRequest()?
> I think there is the case when quickly re-enable Arc and this code triggers.
> However we have separate issue for this.
> 

Could you describe more details how re-enable ARC triggers this code?
Also, I think it is valuable to comment in the code.

> > 
> >
>
https://codereview.chromium.org/2412133004/diff/1/chrome/browser/chromeos/arc...
> > chrome/browser/chromeos/arc/arc_auth_service.cc:914: if
(!IsAuthCodeRequest()
> &&
> > On 2016/10/15 01:16:32, Yusuke Sato wrote:
> > > On 2016/10/13 15:53:21, khmel wrote:
> > > > On 2016/10/13 15:44:32, hidehiko wrote:
> > > > > On 2016/10/13 15:30:19, khmel wrote:
> > > > > > On 2016/10/13 15:22:00, hidehiko wrote:
> > > > > > > In this case, kArcSignedIn should be set to false already,
shouldn't
> > it?
> > > > > > > IIUC, IsAuthCodeRequest() case is the re-sign in request from the
> ARC.
> > > It
> > > > > > sounds
> > > > > > > the real sign-in condition and persistent kArcSignedIn preference
is
> > > > > > > conflicting?
> > > > > > 
> > > > > > If AuthCodeRequest is set then kArcSignedIn may be on or off, there
is
> > no
> > > > > > requirements here for it. GetAuthCode can be called at any time,
> during
> > > > first
> > > > > > boot or on consequence boots. There is also one more flag added here
> to
> > > > > support
> > > > > > the case OOBE integration.
> > > > > 
> > > > > Isn't kArcSignedIn represents the ARC container sign-in status?
> > > > > If yes, and re-auth request means that the container is failed to
> sign-in,
> > > the
> > > > > preference should be set to false to keep the state in sync, at the
> first
> > of
> > > > > re-auth request.
> > > > 
> > > > Re-auth may occur at any moment, with or without SignedIn status. Please
> > note
> > > > that we might have flag skip auth. In this case SignedIn status would be
> set
> > > > without actual signing on Android side. You may consider SignedIn state
as
> > one
> > > > successful run of Android
> > > 
> > > Ok, so we don't have to fix kArcSignedIn behavior at least in this CL,
> > correct?
> > > Assuming so, I guess hidehiko's overall concern on this CL is that
changing
> > > functions' behavior depending on the auth_*callback_ state (null or not)
is
> > > brittle and may incur a maintenance burden. I kind of agree with that.
> > > 
> > > Given that we can remove all other IsAuthCodeRequest() now (I hope), is
> there
> > a
> > > not-too-complicated-and-M55-merge-able way to get rid of the remaining
> > > IsAuthCodeRequest() call at line 914?
> > > 
> > > My (somewhat uninformed) guess is that the function is called only with
> either
> > > the following paths:
> > > 
> > >  GetAuthCode*()
> > >    ...snip...
> > >      OnContextReady()
> > >        CheckAndroidManagement() with |background_mode| == false
> > >          StartArcIfSignedIn()
> > > 
> > >  -or-
> > > 
> > >  GetAuthCode*()
> > >    ...snip...
> > >      OnContextReady()
> > >        CheckAndroidManagement() with |background_mode| == false
> > >          ArcAndroidManagementChecker()'s ctor
> > >            ...snip...
> > >              StartArcIfSignedIn()
> > > 
> > > correct? And |background_mode| indicates if it's re-auth or not. If so,
can
> we
> > > propagate |background_mode| to StartArcIfSignedIn() so that the function
can
> > > know whether or not it's called during re-auth? WDYT? Does this address
your
> > > concern?
> background_mode does not mean auth code request. If it indicates auth code
> request but only as 'side effect'. It is related to android management check
and
> should not be used for auth code detection. You may also refer to
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_auth_ser...
> where auth_code_callback dictates how to handle auth code.
> 
> 
> > > 
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/common/pref_names.cc?q=kArcSigned...
> > 
> > explicitly says it is corresponding to the Android's sign in state. So, if
the
> > semantics of the value is changed, it should be updated. Though, I would
> suggest
> > not to change the semantics, because the value is persistent and old values
> can
> > be inherited, so the Chrome update would cause a trouble I think.
> ArcSignedIn is very old flag. We may have discussion if this name good or not
> (probably). Currently it indicates that user passed OptIn flow and we no need
to
> show OptIn flow on next start. That is the purpose of this flag. As I
understand
> your text above, you don't request to do anything in this CL.
> 

Thank you for explanation and now I understand the goal of the preference.
Could you update the comment for kArcSignedIn to adapt the meaning clearly?
The current comment looks ambiguous and confusing to me. Actually I couldn't
figure out what it is for.

I'm ok to do it in another CL.

> > 
> > If the semantics is kept as is, we need to set the value to false at the
> > beginning of insists*()?
> I don't think so, GetAuthCode and ArcSignedIn state have different
> meaning/purpose. There is only one conclusion we may have. If ArcSignedIn
state
> is OFF that means we definitely need auth code. GetAuthCode on demand can be
> used:
> 1. To handle account password revoke (this work has not been started yes IIRC)
> 2. Provide auth code on demand for OOBE integration flow.
> 3. For any other state on Android side that leads device appears in non-signed
> state.
> 
> If you think that resetting of ArcSignedIn state in GetAuthCode still makes
> sense can we please do it in separate CL? Introducing this would affect UX,
> because resetting this flag will lead to re-shown GetStarted page in some
cases
> (for example user signs out during this), which may need PM review. I think
this
> out out scope of this CL that restores broken GetAuthCode flow.

Could you add detailed comment in the code?

[hidehiko, 2016-10-18 09:38:42]

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

https://codereview.chromium.org/2412133004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:761: StartUI();
This should be PrepareContextForAuthCodeRequest(), too?
If you do not want to do this in this CL, please add TODO.

[hidehiko, 2016-10-20 05:07:14]

Yury, considering this is marked as M55 bug, how about landing this quickly (I'd
appreciate to have more comments I replied in the previous comment, though),
before our refactoring work, so that you can cherry this pick to M55 branch
easily?

Defer the review to Yusuke for iteration speed.

[khmel, 2016-10-20 15:19:23]

Patchset #4 (id:60001) has been deleted

[khmel, 2016-10-20 15:31:41]

Thank you Hidehiko for your comments.


>> Could you describe more details how re-enable ARC triggers >> this code?
>> Also, I think it is valuable to comment in the code.
I have no clear idea how this happened during internal logic flow. This is my
practical observation. When I re-enable Arc I get this error immediately.
This code is used twice:
Second time it used here:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_auth_ser...
Last case looks like with less probability because it assumes that Arc container
is booting for some time.
And this issue is addressed here: b/30938418 (You re-assigned this recently to
yourself)

PTAL

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

https://codereview.chromium.org/2412133004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:761: StartUI();
On 2016/10/18 09:38:42, hidehiko wrote:
> This should be PrepareContextForAuthCodeRequest(), too?
> If you do not want to do this in this CL, please add TODO.

Auth state in this case FETCHING_CODE, PrepareContextForAuthCodeRequest is
called during ACTIVE state. Potentially it is possible to join but this requires
extra round of testing and refactoring. Added TODO

[Yusuke Sato, 2016-10-20 19:44:49]

lgtm

[khmel, 2016-10-20 22:48:08]

Thank you for review.

[khmel, 2016-10-20 22:48:12]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-10-20 22:48:12]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
yusukes@chromium.org
Link to the patchset: https://codereview.chromium.org/2412133004/#ps100001
(title: "remove DCHECK_EQ(state_, State::ACTIVE) (State can be
ACTIVE/FETCHING_CODE)")

[commit-bot: I haz the power, 2016-10-20 22:49:08]

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-10-21 01:30:59]

Description was changed from

==========
arc: Restore broken auth code request on demand.

This feature is required when Android needs re-authorization
or in new OOBE Arc Terms integration.

BUG=b/32124104
TEST=Manually on device during the testing OOBE Arc Terms
==========

to

==========
arc: Restore broken auth code request on demand.

This feature is required when Android needs re-authorization
or in new OOBE Arc Terms integration.

BUG=b/32124104
TEST=Manually on device during the testing OOBE Arc Terms
==========

[commit-bot: I haz the power, 2016-10-21 01:31:00]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-10-21 13:25:47]

Description was changed from

==========
arc: Restore broken auth code request on demand.

This feature is required when Android needs re-authorization
or in new OOBE Arc Terms integration.

BUG=b/32124104
TEST=Manually on device during the testing OOBE Arc Terms
==========

to

==========
arc: Restore broken auth code request on demand.

This feature is required when Android needs re-authorization
or in new OOBE Arc Terms integration.

BUG=b/32124104
TEST=Manually on device during the testing OOBE Arc Terms

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

[commit-bot: I haz the power, 2016-10-21 13:25:48]

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