arc: Closing LSO page shoud disable ARC.

arc: Closing LSO page shoud disable ARC.

This fix regression compared M55 build.

TEST=Manually, close LSO page and observe that Play Store checkbox
     in settings turns off.
BUG=b/34199954

Review-Url: https://codereview.chromium.org/2625833002
Cr-Commit-Position: refs/heads/master@{#442819}
Committed: https://chromium.googlesource.com/chromium/src/+/293f4eecd49dd62088f2ba3e12c3fc17eb5e43fd

[khmel, 2017-01-10 22:10:43]

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

[khmel, 2017-01-10 22:10:44]

Hi Luis,

PTAL

[Luis Héctor Chávez, 2017-01-11 00:23:52]

lgtm but please reword the comment

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

https://codereview.chromium.org/2625833002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:728: // closing UI means no
auth code and sign-in failure. Stop ARC in that case
nit: I'd probably reword it as

"If ARC failed to boot normally, stop ARC. Similarly, if the current page is
LSO, closing the window should stop ARC since the user actively chose to not
sign in. In any other case, ARC is booting normally and the instance should not
be stopped."

[khmel, 2017-01-11 05:36:43]

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

https://codereview.chromium.org/2625833002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:728: // closing UI means no
auth code and sign-in failure. Stop ARC in that case
On 2017/01/11 00:23:52, Luis Héctor Chávez wrote:
> nit: I'd probably reword it as
> 
> "If ARC failed to boot normally, stop ARC. Similarly, if the current page is
> LSO, closing the window should stop ARC since the user actively chose to not
> sign in. In any other case, ARC is booting normally and the instance should
not
> be stopped."

That is nice, thanks!

[khmel, 2017-01-11 05:37:01]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-01-11 05:37:01]

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/2625833002/#ps20001
(title: "comment updated")

[commit-bot: I haz the power, 2017-01-11 05:37:22]

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, 2017-01-11 06:09:31]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484113021456100,
"parent_rev": "d6a80917e2e0bbdda7c537ec36f7acd3183d14e8", "commit_rev":
"293f4eecd49dd62088f2ba3e12c3fc17eb5e43fd"}

[commit-bot: I haz the power, 2017-01-11 06:10:01]

Description was changed from

==========
arc: Closing LSO page shoud disable ARC.

This fix regression compared M55 build.

TEST=Manually, close LSO page and observe that Play Store checkbox
     in settings turns off.
BUG=b/34199954
==========

to

==========
arc: Closing LSO page shoud disable ARC.

This fix regression compared M55 build.

TEST=Manually, close LSO page and observe that Play Store checkbox
     in settings turns off.
BUG=b/34199954

Review-Url: https://codereview.chromium.org/2625833002
Cr-Commit-Position: refs/heads/master@{#442819}
Committed:
https://chromium.googlesource.com/chromium/src/+/293f4eecd49dd62088f2ba3e12c3...
==========

[commit-bot: I haz the power, 2017-01-11 06:10:01]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/293f4eecd49dd62088f2ba3e12c3...

[hidehiko, 2017-01-11 06:44:04]

On 2017/01/11 06:10:01, commit-bot: I haz the power wrote:
> Committed patchset #2 (id:20001) as
>
https://chromium.googlesource.com/chromium/src/+/293f4eecd49dd62088f2ba3e12c3...

Clarification: With this change, the user data will be wiped without user
confirmation,
even if it is reauth case. Is it acceptable and intentional?

[khmel, 2017-01-11 15:39:19]

On 2017/01/11 06:44:04, hidehiko wrote:
> On 2017/01/11 06:10:01, commit-bot: I haz the power wrote:
> > Committed patchset #2 (id:20001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/293f4eecd49dd62088f2ba3e12c3...
> 
> Clarification: With this change, the user data will be wiped without user
> confirmation,
> even if it is reauth case. Is it acceptable and intentional?

The same happens if reauth causes an error. If this is a problem, it must be
solved together.