arc: Handle closing OptIn window for managed Arc.

arc: Handle closing OptIn window for managed Arc.

This allows restarting OptIn flow for managed Arc in case user
re-activates it after stopping.

BUG=b/29121091
TEST=Manually on device for managed Arc (@google.com) and
     non-managed Arc (@gmail.com). In both cases OptIn can be
     restarted after canceling it.

Committed: https://crrev.com/34fd9497a76e94d4c843a781872e1199b6f68275
Cr-Commit-Position: refs/heads/master@{#403371}

[khmel, 2016-06-29 23:53:54]

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

[khmel, 2016-06-29 23:53:55]

Hi Luis,

This is kind of variation of your previous CL.

PTAL

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

https://codereview.chromium.org/2111733002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:589: ShutdownBridge();
This is handled in StopArc and by adding check on the line 597

https://codereview.chromium.org/2111733002/diff/1/chrome/browser/ui/ash/launc...
File
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc
(left):

https://codereview.chromium.org/2111733002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_playstore_shortcut_launcher_item_controller.cc:32:
if (!auth_service->IsArcEnabled())
Arc can be enabled by stopped (in case of managed user). Now EnableArc can care
about this case.

[Luis Héctor Chávez, 2016-06-30 17:04:13]

oh, i like this better :) Thanks for the follow-up and cleanup

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

https://codereview.chromium.org/2111733002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:592: if (state_ !=
State::FETCHING_CODE && ui_page_ != UIPage::ERROR &&
Given that silent opt-in is now off the table, can we still have the behavior of
closing the window means shutting down the container? If so, this condition
should be

if (state_ == State::NOT_INITIALIZED || state_ == State::STOPPED)

[khmel, 2016-06-30 17:41:50]

khmel@chromium.org changed reviewers:
+ oshima@chromium.org

[khmel, 2016-06-30 17:41:50]

Thanks Luis for feedback,

Adding oshima@ to review trivial controller change once Stefan is OOO.

PTAL

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

https://codereview.chromium.org/2111733002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:592: if (state_ !=
State::FETCHING_CODE && ui_page_ != UIPage::ERROR &&
On 2016/06/30 17:04:13, Luis Héctor Chávez wrote:
> Given that silent opt-in is now off the table, can we still have the behavior
of
> closing the window means shutting down the container? If so, this condition
> should be
> 
> if (state_ == State::NOT_INITIALIZED || state_ == State::STOPPED)

I added State::NOT_INITIALIZED handling and comment about State::ACTIVE. In case
State::ACTIVE progress is auxiliary UI and closing it should not stop Arc.

About State::STOPPED: This state is handled correctly at StopArc. Also we cannot
exit early with it because this call (CancelAuthcCode) not only stops the
container but also set arc.enabled=off in preferences (if not managed)

[Luis Héctor Chávez, 2016-06-30 20:46:46]

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

https://codereview.chromium.org/2111733002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_auth_service.cc:592: if (state_ !=
State::FETCHING_CODE && ui_page_ != UIPage::ERROR &&
On 2016/06/30 17:41:50, khmel wrote:
> On 2016/06/30 17:04:13, Luis Héctor Chávez wrote:
> > Given that silent opt-in is now off the table, can we still have the
behavior
> of
> > closing the window means shutting down the container? If so, this condition
> > should be
> > 
> > if (state_ == State::NOT_INITIALIZED || state_ == State::STOPPED)
> 
> I added State::NOT_INITIALIZED handling and comment about State::ACTIVE. In
case
> State::ACTIVE progress is auxiliary UI and closing it should not stop Arc.
> 
> About State::STOPPED: This state is handled correctly at StopArc. Also we
cannot
> exit early with it because this call (CancelAuthcCode) not only stops the
> container but also set arc.enabled=off in preferences (if not managed)    

Discussed this with mitsuji@. He was also of the idea that we shouldn't require
another opt-in flow if the window is closed after the opt-in was done. That
leaves us with two non-conflicting options:

a) Add a global timeout. If the instance has not finished booted after a certain
period of time, we consider that an unrecoverble failure (like the provisioning
failure), and allow for both retry and potentially sending feedback. The problem
that mitsuji@ mentioned is that since you've already closed the window, we would
need some text explaining what happened.
b) Have a way to restore the window to whatever state it was before closing.
Currently if you click on the Play Store icon, nothing happens. If we hit the
timeout, that'l potentially be that way for a couple of minutes. This might also
need some text changes.

[khmel, 2016-06-30 22:16:45]

On 2016/06/30 20:46:46, Luis Héctor Chávez wrote:
>
https://codereview.chromium.org/2111733002/diff/1/chrome/browser/chromeos/arc...
> File chrome/browser/chromeos/arc/arc_auth_service.cc (right):
> 
>
https://codereview.chromium.org/2111733002/diff/1/chrome/browser/chromeos/arc...
> chrome/browser/chromeos/arc/arc_auth_service.cc:592: if (state_ !=
> State::FETCHING_CODE && ui_page_ != UIPage::ERROR &&
> On 2016/06/30 17:41:50, khmel wrote:
> > On 2016/06/30 17:04:13, Luis Héctor Chávez wrote:
> > > Given that silent opt-in is now off the table, can we still have the
> behavior
> > of
> > > closing the window means shutting down the container? If so, this
condition
> > > should be
> > > 
> > > if (state_ == State::NOT_INITIALIZED || state_ == State::STOPPED)
> > 
> > I added State::NOT_INITIALIZED handling and comment about State::ACTIVE. In
> case
> > State::ACTIVE progress is auxiliary UI and closing it should not stop Arc.
> > 
> > About State::STOPPED: This state is handled correctly at StopArc. Also we
> cannot
> > exit early with it because this call (CancelAuthcCode) not only stops the
> > container but also set arc.enabled=off in preferences (if not managed)    
> 
> Discussed this with mitsuji@. He was also of the idea that we shouldn't
require
> another opt-in flow if the window is closed after the opt-in was done. That
> leaves us with two non-conflicting options:
> 
> a) Add a global timeout. If the instance has not finished booted after a
certain
> period of time, we consider that an unrecoverble failure (like the
provisioning
> failure), and allow for both retry and potentially sending feedback. The
problem
> that mitsuji@ mentioned is that since you've already closed the window, we
would
> need some text explaining what happened.
> b) Have a way to restore the window to whatever state it was before closing.
> Currently if you click on the Play Store icon, nothing happens. If we hit the
> timeout, that'l potentially be that way for a couple of minutes. This might
also
> need some text changes.

