Refactor ArcSessionRunner part 1.

Refactor ArcSessionRunner part 1.

This CL renames ArcBridgeServiceImpl to ArcSessionRunner,
and had  as a preparation to get rid of inheritance.
Along with the change, FakeArcBridgeService is removed
and replaced by ArcSessionRunner + FakeArcSession factory.

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

Committed: https://crrev.com/359b352bc1b60c0de02e27d2373078d17a5473a2
Cr-Commit-Position: refs/heads/master@{#440019}

[hidehiko, 2016-12-14 18:56:16]

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

[commit-bot: I haz the power, 2016-12-14 18:57:14]

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-14 19:04:15]

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

[commit-bot: I haz the power, 2016-12-14 19:04:16]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hidehiko, 2016-12-14 19:50:17]

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

[commit-bot: I haz the power, 2016-12-14 19:50:58]

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-14 19:53:34]

Patchset #2 (id:20001) has been deleted

[hidehiko, 2016-12-14 19:53:41]

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

[commit-bot: I haz the power, 2016-12-14 19:54:13]

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-14 20:01:06]

hidehiko@chromium.org changed reviewers:
+ bartfab@chromium.org, benwells@chromium.org, lhchavez@chromium.org,
maxbogue@chromium.org, xiyuan@chromium.org, yoshiki@chromium.org

[hidehiko, 2016-12-14 20:01:07]

PTAL.

Luis, if you prefer a number of relatively smaller CLs, please let me know. I'll
give it a try to split this into pieces. (Though, these are pretty conflicting
each other, so I thought this may be slightly easier to review...)

lhchavez@: All, specifically, c/b/c/arc, components/arc,
bartfab@: c/b/policy
maxbogue@: c/b/sync
xiyuan@: c/b/ui/app_list
benwells@: c/b/ui/views
yoshiki@: ui/arc

Thank you for review in advance,
- hidehiko

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

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

[commit-bot: I haz the power, 2016-12-14 21:03:23]

Dry run: This issue passed the CQ dry run.

[benwells, 2016-12-15 05:51:35]

c/b/ui/views/apps lgtm

[yoshiki, 2016-12-15 09:34:23]

ui/arc lgtm

[maxbogue, 2016-12-15 15:42:01]

c/b/sync/ lgtm

[xiyuan, 2016-12-15 17:44:50]

c/b/ui/app_list lgtm

[hidehiko, 2016-12-16 12:12:40]

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

[commit-bot: I haz the power, 2016-12-16 12:13:00]

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:14:54]

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

[commit-bot: I haz the power, 2016-12-16 12:14:55]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[hidehiko, 2016-12-16 12:20:41]

Thank you for review.

Luis, according to the time stamp, I believe you haven't yet taken a look.
Thus, I split this CL's main parts (components/arc/*) into several CLs, and
rebased on top of them, which looks easier to review to me, and I hope it works
better for you, too.

All dependent CLs are sent to you for review. PTAL this CL, too.

Note: the bot failure looks not a real one. It is patch failure, but no log. So,
I believe, it is fixed once all dependent CLs are landed, and rebased back on
top of master.

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

Yes, thank you very much for splitting the CLs :D

Just confirming: the commit message mentions that it does some things, but IIUC
they were moved to the other CLs:

- Rework state machine.
- Rework restarting by using OneShotTimer.
- Rework restarting timeout for testing (no synchronous call).
- Remove friend for test code.

Is that true? Or did I miss something?

https://codereview.chromium.org/2574013003/diff/80001/components/arc/BUILD.gn
File components/arc/BUILD.gn (right):

https://codereview.chromium.org/2574013003/diff/80001/components/arc/BUILD.gn...
components/arc/BUILD.gn:80: "//mojo/edk/system",
Can this be removed?

https://codereview.chromium.org/2574013003/diff/80001/components/arc/arc_serv...
File components/arc/arc_service_manager.cc (right):

https://codereview.chromium.org/2574013003/diff/80001/components/arc/arc_serv...
components/arc/arc_service_manager.cc:63: arc_bridge_service_.reset(new
ArcSessionRunner(blocking_task_runner));
nit: base::MakeUnique

https://codereview.chromium.org/2574013003/diff/80001/components/arc/arc_sess...
File components/arc/arc_session_runner.h (left):

https://codereview.chromium.org/2574013003/diff/80001/components/arc/arc_sess...
components/arc/arc_session_runner.h:53: // however, we'd like it to happen
immediately to avoid adding unnecessary
why did you remove this? In this context, "it" == automatic restarting.

[hidehiko, 2016-12-19 08:24:57]

Description was changed from

==========
Refactor ArcSessionRunner part 1.

This CL is a couple of refactoring work to split
ArcSession running part from ArcBridgeService.
- Rename ArcBridgeServiceImpl to ArcSessionRunner and expose it
  as preparation to move it to the ArcSessionManager.
