Pref service: create service at browser startup

Pref service: create service at browser startup

Expose the already existing PrefStore implementations, which are
owned through the profile, through the new prefs service. This
new prefs service will replace the current pref service prototype
used in mash and support synchronous deletions and decoupling the
creation of the underling pref stores.

The service is gated by a default-off feature flag.

BUG=654988
Review-Url: https://codereview.chromium.org/2740493002
Cr-Commit-Position: refs/heads/master@{#455968}
Committed: https://chromium.googlesource.com/chromium/src/+/ab3d36b33fb3412d562033d68c716ddae6e4c3bd

[tibell, 2017-03-07 03:07:18]

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

[commit-bot: I haz the power, 2017-03-07 03:08:04]

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

[tibell, 2017-03-07 03:11:39]

Description was changed from

==========
Prefs service: create service at browser startup

BUG=654988
==========

to

==========
Prefs service: create service at browser startup

The service is gated by a default-off feature flag.

BUG=654988
==========

[tibell, 2017-03-07 03:25:57]

Gate behind feature flag

[tibell, 2017-03-07 03:25:59]

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

[commit-bot: I haz the power, 2017-03-07 03:26:17]

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

[tibell, 2017-03-07 04:13:05]

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

[commit-bot: I haz the power, 2017-03-07 04:14:20]

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

[tibell, 2017-03-07 04:18:23]

Description was changed from

==========
Prefs service: create service at browser startup

The service is gated by a default-off feature flag.

BUG=654988
==========

to

==========
Prefs service: create service at browser startup

Expose the already existing PrefStore implementations, which are owned through
the profile, through the new prefs service. This new prefs service will replace
the current pref service prototype used in mash and support synchronous
deletions and decoupling the creation of the underling pref stores.

The service is gated by a default-off feature flag.

BUG=654988
==========

[tibell, 2017-03-07 04:33:57]

tibell@chromium.org changed reviewers:
+ bauerb@chromium.org, mbarbella@chromium.org, sammc@chromium.org

[tibell, 2017-03-07 04:33:57]

tibell@chromium.org changed reviewers:
+ bauerb@chromium.org, mbarbella@chromium.org, sammc@chromium.org

[tibell, 2017-03-07 04:33:58]

There are several steps in adding the new pref service:

1. Support for exposing non-user pref stores over Mojo. This includes a way to
register them so clients can find them and connect to them.
https://crrev.com/2635153002/
2. *This CL*. Registering the above as a new service.
3. Support for exposing the user pref store over Mojo.
https://crrev.com/2601873002/
4. Change mash to use the new pref service instead of the old one + delete the
old one.

This CL mainly deals with the plumbing of exposing a new service and exposing
the existing pref stores through it, without changing the lifetime of the
existing pref store. The browser still accesses the existing pref stores
directly, not through Mojo. The first service client will be mash.

[tibell, 2017-03-07 04:33:58]

There are several steps in adding the new pref service:

1. Support for exposing non-user pref stores over Mojo. This includes a way to
register them so clients can find them and connect to them.
https://crrev.com/2635153002/
2. *This CL*. Registering the above as a new service.
3. Support for exposing the user pref store over Mojo.
https://crrev.com/2601873002/
4. Change mash to use the new pref service instead of the old one + delete the
old one.

This CL mainly deals with the plumbing of exposing a new service and exposing
the existing pref stores through it, without changing the lifetime of the
existing pref store. The browser still accesses the existing pref stores
directly, not through Mojo. The first service client will be mash.

[tibell, 2017-03-07 04:38:45]

tibell@chromium.org changed reviewers:
+ jam@chromium.org

[tibell, 2017-03-07 04:38:45]

sammc@chromium.org: Please review everything
rockot@chromium.org: Please review
  components/sync_preferences/DEPS
bauerb@chromium.org: Please review
  chrome/browser/prefs/chrome_pref_service_factory.cc [13]
  chrome/browser/prefs/chrome_pref_service_factory.h [13]
  chrome/browser/prefs/preferences2_manifest.json [13]
  components/sync_preferences/BUILD.gn [13]
  components/sync_preferences/DEPS [13]
  components/sync_preferences/pref_service_syncable_factory.cc [13]
  components/sync_preferences/pref_service_syncable_factory.h [13]
jam@chromium.org: Please review
  chrome/app/BUILD.gn
  chrome/browser/BUILD.gn
  chrome/browser/chrome_content_browser_client.h
  chrome/browser/chrome_content_browser_client.cc
  chrome/browser/chrome_content_browser_manifest_overlay.json
  chrome/browser/profiles/profile_impl.cc
  content/browser/browser_context.cc
  content/public/browser/content_browser_client.h

[Sam McNally, 2017-03-07 05:32:04]

https://codereview.chromium.org/2740493002/diff/40001/chrome/browser/prefs/pr...
File chrome/browser/prefs/preferences2_manifest.json (right):

https://codereview.chromium.org/2740493002/diff/40001/chrome/browser/prefs/pr...
chrome/browser/prefs/preferences2_manifest.json:1: {
I think this should be //services/preferences/manifest.json.

https://codereview.chromium.org/2740493002/diff/40001/components/sync_prefere...
File components/sync_preferences/pref_service_syncable_factory.cc (right):

https://codereview.chromium.org/2740493002/diff/40001/components/sync_prefere...
components/sync_preferences/pref_service_syncable_factory.cc:65: // In testing
we might not always register.
Mention the disabled-by-flag case too.

https://codereview.chromium.org/2740493002/diff/40001/components/sync_prefere...
components/sync_preferences/pref_service_syncable_factory.cc:86: auto managed =
CreateRegisteredPrefStore(connector, managed_prefs_.get(),
Remove the .get()s.

[tibell, 2017-03-07 06:25:39]

PTAL

https://codereview.chromium.org/2740493002/diff/40001/chrome/browser/prefs/pr...
File chrome/browser/prefs/preferences2_manifest.json (right):

https://codereview.chromium.org/2740493002/diff/40001/chrome/browser/prefs/pr...
chrome/browser/prefs/preferences2_manifest.json:1: {
On 2017/03/07 05:32:03, Sam McNally wrote:
> I think this should be //services/preferences/manifest.json.

Done.

https://codereview.chromium.org/2740493002/diff/40001/components/sync_prefere...
File components/sync_preferences/pref_service_syncable_factory.cc (right):

https://codereview.chromium.org/2740493002/diff/40001/components/sync_prefere...
components/sync_preferences/pref_service_syncable_factory.cc:65: // In testing
we might not always register.
On 2017/03/07 05:32:04, Sam McNally wrote:
> Mention the disabled-by-flag case too.

Done.

https://codereview.chromium.org/2740493002/diff/40001/components/sync_prefere...
components/sync_preferences/pref_service_syncable_factory.cc:86: auto managed =
CreateRegisteredPrefStore(connector, managed_prefs_.get(),
On 2017/03/07 05:32:04, Sam McNally wrote:
> Remove the .get()s.

Done.

[tibell, 2017-03-07 06:25:46]

Address sammc's review comments

[tibell, 2017-03-07 06:25:48]

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

[commit-bot: I haz the power, 2017-03-07 06:26:16]

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

[Sam McNally, 2017-03-07 06:27:31]

lgtm

[commit-bot: I haz the power, 2017-03-07 07:12:23]

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

[commit-bot: I haz the power, 2017-03-07 07:12:25]

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

[Bernhard Bauer, 2017-03-07 09:43:16]

lgtm

https://codereview.chromium.org/2740493002/diff/60001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2740493002/diff/60001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:3247:
base::MakeUnique<prefs::PrefStoreManagerImpl>(
It's a bit strange to construct a unique_ptr from another unique_ptr. You could
use base::WrapUnique() with new, or maybe add a signature to the lambda and let
the compiler infer the type if that works?

[Martin Barbella, 2017-03-07 21:31:03]

security lgtm

[jam, 2017-03-07 21:41:03]

https://codereview.chromium.org/2740493002/diff/60001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2740493002/diff/60001/content/public/browser/...
content/public/browser/content_browser_client.h:690: virtual void
RegisterInProcessServices(BrowserContext* browser_context,
generally we put stuff in ContentBrowserClient as a last resort, when there's no
other more specific delegate interface. In this case, the method should go on
BrowserContext (i.e. after all the Create* methods there)

[tibell, 2017-03-08 00:05:26]

Address bauerb's and jam's review comments

[tibell, 2017-03-08 00:05:29]

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

[tibell, 2017-03-08 00:05:52]

PTAL

https://codereview.chromium.org/2740493002/diff/60001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2740493002/diff/60001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:3247:
base::MakeUnique<prefs::PrefStoreManagerImpl>(
On 2017/03/07 09:43:16, Bernhard Bauer wrote:
> It's a bit strange to construct a unique_ptr from another unique_ptr. You
could
> use base::WrapUnique() with new, or maybe add a signature to the lambda and
let
> the compiler infer the type if that works?

Done.

https://codereview.chromium.org/2740493002/diff/60001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2740493002/diff/60001/content/public/browser/...
content/public/browser/content_browser_client.h:690: virtual void
RegisterInProcessServices(BrowserContext* browser_context,
On 2017/03/07 21:41:02, jam wrote:
> generally we put stuff in ContentBrowserClient as a last resort, when there's
no
> other more specific delegate interface. In this case, the method should go on
> BrowserContext (i.e. after all the Create* methods there)

Done.

[commit-bot: I haz the power, 2017-03-08 00:06:20]

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, 2017-03-08 00:11:20]

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

[commit-bot: I haz the power, 2017-03-08 00:11:21]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tibell, 2017-03-08 00:29:32]

Merge

[tibell, 2017-03-08 00:29:34]

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

[commit-bot: I haz the power, 2017-03-08 00:30:08]

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

[tibell, 2017-03-08 00:42:12]

Description was changed from

==========
Prefs service: create service at browser startup

Expose the already existing PrefStore implementations, which are owned through
the profile, through the new prefs service. This new prefs service will replace
the current pref service prototype used in mash and support synchronous
deletions and decoupling the creation of the underling pref stores.

The service is gated by a default-off feature flag.

BUG=654988
==========

to

==========
Prefs service: create service at browser startup

Expose the already existing PrefStore implementations, which are
owned through the profile, through the new prefs service. This
new prefs service will replace the current pref service prototype
used in mash and support synchronous deletions and decoupling the
creation of the underling pref stores.

The service is gated by a default-off feature flag.

BUG=654988
==========

[commit-bot: I haz the power, 2017-03-08 02:12:16]

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

[commit-bot: I haz the power, 2017-03-08 02:12:17]

Dry run: This issue passed the CQ dry run.

[tibell, 2017-03-08 03:50:19]

Merge

[tibell, 2017-03-08 03:50:22]

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

[commit-bot: I haz the power, 2017-03-08 03:50:46]

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, 2017-03-08 04:23:17]

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

[commit-bot: I haz the power, 2017-03-08 04:23:18]

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_...)

[tibell, 2017-03-09 05:54:55]

Description was changed from

==========
Prefs service: create service at browser startup

Expose the already existing PrefStore implementations, which are
owned through the profile, through the new prefs service. This
new prefs service will replace the current pref service prototype
used in mash and support synchronous deletions and decoupling the
creation of the underling pref stores.

The service is gated by a default-off feature flag.

BUG=654988
==========

to

==========
Pref service: create service at browser startup

Expose the already existing PrefStore implementations, which are
owned through the profile, through the new prefs service. This
new prefs service will replace the current pref service prototype
used in mash and support synchronous deletions and decoupling the
creation of the underling pref stores.

The service is gated by a default-off feature flag.

BUG=654988
==========

[tibell, 2017-03-10 01:35:23]

Ping jam@chromium.org: I'd really like to submit this before going on vacation,
as sammc@chromium.org has outstanding CLs that depend on it being merged.

[jam, 2017-03-10 01:42:47]

lgtm

[tibell, 2017-03-10 01:43:07]

The CQ bit was checked by tibell@chromium.org

[tibell, 2017-03-10 01:43:07]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
sammc@chromium.org, mbarbella@chromium.org
Link to the patchset: https://codereview.chromium.org/2740493002/#ps120001
(title: "Merge")

[commit-bot: I haz the power, 2017-03-10 01:43:44]

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, 2017-03-10 02:57:30]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489110187508270,
"parent_rev": "43ef44689947a189b86a6f56aef3ab13c87fde9e", "commit_rev":
"ab3d36b33fb3412d562033d68c716ddae6e4c3bd"}

[commit-bot: I haz the power, 2017-03-10 02:58:49]

Description was changed from

==========
Pref service: create service at browser startup

Expose the already existing PrefStore implementations, which are
owned through the profile, through the new prefs service. This
new prefs service will replace the current pref service prototype
used in mash and support synchronous deletions and decoupling the
creation of the underling pref stores.

The service is gated by a default-off feature flag.

BUG=654988
==========

to

==========
Pref service: create service at browser startup

Expose the already existing PrefStore implementations, which are
owned through the profile, through the new prefs service. This
new prefs service will replace the current pref service prototype
used in mash and support synchronous deletions and decoupling the
creation of the underling pref stores.

The service is gated by a default-off feature flag.

BUG=654988

Review-Url: https://codereview.chromium.org/2740493002
Cr-Commit-Position: refs/heads/master@{#455968}
Committed:
https://chromium.googlesource.com/chromium/src/+/ab3d36b33fb3412d562033d68c71...
==========

[commit-bot: I haz the power, 2017-03-10 02:58:51]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ab3d36b33fb3412d562033d68c71...