Make "all user" services work when packaged.

Make sure an "all user" service works when packaged as part of another
service:
- Use the empty string as instance id when none provided instead of the
  service name.
- Disable the tracing service by default.
- changed the ServiceManagerListener.OnServiceStarted notification so it
  is fired when the service has replied (instead of when the process is
  created)
- introduced a ServiceManagerListener notification for when a service
  was not started
- added a new instance_per_child flag to service_manager to control
  whether a new process should be created when starting a new
  packaged service

BUG=658368
TEST=ServiceManagerTest.CreatePackagedSingletonInstance

Committed: https://crrev.com/8dcec2d180e56dcfe84506daac08ef37d7d57a24
Cr-Commit-Position: refs/heads/master@{#428098}

[Jay Civelli, 2016-10-21 19:14:10]

Description was changed from

==========
Fixing package service.

Make sure singleton service can be packaged as part of another service.
Use the empty string as instance id when none provided instead of the
service name.
Disable the tracing service by default.

BUG=
==========

to

==========
Make sure an "all user" service works when packaged as part of another
service:
- 
- Use the empty string as instance id when none provided instead of the
  service name.
- Disable the tracing service by default.
- changed the ServiceManagerListener.OnServiceStarted notification so it
  is fired when the service has replied (instead of when the process is
  created)
- introduced a ServiceManagerListener notification for when a service
  was not started

BUG=658368
==========

[Jay Civelli, 2016-10-21 19:17:31]

Description was changed from

==========
Make sure an "all user" service works when packaged as part of another
service:
- 
- Use the empty string as instance id when none provided instead of the
  service name.
- Disable the tracing service by default.
- changed the ServiceManagerListener.OnServiceStarted notification so it
  is fired when the service has replied (instead of when the process is
  created)
- introduced a ServiceManagerListener notification for when a service
  was not started

BUG=658368
==========

to

==========
Make sure an "all user" service works when packaged as part of another
service:
- Use the empty string as instance id when none provided instead of the
  service name.
- Disable the tracing service by default.
- changed the ServiceManagerListener.OnServiceStarted notification so it
  is fired when the service has replied (instead of when the process is
  created)
- introduced a ServiceManagerListener notification for when a service
  was not started

BUG=658368
TEST=ServiceManagerTest.CreatePackagedSingletonInstance
==========

[Jay Civelli, 2016-10-21 19:17:32]

jcivelli@chromium.org changed reviewers:
+ rockot@chromium.org

[Jay Civelli, 2016-10-21 19:19:23]

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
File services/service_manager/tests/connect/connect_unittest.cc (right):

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
services/service_manager/tests/connect/connect_unittest.cc:211: // When both the
unresolved and resolved instance names are their default
@rockot Do you think this test should go entirely?

[Jay Civelli, 2016-10-21 19:19:30]

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

[commit-bot: I haz the power, 2016-10-21 19:20:31]

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

[Ken Rockot(use gerrit already), 2016-10-21 19:52:33]

lgtm

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
File services/service_manager/service_manager.cc (right):

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
services/service_manager/service_manager.cc:270: // This connection was not
started yet.
nit: in these comments I think s/connection/instance/ is more accurate

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
services/service_manager/service_manager.cc:882: // When part of a packaged app,
use the original user ID.
nit: s/app/service/

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
File services/service_manager/switches.cc (right):

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
services/service_manager/switches.cc:11: const char kEnableTracing[] =
"enable-tracing";
Please change this to --enable-service-manager-tracing

--enable-tracing is already a content switch, and we use ServiceManager in
content.

Maybe they should eventually be the same flag, but I'd rather punt on that
decision.

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
File services/service_manager/tests/connect/connect_unittest.cc (right):

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
services/service_manager/tests/connect/connect_unittest.cc:211: // When both the
unresolved and resolved instance names are their default
On 2016/10/21 at 19:19:23, Jay Civelli wrote:
> @rockot Do you think this test should go entirely?

Yeah, seems reasonable to delete this.

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
File services/service_manager/tests/service_manager/service_manager_unittest.cc
(right):

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
services/service_manager/tests/service_manager/service_manager_unittest.cc:237:
base::MessageLoop::current()->QuitWhenIdle();
nit: QuitWhenIdle() usage in tests is prone to flaky behavior. Would you mind
just updating these tests so that you pass an explicit quit closure around as
needed? e.g.

  void OnServiceStartedCallback(
      int* start_count,
      std::string* service_name,
      const service_manager::Identity& identity,
      const base::Closure& continuation) {
    (*start_count)++;
    *service_name = identity.name();
    continuation.Run();
  }

  {
    base::RunLoop loop;
    set_service_started_callback(base::BindRepeating(
        &OnServiceStartedClalback, &start_count, &service_name,
        loop.QuitClosure());

    // Do stuff...
    loop.Run();
  }

This makes the flow control explicit rather than relying on assumptions about
MessageLoop business.

[commit-bot: I haz the power, 2016-10-21 20:04:40]

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

[commit-bot: I haz the power, 2016-10-21 20:04:41]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Jay Civelli, 2016-10-21 20:48:33]

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
File services/service_manager/service_manager.cc (right):

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
services/service_manager/service_manager.cc:270: // This connection was not
started yet.
On 2016/10/21 19:52:33, Ken Rockot wrote:
> nit: in these comments I think s/connection/instance/ is more accurate

Done.

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
services/service_manager/service_manager.cc:882: // When part of a packaged app,
use the original user ID.
On 2016/10/21 19:52:33, Ken Rockot wrote:
> nit: s/app/service/

Done.

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
File services/service_manager/switches.cc (right):

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
services/service_manager/switches.cc:11: const char kEnableTracing[] =
"enable-tracing";
On 2016/10/21 19:52:33, Ken Rockot wrote:
> Please change this to --enable-service-manager-tracing
> 
> --enable-tracing is already a content switch, and we use ServiceManager in
> content.
> 
> Maybe they should eventually be the same flag, but I'd rather punt on that
> decision.

Done.

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
File services/service_manager/tests/connect/connect_unittest.cc (right):

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
services/service_manager/tests/connect/connect_unittest.cc:211: // When both the
unresolved and resolved instance names are their default
On 2016/10/21 19:52:33, Ken Rockot wrote:
> On 2016/10/21 at 19:19:23, Jay Civelli wrote:
> > @rockot Do you think this test should go entirely?
> 
> Yeah, seems reasonable to delete this.

Removed.

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
File services/service_manager/tests/service_manager/service_manager_unittest.cc
(right):

https://codereview.chromium.org/2440903002/diff/40001/services/service_manage...
services/service_manager/tests/service_manager/service_manager_unittest.cc:237:
base::MessageLoop::current()->QuitWhenIdle();
On 2016/10/21 19:52:33, Ken Rockot wrote:
> nit: QuitWhenIdle() usage in tests is prone to flaky behavior. Would you mind
> just updating these tests so that you pass an explicit quit closure around as
> needed? e.g.
> 
>   void OnServiceStartedCallback(
>       int* start_count,
>       std::string* service_name,
>       const service_manager::Identity& identity,
>       const base::Closure& continuation) {
>     (*start_count)++;
>     *service_name = identity.name();
>     continuation.Run();
>   }
> 
>   {
>     base::RunLoop loop;
>     set_service_started_callback(base::BindRepeating(
>         &OnServiceStartedClalback, &start_count, &service_name,
>         loop.QuitClosure());
> 
>     // Do stuff...
>     loop.Run();
>   }
> 
> This makes the flow control explicit rather than relying on assumptions about
> MessageLoop business.

Great idea, done.

[Jay Civelli, 2016-10-21 20:48:40]

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

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

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-10-21 21:58:14]

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

[commit-bot: I haz the power, 2016-10-21 21:58:15]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Jay Civelli, 2016-10-21 22:40:14]