- Rework state machine.
- Rework restarting by using OneShotTimer.
- Rework restarting timeout for testing (no synchronous call).
- Remove FakeArcBridgeService.
- Remove friend for test code.

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

to

==========
Refactor ArcSessionRunner part 1.

This CL renames ArcBridgeServiceImpl to ArcSessionRunner,
and had  as a preparation to get rid of inheritance.
Along with the change, FakeArcBridgeService is removed
and replaced by ArcSessionRunner + FakeArcSession factory.

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

[hidehiko, 2016-12-19 08:27:04]

Thank you for review.

Luis, you're right about the splitting, and sorry I overlooked to update the CL
description.

Now it is updated.

PTAL.

Note: I'll re-run the trybots when dependent CL is landed then this is rebased
on top of master (considering the previous patch failing, I'm not sure why
though).

https://codereview.chromium.org/2574013003/diff/80001/components/arc/BUILD.gn
File components/arc/BUILD.gn (right):

https://codereview.chromium.org/2574013003/diff/80001/components/arc/BUILD.gn...
components/arc/BUILD.gn:80: "//mojo/edk/system",
On 2016/12/16 23:31:00, Luis Héctor Chávez wrote:
> Can this be removed?

Unfortunately, we cannot.
Some other modules (crash_reporter, bluetooth) directly depends on mojo::edk
(specifically ScopedHandler etc), and if we do not have this here, gn reports an
error.

https://codereview.chromium.org/2574013003/diff/80001/components/arc/arc_serv...
File components/arc/arc_service_manager.cc (right):

https://codereview.chromium.org/2574013003/diff/80001/components/arc/arc_serv...
components/arc/arc_service_manager.cc:63: arc_bridge_service_.reset(new
ArcSessionRunner(blocking_task_runner));
On 2016/12/16 23:31:00, Luis Héctor Chávez wrote:
> nit: base::MakeUnique

Done.

https://codereview.chromium.org/2574013003/diff/80001/components/arc/arc_sess...
File components/arc/arc_session_runner.h (left):

https://codereview.chromium.org/2574013003/diff/80001/components/arc/arc_sess...
components/arc/arc_session_runner.h:53: // however, we'd like it to happen
immediately to avoid adding unnecessary
On 2016/12/16 23:31:00, Luis Héctor Chávez wrote:
> why did you remove this? In this context, "it" == automatic restarting.

Oh, I misinterprested the sentence. Reverted. Thank you!

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

lgtm with some minor nits.

btw if you want to land this while the tree is quiet, it looks like a good
candidate for TBR-ing:
https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review...

https://codereview.chromium.org/2574013003/diff/120001/components/arc/arc_ser...
File components/arc/arc_service_manager.h (right):

https://codereview.chromium.org/2574013003/diff/120001/components/arc/arc_ser...
components/arc/arc_service_manager.h:70: // creation. ArcServiceManager owns
|arc_bridge_service|.
nit: "ArcServiceManager owns |arc_session_runner|."

Then again, you're passing a std::unique_ptr, which is a very clear indication
that you are transferring ownership, so the last part of the comment is
redundant.

https://codereview.chromium.org/2574013003/diff/120001/components/arc/arc_ses...
File components/arc/arc_session_runner_unittest.cc (right):

https://codereview.chromium.org/2574013003/diff/120001/components/arc/arc_ses...
components/arc/arc_session_runner_unittest.cc:89: base::MessageLoopForUI
message_loop_;
Do we still need this? It seems to be unused.

https://codereview.chromium.org/2574013003/diff/120001/components/arc/arc_ses...
components/arc/arc_session_runner_unittest.cc:101: bool IsReadyCalled() { return
ready_called_; }
super optional nit: bool ready_called() const { return ready_called_; }

same below.

https://codereview.chromium.org/2574013003/diff/120001/components/arc/arc_ses...
components/arc/arc_session_runner_unittest.cc:123: arc_session_runner(),
&observer));
Don't you need base::Unretained? ArcSessionRunner is not a ref-counted class.

Also, why do you (still) need this? You added an explicit call to
arc_session_runner()->RemoveObserver(&observer) in L134, and there is no other
way to exit this method.

[hidehiko, 2016-12-21 03:39:26]

Description was changed from

==========
Refactor ArcSessionRunner part 1.

This CL renames ArcBridgeServiceImpl to ArcSessionRunner,
and had  as a preparation to get rid of inheritance.
Along with the change, FakeArcBridgeService is removed
and replaced by ArcSessionRunner + FakeArcSession factory.

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

to

==========
Refactor ArcSessionRunner part 1.

This CL renames ArcBridgeServiceImpl to ArcSessionRunner,
and had  as a preparation to get rid of inheritance.
Along with the change, FakeArcBridgeService is removed
and replaced by ArcSessionRunner + FakeArcSession factory.

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

[hidehiko, 2016-12-21 03:40:37]

