Move ArcSessionRunner from ArcBridgeService to ArcSessionManager.

Move ArcSessionRunner from ArcBridgeService to ArcSessionManager.

With this CL, as named, ArcSessionManager starts to manage
ArcSession.
ArcBridgeService is now simple Arc*Instance mojo object holder.

TBR=bartfab@chromium.org
BUG=657687
BUG=b/31079732
TEST=Ran bots.

Committed: https://crrev.com/5cc0f2f484ee3475b1f590f9809cea30817efb54
Cr-Commit-Position: refs/heads/master@{#440737}

[hidehiko, 2016-12-22 15:33:09]

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

[commit-bot: I haz the power, 2016-12-22 15:33:25]

Dry run: 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-12-22 16:11:36]

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

[commit-bot: I haz the power, 2016-12-22 16:11:37]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-12-22 16:33:44]

hidehiko@chromium.org changed reviewers:
+ bartfab@chromium.org, lhchavez@chromium.org, msw@chromium.org,
xiyuan@chromium.org

[hidehiko, 2016-12-22 16:33:45]

Last ArcBridgeService related clean up in this year. Next one will come in new
year.

PTAL.

lhchavez@: overall code change, specifically c/b/c/arc, components/arc.
bartfab@: c/b/policy
xiyuan@: c/b/ui/app_list
msw@: c/b/ui/views

Thank you for review in advance.
- hidehiko

[msw, 2016-12-22 16:47:20]

c/b/ui/views lgtm

[xiyuan, 2016-12-22 17:42:07]

c/b/ui/app_list lgtm

[Luis Héctor Chávez, 2016-12-22 18:29:56]

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

https://codereview.chromium.org/2596273002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:283: // Due asynchronous
nature of stopping ARC instance, OnProvisioningFinished
nit: the ARC instance

https://codereview.chromium.org/2596273002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:285: // is not guaranty set
here. prefs::kArcDataRemoveRequested is also can be
nit: s/guaranty/guaranteed to be/;s/is also can be/also can be/

https://codereview.chromium.org/2596273002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:652: // STOPPED immediately
here.
just curious: is the plan to delete IsSessionStopped() and have
IsSessionRunning() be the only source of truth?

If that's the case, maybe it's better to change all the checks in
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc to only use
IsSessionRunning() to avoid future changes there. (only if the weird nature of
both flags not being exactly opposite of each other doesn't interact badly with
the test, but given that they all used ready() I *think* it should be fine).

https://codereview.chromium.org/2596273002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:794: // If the user attempts
to re-enable ARC while ARC instance is still
nit: the ARC instance. same below.

https://codereview.chromium.org/2596273002/diff/1/components/arc/arc_bridge_s...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/2596273002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.h:51: // The Chrome-side service that handles
ARC instances and ARC bridge creation.
Not anymore :)

https://codereview.chromium.org/2596273002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.h:57: virtual ~ArcBridgeService();
This class does not have any subclasses anymore. Can we get rid of this virtual?

[hidehiko, 2016-12-22 18:47:37]

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

[commit-bot: I haz the power, 2016-12-22 18:47:52]

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

[hidehiko, 2016-12-22 19:04:12]

Thank you for quick review. PTAL.

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

https://codereview.chromium.org/2596273002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:283: // Due asynchronous
nature of stopping ARC instance, OnProvisioningFinished
On 2016/12/22 18:29:56, Luis Héctor Chávez wrote:
> nit: the ARC instance

Done.

https://codereview.chromium.org/2596273002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:285: // is not guaranty set
here. prefs::kArcDataRemoveRequested is also can be
On 2016/12/22 18:29:56, Luis Héctor Chávez wrote:
> nit: s/guaranty/guaranteed to be/;s/is also can be/also can be/

Done.

https://codereview.chromium.org/2596273002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:652: // STOPPED immediately
here.
On 2016/12/22 18:29:56, Luis Héctor Chávez wrote:
> just curious: is the plan to delete IsSessionStopped() and have
> IsSessionRunning() be the only source of truth?
> 
> If that's the case, maybe it's better to change all the checks in
> chrome/browser/chromeos/arc/arc_session_manager_unittest.cc to only use
> IsSessionRunning() to avoid future changes there. (only if the weird nature of
> both flags not being exactly opposite of each other doesn't interact badly
with
> the test, but given that they all used ready() I *think* it should be fine).

TBD. Currently, IsSessionStopped()/Running() is just proxy from
ArcSessionRunner. IMHO, whether Actually ARC is running or not looks too details
for this layer, and maybe it's better to avoid exposing them for encapsulation.
However, for now, it looks not yet clear enough to me whether it is technically
doable. So, let's address it separately.

As for testing code, indeed the original code looked to assume "STOPPED ==
!RUNNING", but it is not true, as you know. So, I'd like to keep the current
code to follow the current semantics, for now.

WDYT?

https://codereview.chromium.org/2596273002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:794: // If the user attempts
to re-enable ARC while ARC instance is still
On 2016/12/22 18:29:56, Luis Héctor Chávez wrote:
> nit: the ARC instance. same below.