jcivelli@chromium.org changed reviewers:
+ ben@chromium.org

[Jay Civelli, 2016-10-21 22:40:15]

Adding @ben for OWNER review of mash/task_viewer/task_viewer.cc

[Jay Civelli, 2016-10-21 22:50:26]

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

[commit-bot: I haz the power, 2016-10-21 22:51: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-10-22 01:11:59]

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

[commit-bot: I haz the power, 2016-10-22 01:12:00]

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

[Ben Goodger (Google), 2016-10-23 22:51:05]

lgtm

[Jay Civelli, 2016-10-24 15:49:05]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2016-10-24 15:49:05]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2440903002/#ps80001
(title: "Fix the ServiceManagerTest.CreateInstance")

[commit-bot: I haz the power, 2016-10-24 15:49:32]

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-10-24 16:17:08]

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

[commit-bot: I haz the power, 2016-10-24 16:17:09]

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

[Jay Civelli, 2016-10-24 16:24:01]

jcivelli@chromium.org changed reviewers:
+ tsepez@chromium.org

[Jay Civelli, 2016-10-24 16:25:02]

Adding @tsepez for IPC security OWNER review for change to
services/service_manager/public/interfaces/service_manager.mojom.

Thanks.

[Tom Sepez, 2016-10-24 20:02:15]