We should show spinning animation on playstore. do you want to do in context of
this CL?

[oshima, 2016-06-30 22:36:28]

controller lgtm

[Luis Héctor Chávez, 2016-06-30 22:38:12]

On 2016/06/30 22:16:45, khmel wrote:
> On 2016/06/30 20:46:46, Luis Héctor Chávez wrote:
> >
>
https://codereview.chromium.org/2111733002/diff/1/chrome/browser/chromeos/arc...
> > File chrome/browser/chromeos/arc/arc_auth_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/2111733002/diff/1/chrome/browser/chromeos/arc...
> > chrome/browser/chromeos/arc/arc_auth_service.cc:592: if (state_ !=
> > State::FETCHING_CODE && ui_page_ != UIPage::ERROR &&
> > On 2016/06/30 17:41:50, khmel wrote:
> > > On 2016/06/30 17:04:13, Luis Héctor Chávez wrote:
> > > > Given that silent opt-in is now off the table, can we still have the
> > behavior
> > > of
> > > > closing the window means shutting down the container? If so, this
> condition
> > > > should be
> > > > 
> > > > if (state_ == State::NOT_INITIALIZED || state_ == State::STOPPED)
> > > 
> > > I added State::NOT_INITIALIZED handling and comment about State::ACTIVE.
In
> > case
> > > State::ACTIVE progress is auxiliary UI and closing it should not stop Arc.
> > > 
> > > About State::STOPPED: This state is handled correctly at StopArc. Also we
> > cannot
> > > exit early with it because this call (CancelAuthcCode) not only stops the
> > > container but also set arc.enabled=off in preferences (if not managed)    
> > 
> > Discussed this with mitsuji@. He was also of the idea that we shouldn't
> require
> > another opt-in flow if the window is closed after the opt-in was done. That
> > leaves us with two non-conflicting options:
> > 
> > a) Add a global timeout. If the instance has not finished booted after a
> certain
> > period of time, we consider that an unrecoverble failure (like the
> provisioning
> > failure), and allow for both retry and potentially sending feedback. The
> problem
> > that mitsuji@ mentioned is that since you've already closed the window, we
> would
> > need some text explaining what happened.
> > b) Have a way to restore the window to whatever state it was before closing.
> > Currently if you click on the Play Store icon, nothing happens. If we hit
the
> > timeout, that'l potentially be that way for a couple of minutes. This might
> also
> > need some text changes.
> 
> We should show spinning animation on playstore. do you want to do in context
of
> this CL?

it can be done separately, since this is good to have by itself since it does
make the code cleaner and easier to reason about. lgtm then

[khmel, 2016-06-30 22:49:57]

Thanks for review!

Following is created to implement more features:
b/29915185
b/29915769

[khmel, 2016-06-30 22:50:02]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2016-06-30 22:50:49]

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-06-30 23:06:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-30 23:06:41]

Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[khmel, 2016-06-30 23:38:52]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-06-30 23:38:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2111733002/#ps40001
(title: "fix comilation error")

[commit-bot: I haz the power, 2016-06-30 23:39:49]

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-07-01 00:30:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-01 00:32:00]

Description was changed from

==========
arc: Handle closing OptIn window for managed Arc.

This allows restarting OptIn flow for managed Arc in case user
re-activates it after stopping.

BUG=b/29121091
TEST=Manually on device for managed Arc (@google.com) and
     non-managed Arc (@gmail.com). In both cases OptIn can be
     restarted after canceling it.
==========

to

==========
arc: Handle closing OptIn window for managed Arc.

This allows restarting OptIn flow for managed Arc in case user
re-activates it after stopping.

BUG=b/29121091
TEST=Manually on device for managed Arc (@google.com) and
     non-managed Arc (@gmail.com). In both cases OptIn can be
     restarted after canceling it.

Committed: https://crrev.com/34fd9497a76e94d4c843a781872e1199b6f68275
Cr-Commit-Position: refs/heads/master@{#403371}
==========

[commit-bot: I haz the power, 2016-07-01 00:32:01]

Patchset 3 (id:??) landed as
https://crrev.com/34fd9497a76e94d4c843a781872e1199b6f68275
Cr-Commit-Position: refs/heads/master@{#403371}

[mtomasz, 2016-07-20 03:46:25]

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

[mtomasz, 2016-07-20 03:46:26]

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

https://codereview.chromium.org/2111733002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:648:
profile_->GetPrefs()->SetBoolean(prefs::kArcEnabled, false);
Why isn't there a "if (IsArcManaged())" guard like in #637? Is it intentional?

[khmel, 2016-07-20 16:50:41]

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

https://codereview.chromium.org/2111733002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_auth_service.cc:648:
profile_->GetPrefs()->SetBoolean(prefs::kArcEnabled, false);
On 2016/07/20 03:46:25, mtomasz wrote:
> Why isn't there a "if (IsArcManaged())" guard like in #637? Is it intentional?

This has no effect for managed Arc. Probably for consistency we may add it.