Thank you for review.
Submitting with TBR=bartfab, actually this case meets the condition described in
the doc Luis linked. Thank you for the suggestion, too!

https://codereview.chromium.org/2574013003/diff/120001/components/arc/arc_ser...
File components/arc/arc_service_manager.h (right):

https://codereview.chromium.org/2574013003/diff/120001/components/arc/arc_ser...
components/arc/arc_service_manager.h:70: // creation. ArcServiceManager owns
|arc_bridge_service|.
On 2016/12/19 22:21:37, Luis Héctor Chávez wrote:
> nit: "ArcServiceManager owns |arc_session_runner|."
> 
> Then again, you're passing a std::unique_ptr, which is a very clear indication
> that you are transferring ownership, so the last part of the comment is
> redundant.

Done to drop the last sentence.

https://codereview.chromium.org/2574013003/diff/120001/components/arc/arc_ses...
File components/arc/arc_session_runner_unittest.cc (right):

https://codereview.chromium.org/2574013003/diff/120001/components/arc/arc_ses...
components/arc/arc_session_runner_unittest.cc:89: base::MessageLoopForUI
message_loop_;
On 2016/12/19 22:21:37, Luis Héctor Chávez wrote:
> Do we still need this? It seems to be unused.

Yes, we need. Some tests run RunLoop().
This is not a regression, and no change from arc_bridge_service_unittest.cc, so
let me keep it as is for now.
I'm happy to clean up if necessary in a separate CL.

https://codereview.chromium.org/2574013003/diff/120001/components/arc/arc_ses...
components/arc/arc_session_runner_unittest.cc:101: bool IsReadyCalled() { return
ready_called_; }
On 2016/12/19 22:21:37, Luis Héctor Chávez wrote:
> super optional nit: bool ready_called() const { return ready_called_; }
> 
> same below.

I personally prefer "is", "has", "can" prefixes etc., but that's not a big
matter. Followed your suggestion.

https://codereview.chromium.org/2574013003/diff/120001/components/arc/arc_ses...
components/arc/arc_session_runner_unittest.cc:123: arc_session_runner(),
&observer));
On 2016/12/19 22:21:38, Luis Héctor Chávez wrote:
> Don't you need base::Unretained? ArcSessionRunner is not a ref-counted class.
> 
> Also, why do you (still) need this? You added an explicit call to
> arc_session_runner()->RemoveObserver(&observer) in L134, and there is no other
> way to exit this method.

> base::Unretained.
We do not need in this case. Unretained is for the class method.

> RemoveObserver() at the end.
It was my mistake on rebasing. I rebased again and it's now gone.

[hidehiko, 2016-12-21 03:40:44]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-12-21 03:40:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org, xiyuan@chromium.org, maxbogue@chromium.org,
yoshiki@chromium.org, lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2574013003/#ps160001
(title: "Address comments")

[commit-bot: I haz the power, 2016-12-21 03:41:05]

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-21 04:49:39]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1482291644192780,
"parent_rev": "5572f9d99f029633312dc7f1bde85fa501e1775d", "commit_rev":
"4a85e4adab768737c1c2e2655ad109791515e991"}

[commit-bot: I haz the power, 2016-12-21 04:50:05]

Description was changed from

==========
Refactor ArcSessionRunner part 1.

This CL renames ArcBridgeServiceImpl to ArcSessionRunner,
and had  as a preparation to get rid of inheritance.
Along with the change, FakeArcBridgeService is removed
and replaced by ArcSessionRunner + FakeArcSession factory.

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

to

==========
Refactor ArcSessionRunner part 1.

This CL renames ArcBridgeServiceImpl to ArcSessionRunner,
and had  as a preparation to get rid of inheritance.
Along with the change, FakeArcBridgeService is removed
and replaced by ArcSessionRunner + FakeArcSession factory.

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

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

[commit-bot: I haz the power, 2016-12-21 04:50:06]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-12-21 04:52:15]

Description was changed from

==========
Refactor ArcSessionRunner part 1.

This CL renames ArcBridgeServiceImpl to ArcSessionRunner,
and had  as a preparation to get rid of inheritance.
Along with the change, FakeArcBridgeService is removed
and replaced by ArcSessionRunner + FakeArcSession factory.

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

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

to

==========
Refactor ArcSessionRunner part 1.

This CL renames ArcBridgeServiceImpl to ArcSessionRunner,
and had  as a preparation to get rid of inheritance.
Along with the change, FakeArcBridgeService is removed
and replaced by ArcSessionRunner + FakeArcSession factory.

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

Committed: https://crrev.com/359b352bc1b60c0de02e27d2373078d17a5473a2
Cr-Commit-Position: refs/heads/master@{#440019}
==========

[commit-bot: I haz the power, 2016-12-21 04:52:17]

Patchset 8 (id:??) landed as
https://crrev.com/359b352bc1b60c0de02e27d2373078d17a5473a2
Cr-Commit-Position: refs/heads/master@{#440019}