lgtm

[Jay Civelli, 2016-10-24 20:05:59]

The CQ bit was checked by jcivelli@chromium.org

[commit-bot: I haz the power, 2016-10-24 20:06:51]

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-10-24 21:46:52]

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

[commit-bot: I haz the power, 2016-10-24 21:46:53]

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

[Jay Civelli, 2016-10-26 21:00:01]

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

[commit-bot: I haz the power, 2016-10-26 21:00:55]

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

[Jay Civelli, 2016-10-26 21:01:20]

Fixed the Mash browser test.
@rockot could you take a look?

Thanks.

[commit-bot: I haz the power, 2016-10-26 22:33:27]

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

[commit-bot: I haz the power, 2016-10-26 22:33:28]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-10-27 17:01:29]

LGTM!

https://codereview.chromium.org/2440903002/diff/160001/chrome/test/base/mash_...
File chrome/test/base/mash_browser_tests_manifest.json (right):

https://codereview.chromium.org/2440903002/diff/160001/chrome/test/base/mash_...
chrome/test/base/mash_browser_tests_manifest.json:13: "service:service_manager":
[
Please also add this to chrome/app/mash/chrome_mash_manifest.json, or we'll
break chrome --mash. We should probably find a way to make browser_tests use
that manifest instead, but that's a yak-shave for another time.

https://codereview.chromium.org/2440903002/diff/160001/services/catalog/entry.cc
File services/catalog/entry.cc (right):

https://codereview.chromium.org/2440903002/diff/160001/services/catalog/entry...
services/catalog/entry.cc:264:
service_manager::mojom::kServiceManager_ConnectorSpec);
note to self: this is weird, we should probably put service manager capability
declarations (e.g. all_users, instance_per_child) somewhere that isn't
interface_provider_specs. let's someday soon introduce a service_options key
into the manifest toplevel, and we can do more reasonable things like

  "service_options": {
    "all_users": true,
    "instance_per_child": true
  }

etc

[Jay Civelli, 2016-10-27 17:43:28]

https://codereview.chromium.org/2440903002/diff/160001/chrome/test/base/mash_...
File chrome/test/base/mash_browser_tests_manifest.json (right):

https://codereview.chromium.org/2440903002/diff/160001/chrome/test/base/mash_...
chrome/test/base/mash_browser_tests_manifest.json:13: "service:service_manager":
[
On 2016/10/27 17:01:29, Ken Rockot wrote:
> Please also add this to chrome/app/mash/chrome_mash_manifest.json, or we'll
> break chrome --mash. We should probably find a way to make browser_tests use
> that manifest instead, but that's a yak-shave for another time.

Good catch! Done and verified chrome mash still works.

https://codereview.chromium.org/2440903002/diff/160001/services/catalog/entry.cc
File services/catalog/entry.cc (right):

https://codereview.chromium.org/2440903002/diff/160001/services/catalog/entry...
services/catalog/entry.cc:264:
service_manager::mojom::kServiceManager_ConnectorSpec);
On 2016/10/27 17:01:29, Ken Rockot wrote:
> note to self: this is weird, we should probably put service manager capability
> declarations (e.g. all_users, instance_per_child) somewhere that isn't
> interface_provider_specs. let's someday soon introduce a service_options key
> into the manifest toplevel, and we can do more reasonable things like
> 
>   "service_options": {
>     "all_users": true,
>     "instance_per_child": true
>   }
> 
> etc

