Refactor ArcBridgeServiceImpl part 2.

Refactor ArcBridgeServiceImpl part 2.

This CL is the preparation to split ArcSession running parts
from ArcBridgeService.
- Rename ArcBridgeService::State names to be consistent with
  terms used in other ARC modules.
- Remove unused CONNECTED from State.
- Migrate PrerequisitesChanged() and StopInstance() into
  RequestStart() and RequestStop().
- Rename some fields of ArcBridgeServiceImpl for consistency.
- Cleaned up include directives.

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

Committed: https://crrev.com/70586e6fd361f66febe643223e61c08caf776c86
Cr-Commit-Position: refs/heads/master@{#439875}

[hidehiko, 2016-12-16 11:41:09]

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

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

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-16 12:08:42]

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

[commit-bot: I haz the power, 2016-12-16 12:08:42]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-12-16 12:15:31]

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

[hidehiko, 2016-12-16 12:15:31]

For better code review-ability, I split this CL from crrev.com/2574013003. Hope
it is also better for you. PTAL.

[Luis Héctor Chávez, 2016-12-16 23:20:12]

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

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.h:150: // ArcBridgeService's state machien can
be stopped at any phase.
nit: s/machien/machine/

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.h:161: // Request to start ARC instance is
received. Startin an ARC instance.
nit: s/Startin/Starting/

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
File components/arc/arc_bridge_service_impl.cc (right):

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.cc:41: // Here the |running_| becomes
false to true. So, |restart_timer_| must be
nit: "Here |running_| transitions from false to true."

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.cc:47: // This case, a client calls
RequestStop() in advance, then soon
nit: "In this case, RequestStop() was called, and before |arc_session_| had
finished stopping, RequestStart() was called. Do nothing in that case, since
when |arc_session_| does actually stop, OnSessionStopped() will be called, where
it should automatically restart."

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.cc:77: if (state() != State::STOPPING) {
nit:

if (state() == State::STOPPING) {
  // STOPPING is found ...
  return;
}

that way "do nothing" is immediately followed by an explicit return that does
nothing.

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.cc:155: // If CONNECTING, ARC instance
has not been booted properly, so do not
STARTING?

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

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.h:69: bool running_ = false;
nit: I'd call this |run_requested_|, because it is possible for this to be true
when it is not running, and false when it is.

[hidehiko, 2016-12-19 08:05:05]

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

[hidehiko, 2016-12-19 08:05:12]

Thank you for review. PTAL.

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

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.h:150: // ArcBridgeService's state machien can
be stopped at any phase.
On 2016/12/16 23:20:12, Luis Héctor Chávez wrote:
> nit: s/machien/machine/

Done.

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service.h:161: // Request to start ARC instance is
received. Startin an ARC instance.
On 2016/12/16 23:20:12, Luis Héctor Chávez wrote:
> nit: s/Startin/Starting/

Done.

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
File components/arc/arc_bridge_service_impl.cc (right):

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.cc:41: // Here the |running_| becomes
false to true. So, |restart_timer_| must be
On 2016/12/16 23:20:12, Luis Héctor Chávez wrote:
> nit: "Here |running_| transitions from false to true."

Done.

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.cc:47: // This case, a client calls
RequestStop() in advance, then soon
On 2016/12/16 23:20:12, Luis Héctor Chávez wrote:
> nit: "In this case, RequestStop() was called, and before |arc_session_| had
> finished stopping, RequestStart() was called. Do nothing in that case, since
> when |arc_session_| does actually stop, OnSessionStopped() will be called,
where
> it should automatically restart."

Done.

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.cc:77: if (state() != State::STOPPING) {
On 2016/12/16 23:20:12, Luis Héctor Chávez wrote:
> nit:
> 
> if (state() == State::STOPPING) {
>   // STOPPING is found ...
>   return;
> }
> 
> that way "do nothing" is immediately followed by an explicit return that does
> nothing.

Done.

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.cc:155: // If CONNECTING, ARC instance
has not been booted properly, so do not
On 2016/12/16 23:20:12, Luis Héctor Chávez wrote:
> STARTING?

Done.

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

https://codereview.chromium.org/2577373002/diff/1/components/arc/arc_bridge_s...
components/arc/arc_bridge_service_impl.h:69: bool running_ = false;
On 2016/12/16 23:20:12, Luis Héctor Chávez wrote:
> nit: I'd call this |run_requested_|, because it is possible for this to be
true
> when it is not running, and false when it is.

Wow, good suggestion. Thank you!

[commit-bot: I haz the power, 2016-12-19 08:05: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-19 08:33:27]

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

[commit-bot: I haz the power, 2016-12-19 08:33:28]

Dry run: This issue passed the CQ dry run.

[Luis Héctor Chávez, 2016-12-19 22:24:40]

lgtm

[hidehiko, 2016-12-20 19:37:18]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-12-20 19:37:18]

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/2577373002/#ps60001
(title: "Rebase")

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

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-20 20:19:54]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482262638279730,
"parent_rev": "9c6951f78cdb8f358078d1e9600fc424ab41c648", "commit_rev":
"42c4b46fcd5f2f6da06211b8547fee585e364b29"}

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

Description was changed from

==========
Refactor ArcBridgeServiceImpl part 2.

This CL is the preparation to split ArcSession running parts
from ArcBridgeService.
- Rename ArcBridgeService::State names to be consistent with
  terms used in other ARC modules.
- Remove unused CONNECTED from State.
- Migrate PrerequisitesChanged() and StopInstance() into
  RequestStart() and RequestStop().
- Rename some fields of ArcBridgeServiceImpl for consistency.
- Cleaned up include directives.

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

to

==========
Refactor ArcBridgeServiceImpl part 2.

This CL is the preparation to split ArcSession running parts
from ArcBridgeService.
- Rename ArcBridgeService::State names to be consistent with
  terms used in other ARC modules.
- Remove unused CONNECTED from State.
- Migrate PrerequisitesChanged() and StopInstance() into
  RequestStart() and RequestStop().
- Rename some fields of ArcBridgeServiceImpl for consistency.
- Cleaned up include directives.

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

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

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-20 20:23:48]

Description was changed from

==========
Refactor ArcBridgeServiceImpl part 2.

This CL is the preparation to split ArcSession running parts
from ArcBridgeService.
- Rename ArcBridgeService::State names to be consistent with
  terms used in other ARC modules.
- Remove unused CONNECTED from State.
- Migrate PrerequisitesChanged() and StopInstance() into
  RequestStart() and RequestStop().
- Rename some fields of ArcBridgeServiceImpl for consistency.
- Cleaned up include directives.

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

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

to

==========
Refactor ArcBridgeServiceImpl part 2.

This CL is the preparation to split ArcSession running parts
from ArcBridgeService.
- Rename ArcBridgeService::State names to be consistent with
  terms used in other ARC modules.
- Remove unused CONNECTED from State.
- Migrate PrerequisitesChanged() and StopInstance() into
  RequestStart() and RequestStop().
- Rename some fields of ArcBridgeServiceImpl for consistency.
- Cleaned up include directives.

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

Committed: https://crrev.com/70586e6fd361f66febe643223e61c08caf776c86
Cr-Commit-Position: refs/heads/master@{#439875}
==========

[commit-bot: I haz the power, 2016-12-20 20:23:49]

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