arc: Add ArcServiceManager to own all future ARC services

arc: Add ArcServiceManager to own all future ARC services

Currently the ArcBridgeService is the only ARC related service, but we have
more services coming. To initialize those services along with the
ArcBridgeService, this CL adds an ArcServiceManager which owns, initializes
and destroys them. This way we will not have to litter
ChromeBrowserMainPartsChromeos with multiple ARC services.

BUG=chromium:558499
TEST=This is a refactoring, check that the ArcBridgeService is still operating
     as expected.

Committed: https://crrev.com/00457c2c9713ffde3122212c8a8623886781e251
Cr-Commit-Position: refs/heads/master@{#363522}

[denniskempin, 2015-12-04 00:46:58]

denniskempin@google.com changed reviewers:
+ elijahtaylor@chromium.org, lhchavez@lhchavez.com, oshima@chromium.org

[denniskempin, 2015-12-04 00:46:59]

PTAL. Oshima, this is the change we talked about in person.

[lhc(google), 2015-12-04 02:24:16]

lhchavez@google.com changed reviewers:
+ lhchavez@google.com

[lhc(google), 2015-12-04 02:24:16]

Wasn't the service manager supposed to be in components/arc/service_manager to
avoid adding dependencies directly on components/arc?

[denniskempin, 2015-12-04 18:15:32]

On 2015/12/04 02:24:16, lhc(google) wrote:
> Wasn't the service manager supposed to be in components/arc/service_manager to
> avoid adding dependencies directly on components/arc?
I'm not sure I understand. What difference would it make if the dependencies
were added to components/arc/service_manager an not components/arc?

Either way, we could hide the dependencies of additional services so that the
service manager will not need the additional dependencies. (i.e. have a pimpl
class for ArcInputBridge so that ArcInputBridge won't need to depend on
ui/ash/aura

[oshima, 2015-12-04 18:24:14]

On 2015/12/04 18:15:32, denniskempin wrote:
> On 2015/12/04 02:24:16, lhc(google) wrote:
> > Wasn't the service manager supposed to be in components/arc/service_manager
to
> > avoid adding dependencies directly on components/arc?
> I'm not sure I understand. What difference would it make if the dependencies
> were added to components/arc/service_manager an not components/arc?
> Either way, we could hide the dependencies of additional services so that the
> service manager will not need the additional dependencies. (i.e. have a pimpl
> class for ArcInputBridge so that ArcInputBridge won't need to depend on
> ui/ash/aura

It's probably move important to create a directory for each services (which
means, move bridge service to c/arc/bridge),
and setup proper DPES in each directories.

Since I don't have a sense of how big these services will be, I don't have
strong opinion now though.
I'm fine either way as long as you guys agree.

[Luis Héctor Chávez, 2015-12-04 18:26:51]

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

[Luis Héctor Chávez, 2015-12-04 18:26:51]

Discussed offline, this is the right approach. lgtm

[denniskempin, 2015-12-04 18:30:53]

On 2015/12/04 18:26:51, Luis Héctor Chávez wrote:
> Discussed offline, this is the right approach. lgtm

[denniskempin, 2015-12-04 18:32:15]

On 2015/12/04 18:24:14, oshima wrote:
> On 2015/12/04 18:15:32, denniskempin wrote:
> > On 2015/12/04 02:24:16, lhc(google) wrote:
> > > Wasn't the service manager supposed to be in
components/arc/service_manager
> to
> > > avoid adding dependencies directly on components/arc?
> > I'm not sure I understand. What difference would it make if the dependencies
> > were added to components/arc/service_manager an not components/arc?
> > Either way, we could hide the dependencies of additional services so that
the
> > service manager will not need the additional dependencies. (i.e. have a
pimpl
> > class for ArcInputBridge so that ArcInputBridge won't need to depend on
> > ui/ash/aura
> 
> It's probably move important to create a directory for each services (which
> means, move bridge service to c/arc/bridge),
> and setup proper DPES in each directories.
> 
> Since I don't have a sense of how big these services will be, I don't have
> strong opinion now though.
> I'm fine either way as long as you guys agree.

I agree. Other services will have a subfolder each with DEPS files on them. 
For the arc bridge service, we would like to keep it there for now since there
is too many people actively working on it right now and we do not want to cause
too many conflicts.

[oshima, 2015-12-04 18:33:34]

lgtm

https://codereview.chromium.org/1496193002/diff/20001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/1496193002/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:331:
arc_bridge_service->Shutdown();
I actually have a slight preference for the original style, but I'm ok if you
prefer this way.

https://codereview.chromium.org/1496193002/diff/20001/components/arc/arc_serv...
File components/arc/arc_service_manager.h (right):

https://codereview.chromium.org/1496193002/diff/20001/components/arc/arc_serv...
components/arc/arc_service_manager.h:24: class ArcServiceManager {
Consider adding comment to avoid adding unrelated code/features other than
managing services. It can become "the place to add misc stuff".

[denniskempin, 2015-12-04 19:12:25]

fixed nits! Go go gadget commit queue!

https://codereview.chromium.org/1496193002/diff/20001/chrome/browser/ui/ash/c...
File chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc (right):

https://codereview.chromium.org/1496193002/diff/20001/chrome/browser/ui/ash/c...
chrome/browser/ui/ash/chrome_shell_delegate_chromeos.cc:331:
arc_bridge_service->Shutdown();
On 2015/12/04 18:33:34, oshima wrote:
> I actually have a slight preference for the original style, but I'm ok if you
> prefer this way.
Sure, I don't feel strongly about this!

https://codereview.chromium.org/1496193002/diff/20001/components/arc/arc_serv...
File components/arc/arc_service_manager.h (right):

https://codereview.chromium.org/1496193002/diff/20001/components/arc/arc_serv...
components/arc/arc_service_manager.h:24: class ArcServiceManager {
On 2015/12/04 18:33:34, oshima wrote:
> Consider adding comment to avoid adding unrelated code/features other than
> managing services. It can become "the place to add misc stuff".

Done.

[denniskempin, 2015-12-04 19:31:17]

The CQ bit was checked by denniskempin@google.com

[denniskempin, 2015-12-04 19:31:17]

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/1496193002/#ps40001
(title: "Nit fixes")

[commit-bot: I haz the power, 2015-12-04 19:33:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496193002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496193002/40001

[commit-bot: I haz the power, 2015-12-04 19:48:35]

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

[commit-bot: I haz the power, 2015-12-04 19:48:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[elijahtaylor1, 2015-12-04 23:46:07]

lgtm

[denniskempin, 2015-12-04 23:51:52]

denniskempin@google.com changed reviewers:
+ khmel@chromium.org, xiyuan@chromium.org

[xiyuan, 2015-12-04 23:54:41]

lgtm

[denniskempin, 2015-12-04 23:56:19]

On 2015/12/04 23:54:41, xiyuan wrote:
> lgtm

thanks everyone :)

[denniskempin, 2015-12-04 23:56:22]

The CQ bit was checked by denniskempin@google.com

[denniskempin, 2015-12-04 23:56:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, oshima@chromium.org, elijahtaylor@chromium.org
Link to the patchset: https://codereview.chromium.org/1496193002/#ps80001
(title: "updated arc app list")

[commit-bot: I haz the power, 2015-12-05 00:04:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496193002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496193002/80001

[commit-bot: I haz the power, 2015-12-05 00:45:57]

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

[commit-bot: I haz the power, 2015-12-05 00:45:58]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[denniskempin, 2015-12-05 01:41:29]

The CQ bit was checked by denniskempin@google.com

[denniskempin, 2015-12-05 01:41:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
elijahtaylor@chromium.org, xiyuan@chromium.org, lhchavez@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1496193002/#ps100001
(title: "reverted removal of ArcBridgeService::Get due to dependencies that are
using it in tests")

[denniskempin, 2015-12-05 01:42:26]

The CQ bit was unchecked by denniskempin@google.com

[denniskempin, 2015-12-05 01:47:40]

The CQ bit was checked by denniskempin@google.com

[denniskempin, 2015-12-05 01:47:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
elijahtaylor@chromium.org, xiyuan@chromium.org, lhchavez@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1496193002/#ps140001
(title: "readded file")

[commit-bot: I haz the power, 2015-12-05 01:50:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496193002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496193002/140001

[commit-bot: I haz the power, 2015-12-05 03:54:38]

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

[commit-bot: I haz the power, 2015-12-05 03:54:38]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[denniskempin, 2015-12-05 05:27:45]

The CQ bit was checked by denniskempin@google.com

[commit-bot: I haz the power, 2015-12-05 05:28:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496193002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496193002/140001

[commit-bot: I haz the power, 2015-12-05 07:31:18]

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

[commit-bot: I haz the power, 2015-12-05 07:31:19]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[denniskempin, 2015-12-05 17:00:49]

The CQ bit was checked by denniskempin@google.com

[commit-bot: I haz the power, 2015-12-05 17:03:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496193002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496193002/140001

[commit-bot: I haz the power, 2015-12-05 19:04:59]

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

[commit-bot: I haz the power, 2015-12-05 19:05:00]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)

[Luis Héctor Chávez, 2015-12-07 17:18:59]

The CQ bit was checked by lhchavez@chromium.org

[commit-bot: I haz the power, 2015-12-07 17:19:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1496193002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1496193002/140001

[commit-bot: I haz the power, 2015-12-07 18:28:42]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-07 18:29:37]

Description was changed from

==========
arc: Add ArcServiceManager to own all future ARC services

Currently the ArcBridgeService is the only ARC related service, but we have
more services coming. To initialize those services along with the
ArcBridgeService, this CL adds an ArcServiceManager which owns, initializes
and destroys them. This way we will not have to litter
ChromeBrowserMainPartsChromeos with multiple ARC services.

BUG=chromium:558499
TEST=This is a refactoring, check that the ArcBridgeService is still operating
     as expected.
==========

to

==========
arc: Add ArcServiceManager to own all future ARC services

Currently the ArcBridgeService is the only ARC related service, but we have
more services coming. To initialize those services along with the
ArcBridgeService, this CL adds an ArcServiceManager which owns, initializes
and destroys them. This way we will not have to litter
ChromeBrowserMainPartsChromeos with multiple ARC services.

BUG=chromium:558499
TEST=This is a refactoring, check that the ArcBridgeService is still operating
     as expected.

Committed: https://crrev.com/00457c2c9713ffde3122212c8a8623886781e251
Cr-Commit-Position: refs/heads/master@{#363522}
==========

[commit-bot: I haz the power, 2015-12-07 18:29:38]

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