Acknowledged.

[Jay Civelli, 2016-10-27 17:43:36]

The CQ bit was checked by jcivelli@chromium.org

[Jay Civelli, 2016-10-27 17:43:37]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
ben@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2440903002/#ps180001
(title: "Addressed latest comments and synced.")

[commit-bot: I haz the power, 2016-10-27 17:44:20]

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

[Jay Civelli, 2016-10-27 17:48:03]

Description was changed from

==========
Make sure an "all user" service works when packaged as part of another
service:
- Use the empty string as instance id when none provided instead of the
  service name.
- Disable the tracing service by default.
- changed the ServiceManagerListener.OnServiceStarted notification so it
  is fired when the service has replied (instead of when the process is
  created)
- introduced a ServiceManagerListener notification for when a service
  was not started

BUG=658368
TEST=ServiceManagerTest.CreatePackagedSingletonInstance
==========

to

==========
Make sure an "all user" service works when packaged as part of another
service:
- Use the empty string as instance id when none provided instead of the
  service name.
- Disable the tracing service by default.
- changed the ServiceManagerListener.OnServiceStarted notification so it
  is fired when the service has replied (instead of when the process is
  created)
- introduced a ServiceManagerListener notification for when a service
  was not started
- added a new instance_per_child flag to service_manager to control
  whether a new process should be created when starting a new
  packaged service

BUG=658368
TEST=ServiceManagerTest.CreatePackagedSingletonInstance
==========

[commit-bot: I haz the power, 2016-10-27 18:54:46]

Description was changed from

==========
Make sure an "all user" service works when packaged as part of another
service:
- Use the empty string as instance id when none provided instead of the
  service name.
- Disable the tracing service by default.
- changed the ServiceManagerListener.OnServiceStarted notification so it
  is fired when the service has replied (instead of when the process is
  created)
- introduced a ServiceManagerListener notification for when a service
  was not started
- added a new instance_per_child flag to service_manager to control
  whether a new process should be created when starting a new
  packaged service

BUG=658368
TEST=ServiceManagerTest.CreatePackagedSingletonInstance
==========

to

==========
Make sure an "all user" service works when packaged as part of another
service:
- Use the empty string as instance id when none provided instead of the
  service name.
- Disable the tracing service by default.
- changed the ServiceManagerListener.OnServiceStarted notification so it
  is fired when the service has replied (instead of when the process is
  created)
- introduced a ServiceManagerListener notification for when a service
  was not started
- added a new instance_per_child flag to service_manager to control
  whether a new process should be created when starting a new
  packaged service

BUG=658368
TEST=ServiceManagerTest.CreatePackagedSingletonInstance
==========

[commit-bot: I haz the power, 2016-10-27 18:54:48]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-10-27 19:02:03]

Description was changed from

==========
Make sure an "all user" service works when packaged as part of another
service:
- Use the empty string as instance id when none provided instead of the
  service name.
- Disable the tracing service by default.
- changed the ServiceManagerListener.OnServiceStarted notification so it
  is fired when the service has replied (instead of when the process is
  created)
- introduced a ServiceManagerListener notification for when a service
  was not started
- added a new instance_per_child flag to service_manager to control
  whether a new process should be created when starting a new
  packaged service

BUG=658368
TEST=ServiceManagerTest.CreatePackagedSingletonInstance
==========

to

==========
Make sure an "all user" service works when packaged as part of another
service:
- Use the empty string as instance id when none provided instead of the
  service name.
- Disable the tracing service by default.
- changed the ServiceManagerListener.OnServiceStarted notification so it
  is fired when the service has replied (instead of when the process is
  created)
- introduced a ServiceManagerListener notification for when a service
  was not started
- added a new instance_per_child flag to service_manager to control
  whether a new process should be created when starting a new
  packaged service

BUG=658368
TEST=ServiceManagerTest.CreatePackagedSingletonInstance

Committed: https://crrev.com/8dcec2d180e56dcfe84506daac08ef37d7d57a24
Cr-Commit-Position: refs/heads/master@{#428098}
==========

[commit-bot: I haz the power, 2016-10-27 19:02:04]

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