Done.

https://codereview.chromium.org/2596273002/diff/1/components/arc/arc_bridge_s...
File components/arc/arc_bridge_service.h (right):

https://codereview.chromium.org/2596273002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.h:51: // The Chrome-side service that handles
ARC instances and ARC bridge creation.
On 2016/12/22 18:29:56, Luis Héctor Chávez wrote:
> Not anymore :)

Oops. Updated.

https://codereview.chromium.org/2596273002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.h:57: virtual ~ArcBridgeService();
On 2016/12/22 18:29:56, Luis Héctor Chávez wrote:
> This class does not have any subclasses anymore. Can we get rid of this
virtual?

Good catch! Fixed.

[commit-bot: I haz the power, 2016-12-22 19:19:20]

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

[commit-bot: I haz the power, 2016-12-22 19:19:21]

Dry run: This issue passed the CQ dry run.

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

lgtm with one more nit.

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

https://codereview.chromium.org/2596273002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:650: // TODO(hidehiko):
Actual the ARC instance stop will be delayed, and until
nit: The ARC instance's stopping is asynchronous, so it might still be running
when we return from this function. Do not set the STOPPED state immediately
here.

[hidehiko, 2016-12-27 06:33:49]

Description was changed from

==========
Move ArcSessionRunner from ArcBridgeService to ArcSessionManager.

With this CL, as named, ArcSessionManager starts to manage
ArcSession.
ArcBridgeService is now simple Arc*Instance mojo object holder.

BUG=657687
BUG=b/31079732
TEST=Ran bots.
==========

to

==========
Move ArcSessionRunner from ArcBridgeService to ArcSessionManager.

With this CL, as named, ArcSessionManager starts to manage
ArcSession.
ArcBridgeService is now simple Arc*Instance mojo object holder.

TBR=bartfab@chromium.org
BUG=657687
BUG=b/31079732
TEST=Ran bots.
==========

[hidehiko, 2016-12-27 06:35:17]

Thank you for review. Submitting with TBR=bartfab for policy change, which is
boilerplate change along with the main refactoring in c/b/c/arc and
components/arc, reviewed by Luis.

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

https://codereview.chromium.org/2596273002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:650: // TODO(hidehiko):
Actual the ARC instance stop will be delayed, and until
On 2016/12/22 20:10:06, Luis Héctor Chávez wrote:
> nit: The ARC instance's stopping is asynchronous, so it might still be running
> when we return from this function. Do not set the STOPPED state immediately
> here.

Done.

[hidehiko, 2016-12-27 06:35:24]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-12-27 06:35:25]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
msw@chromium.org, lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2596273002/#ps40001
(title: "Rebase and update comments.")

[commit-bot: I haz the power, 2016-12-27 06:35:39]

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-12-27 07:14:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482820524846470,
"parent_rev": "0da675847cfc11ac9a126bc56510b1c6ee0c47f1", "commit_rev":
"8bc6bd3217fe5968e05012258dc05aff9df9db9d"}

[commit-bot: I haz the power, 2016-12-27 07:14:53]

Description was changed from

==========
Move ArcSessionRunner from ArcBridgeService to ArcSessionManager.

With this CL, as named, ArcSessionManager starts to manage
ArcSession.
ArcBridgeService is now simple Arc*Instance mojo object holder.

TBR=bartfab@chromium.org
BUG=657687
BUG=b/31079732
TEST=Ran bots.
==========

to

==========
Move ArcSessionRunner from ArcBridgeService to ArcSessionManager.

With this CL, as named, ArcSessionManager starts to manage
ArcSession.
ArcBridgeService is now simple Arc*Instance mojo object holder.

TBR=bartfab@chromium.org
BUG=657687
BUG=b/31079732
TEST=Ran bots.

Review-Url: https://codereview.chromium.org/2596273002
==========

[commit-bot: I haz the power, 2016-12-27 07:14:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2017-01-02 15:45:41]

Description was changed from

==========
Move ArcSessionRunner from ArcBridgeService to ArcSessionManager.

With this CL, as named, ArcSessionManager starts to manage
ArcSession.
ArcBridgeService is now simple Arc*Instance mojo object holder.

TBR=bartfab@chromium.org
BUG=657687
BUG=b/31079732
TEST=Ran bots.

Review-Url: https://codereview.chromium.org/2596273002
==========

to

==========
Move ArcSessionRunner from ArcBridgeService to ArcSessionManager.

With this CL, as named, ArcSessionManager starts to manage
ArcSession.
ArcBridgeService is now simple Arc*Instance mojo object holder.

TBR=bartfab@chromium.org
BUG=657687
BUG=b/31079732
TEST=Ran bots.

Committed: https://crrev.com/5cc0f2f484ee3475b1f590f9809cea30817efb54
Cr-Commit-Position: refs/heads/master@{#440737}
==========

[commit-bot: I haz the power, 2017-01-02 15:45:42]

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