Pref service: have Mash and Chrome connect to the right pref stores

Pref service: have Mash and Chrome connect to the right pref stores

Chrome contains the implementation of all pref stores, except the
default pref store, in-process and thus only needs to connect to
one store. Mash needs to connect to all pref stores.

This is achieved by having clients specify which pref stores they
already know of in-process, rather than the other way
around. That way e.g. mash doesn't need to know which pref stores
Chrome exposes to it.

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

[tibell, 2017-04-03 06:54:53]

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

[commit-bot: I haz the power, 2017-04-03 06:55: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-04-03 06:56:26]

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

[tibell, 2017-04-03 07:04:26]

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

[tibell, 2017-04-03 07:04:26]

sammc@chromium.org: Please review all changes as the primary reviewer.

bauerb@chromium.org: Please review changes in 
- chrome/browser/prefs/*

sky@chromium.org: Please review changes in
- ash/shell.cc
- chrome/browser/profiles/profile_impl.cc

mbarbella@chromium.org: Please review changes in 
- services/preferences/public/interfaces/preferences.mojom

[tibell, 2017-04-03 07:06:42]

Description was changed from

==========
Pref service: have Mash and Chrome connect to the right pref stores

Chrome contains the implementation of all pref stores, except the default pref
store, in-process and thus only needs to connect to one store. Mash needs to
connect to all pref stores.

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

to

==========
Pref service: have Mash and Chrome connect to the right pref stores

Chrome contains the implementation of all pref stores, except the
default pref store, in-process and thus only needs to connect to
one store. Mash needs to connect to all pref stores.

This is achieved by having clients specify which pref stores they
already know of in-process, rather than the other way
around. That way e.g. mash doesn't need to know which pref stores
Chrome exposes to it.

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

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

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

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

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tibell, 2017-04-03 07:15:47]

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

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

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-04-03 07:40:42]

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

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

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

[Sam McNally, 2017-04-03 10:09:32]

lgtm

https://codereview.chromium.org/2791903003/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2791903003/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:817: PrefValueStore::COMMAND_LINE_STORE,
PrefValueStore::USER_STORE,
Remove USER_STORE.

https://codereview.chromium.org/2791903003/diff/1/chrome/browser/prefs/profil...
File chrome/browser/prefs/profile_pref_store_manager.cc (right):

https://codereview.chromium.org/2791903003/diff/1/chrome/browser/prefs/profil...
chrome/browser/prefs/profile_pref_store_manager.cc:122:
std::vector<PrefValueStore::PrefStoreType> in_process_types;
in_process_types(in_process_types_set.begin(), in_process_types_set.end());

https://codereview.chromium.org/2791903003/diff/1/services/preferences/pref_s...
File services/preferences/pref_store_manager_impl.cc (right):

https://codereview.chromium.org/2791903003/diff/1/services/preferences/pref_s...
services/preferences/pref_store_manager_impl.cc:25: const mojom::PrefStorePtr&>;
mojom::PrefStore*

https://codereview.chromium.org/2791903003/diff/1/services/preferences/pref_s...
File services/preferences/pref_store_manager_impl.h (right):

https://codereview.chromium.org/2791903003/diff/1/services/preferences/pref_s...
services/preferences/pref_store_manager_impl.h:53: struct PendingConnect {
Could this be defined in the .cc file?

[Bernhard Bauer, 2017-04-03 10:37:40]

https://codereview.chromium.org/2791903003/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2791903003/diff/20001/ash/shell.cc#newcode802
ash/shell.cc:802: make_scoped_refptr(new PrefRegistrySimple()), {},
Nit: Add a comment /* already_connected_types = */ before the argument?
Otherwise it's a bit hard to tell at a glance what that parameter is for.

https://codereview.chromium.org/2791903003/diff/20001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2791903003/diff/20001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:796: PrefValueStore::MANAGED_STORE,
<sigh> I guess clang-format doesn't deal with initializer lists properly...

https://codereview.chromium.org/2791903003/diff/20001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:804: PrefValueStore::DEFAULT_STORE
Why is the USER_STORE missing here?

https://codereview.chromium.org/2791903003/diff/20001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:808:
std::set<PrefValueStore::PrefStoreType> InProcessPrefStores() {
I guess you could really implement this method in terms of ExpectedPrefStores()
and just remove the DEFAULT_STORE?

https://codereview.chromium.org/2791903003/diff/20001/services/preferences/pr...
File services/preferences/pref_store_manager_impl.cc (right):

https://codereview.chromium.org/2791903003/diff/20001/services/preferences/pr...
services/preferences/pref_store_manager_impl.cc:118:
expected_pref_stores_.insert(PrefValueStore::DEFAULT_STORE);
Remove this now that we explicitly pass in DEFAULT_STORE here? Or maybe replace
it with a DCHECK that DEFAULT_STORE is included?

[sky, 2017-04-03 16:28:13]

LGTM

[Martin Barbella, 2017-04-03 18:17:12]

mojom lgtm

[tibell, 2017-04-04 04:05:36]

Address review comments

[tibell, 2017-04-04 04:05:39]

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

[tibell, 2017-04-04 04:05:59]

PTAL

https://codereview.chromium.org/2791903003/diff/1/chrome/browser/prefs/browse...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2791903003/diff/1/chrome/browser/prefs/browse...
chrome/browser/prefs/browser_prefs.cc:817: PrefValueStore::COMMAND_LINE_STORE,
PrefValueStore::USER_STORE,
On 2017/04/03 10:09:31, Sam McNally wrote:
> Remove USER_STORE.

Done.

https://codereview.chromium.org/2791903003/diff/1/chrome/browser/prefs/profil...
File chrome/browser/prefs/profile_pref_store_manager.cc (right):

https://codereview.chromium.org/2791903003/diff/1/chrome/browser/prefs/profil...
chrome/browser/prefs/profile_pref_store_manager.cc:122:
std::vector<PrefValueStore::PrefStoreType> in_process_types;
On 2017/04/03 10:09:31, Sam McNally wrote:
> in_process_types(in_process_types_set.begin(), in_process_types_set.end());

Done.

https://codereview.chromium.org/2791903003/diff/1/services/preferences/pref_s...
File services/preferences/pref_store_manager_impl.cc (right):

https://codereview.chromium.org/2791903003/diff/1/services/preferences/pref_s...
services/preferences/pref_store_manager_impl.cc:25: const mojom::PrefStorePtr&>;
On 2017/04/03 10:09:31, Sam McNally wrote:
> mojom::PrefStore*

Done.

https://codereview.chromium.org/2791903003/diff/1/services/preferences/pref_s...
File services/preferences/pref_store_manager_impl.h (right):

https://codereview.chromium.org/2791903003/diff/1/services/preferences/pref_s...
services/preferences/pref_store_manager_impl.h:53: struct PendingConnect {
On 2017/04/03 10:09:31, Sam McNally wrote:
> Could this be defined in the .cc file?

Done.

https://codereview.chromium.org/2791903003/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2791903003/diff/20001/ash/shell.cc#newcode802
ash/shell.cc:802: make_scoped_refptr(new PrefRegistrySimple()), {},
On 2017/04/03 10:37:39, Bernhard (slow until Apr 6) wrote:
> Nit: Add a comment /* already_connected_types = */ before the argument?
> Otherwise it's a bit hard to tell at a glance what that parameter is for.

I instead changed it to spell out the type, since {} doesn't work on all of our
compilers anyway. :/

https://codereview.chromium.org/2791903003/diff/20001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2791903003/diff/20001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:796: PrefValueStore::MANAGED_STORE,
On 2017/04/03 10:37:39, Bernhard (slow until Apr 6) wrote:
> <sigh> I guess clang-format doesn't deal with initializer lists properly... 

Done.

https://codereview.chromium.org/2791903003/diff/20001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:804: PrefValueStore::DEFAULT_STORE
On 2017/04/03 10:37:40, Bernhard (slow until Apr 6) wrote:
> Why is the USER_STORE missing here?

It's always present, but I changed the code to allow it here for completeness
(the service itself ignores it if in the list).

https://codereview.chromium.org/2791903003/diff/20001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:808:
std::set<PrefValueStore::PrefStoreType> InProcessPrefStores() {
On 2017/04/03 10:37:39, Bernhard (slow until Apr 6) wrote:
> I guess you could really implement this method in terms of
ExpectedPrefStores()
> and just remove the DEFAULT_STORE?

Done.

https://codereview.chromium.org/2791903003/diff/20001/services/preferences/pr...
File services/preferences/pref_store_manager_impl.cc (right):

https://codereview.chromium.org/2791903003/diff/20001/services/preferences/pr...
services/preferences/pref_store_manager_impl.cc:118:
expected_pref_stores_.insert(PrefValueStore::DEFAULT_STORE);
On 2017/04/03 10:37:40, Bernhard (slow until Apr 6) wrote:
> Remove this now that we explicitly pass in DEFAULT_STORE here? Or maybe
replace
> it with a DCHECK that DEFAULT_STORE is included?

Done.

[commit-bot: I haz the power, 2017-04-04 04:06:09]

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-04-04 04:34:54]

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

[commit-bot: I haz the power, 2017-04-04 04:34:55]

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

[tibell, 2017-04-04 05:03:25]

PrefServiceFactoryTest: fix service creation

[tibell, 2017-04-04 05:03:28]

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

[commit-bot: I haz the power, 2017-04-04 05:03:42]

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

[tibell, 2017-04-04 05:29:19]

Rebase

[tibell, 2017-04-04 05:29:22]

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

[commit-bot: I haz the power, 2017-04-04 05:29:36]

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

[tibell, 2017-04-04 05:48:16]

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

[commit-bot: I haz the power, 2017-04-04 05:48:26]

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-04-04 06:46:40]

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

[commit-bot: I haz the power, 2017-04-04 06:46:41]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-04-05 10:04:32]

lgtm

https://codereview.chromium.org/2791903003/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2791903003/diff/20001/ash/shell.cc#newcode802
ash/shell.cc:802: make_scoped_refptr(new PrefRegistrySimple()), {},
On 2017/04/04 04:05:59, tibell wrote:
> On 2017/04/03 10:37:39, Bernhard (slow until Apr 6) wrote:
> > Nit: Add a comment /* already_connected_types = */ before the argument?
> > Otherwise it's a bit hard to tell at a glance what that parameter is for.
> 
> I instead changed it to spell out the type, since {} doesn't work on all of
our
> compilers anyway. :/

:-/ An alternative might be to add a helper method named something like
NoPrefStoreTypes() that will return an empty vector.

https://codereview.chromium.org/2791903003/diff/20001/services/preferences/pr...
File services/preferences/pref_store_manager_impl.cc (right):

https://codereview.chromium.org/2791903003/diff/20001/services/preferences/pr...
services/preferences/pref_store_manager_impl.cc:118:
expected_pref_stores_.insert(PrefValueStore::DEFAULT_STORE);
On 2017/04/04 04:05:59, tibell wrote:
> On 2017/04/03 10:37:40, Bernhard (slow until Apr 6) wrote:
> > Remove this now that we explicitly pass in DEFAULT_STORE here? Or maybe
> replace
> > it with a DCHECK that DEFAULT_STORE is included?
> 
> Done.

Not really? :)

Could you also add a comment that explains why we modify the set of expected
pref stores?

https://codereview.chromium.org/2791903003/diff/100001/services/preferences/p...
File services/preferences/pref_store_manager_impl.cc (right):

https://codereview.chromium.org/2791903003/diff/100001/services/preferences/p...
services/preferences/pref_store_manager_impl.cc:253:
std::find(already_connected_types.begin(),
Nit: base::ContainsValue() (from base/stl_util.h) does this, and looks slightly
more concise.

[tibell, 2017-04-06 01:05:49]

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

[tibell, 2017-04-06 01:05:53]

PTAL

https://codereview.chromium.org/2791903003/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2791903003/diff/20001/ash/shell.cc#newcode802
ash/shell.cc:802: make_scoped_refptr(new PrefRegistrySimple()), {},
On 2017/04/05 10:04:32, Bernhard (slow until Apr 6) wrote:
> On 2017/04/04 04:05:59, tibell wrote:
> > On 2017/04/03 10:37:39, Bernhard (slow until Apr 6) wrote:
> > > Nit: Add a comment /* already_connected_types = */ before the argument?
> > > Otherwise it's a bit hard to tell at a glance what that parameter is for.
> > 
> > I instead changed it to spell out the type, since {} doesn't work on all of
> our
> > compilers anyway. :/
> 
> :-/ An alternative might be to add a helper method named something like
> NoPrefStoreTypes() that will return an empty vector.

I'm going to stick with what I have. This code is currently commented out in
HEAD due a ash vs Chrome startup ordering problem, otherwise I'd add a /* */
comment here, but you can't nest those. I will try to remember adding such a
comment back when we re-enable this code.

https://codereview.chromium.org/2791903003/diff/20001/services/preferences/pr...
File services/preferences/pref_store_manager_impl.cc (right):

https://codereview.chromium.org/2791903003/diff/20001/services/preferences/pr...
services/preferences/pref_store_manager_impl.cc:118:
expected_pref_stores_.insert(PrefValueStore::DEFAULT_STORE);
On 2017/04/05 10:04:32, Bernhard (slow until Apr 6) wrote:
> On 2017/04/04 04:05:59, tibell wrote:
> > On 2017/04/03 10:37:40, Bernhard (slow until Apr 6) wrote:
> > > Remove this now that we explicitly pass in DEFAULT_STORE here? Or maybe
> > replace
> > > it with a DCHECK that DEFAULT_STORE is included?
> > 
> > Done.
> 
> Not really? :)
> 
> Could you also add a comment that explains why we modify the set of expected
> pref stores?

I changed my mind after replying to this comment in the last round and now I've
changed my mind again. Here's what I've ended up with:

The user and default stores are required by the service itself, since it needs
somewhere to write prefs and all prefs are required to be registered together
with their defaults. The question is just how explicit the user of
PrefStoreManagerImpl has to be. Here are the options when constructing the
object:

1. The user must mention the user/default stores explicitly. Pros: we make sure
the users understands what they are getting by explicitly stating it.
2. The user must not mention the user/default stores explicitly. Pros: the user
doesn't have to provide redundant information.
3. The user can chose. Pros: it "just works".

I think all are OK choices (the end result is the same), but I've opted for (1)
in the name of explicitness. I've added a DCHECK in the PrefStoreManagerImpl
constructor to enforce this.

I've also updated the docs in the .h file and the corresponding test to reflect
this choice.

P.S. I also explicitly require connecting users to not list the user/default
stores in the |already_connected_types| because all users, including Chrome,
should access these through the service instead of some in-process backdoor.

https://codereview.chromium.org/2791903003/diff/100001/services/preferences/p...
File services/preferences/pref_store_manager_impl.cc (right):

https://codereview.chromium.org/2791903003/diff/100001/services/preferences/p...
services/preferences/pref_store_manager_impl.cc:253:
std::find(already_connected_types.begin(),
On 2017/04/05 10:04:32, Bernhard (slow until Apr 6) wrote:
> Nit: base::ContainsValue() (from base/stl_util.h) does this, and looks
slightly
> more concise.

Done.

[commit-bot: I haz the power, 2017-04-06 01:06:37]

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-04-06 01:33:33]

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

[commit-bot: I haz the power, 2017-04-06 01:33:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-04-06 01:47:00]

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

[commit-bot: I haz the power, 2017-04-06 01:47:32]

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-04-06 03:08:43]

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

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

Dry run: This issue passed the CQ dry run.

[tibell, 2017-04-06 05:02:16]

On 2017/04/06 01:05:53, tibell wrote:
> PTAL
> 
> https://codereview.chromium.org/2791903003/diff/20001/ash/shell.cc
> File ash/shell.cc (right):
> 
> https://codereview.chromium.org/2791903003/diff/20001/ash/shell.cc#newcode802
> ash/shell.cc:802: make_scoped_refptr(new PrefRegistrySimple()), {},
> On 2017/04/05 10:04:32, Bernhard (slow until Apr 6) wrote:
> > On 2017/04/04 04:05:59, tibell wrote:
> > > On 2017/04/03 10:37:39, Bernhard (slow until Apr 6) wrote:
> > > > Nit: Add a comment /* already_connected_types = */ before the argument?
> > > > Otherwise it's a bit hard to tell at a glance what that parameter is
for.
> > > 
> > > I instead changed it to spell out the type, since {} doesn't work on all
of
> > our
> > > compilers anyway. :/
> > 
> > :-/ An alternative might be to add a helper method named something like
> > NoPrefStoreTypes() that will return an empty vector.
> 
> I'm going to stick with what I have. This code is currently commented out in
> HEAD due a ash vs Chrome startup ordering problem, otherwise I'd add a /* */
> comment here, but you can't nest those. I will try to remember adding such a
> comment back when we re-enable this code.
> 
>
https://codereview.chromium.org/2791903003/diff/20001/services/preferences/pr...
> File services/preferences/pref_store_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/2791903003/diff/20001/services/preferences/pr...
> services/preferences/pref_store_manager_impl.cc:118:
> expected_pref_stores_.insert(PrefValueStore::DEFAULT_STORE);
> On 2017/04/05 10:04:32, Bernhard (slow until Apr 6) wrote:
> > On 2017/04/04 04:05:59, tibell wrote:
> > > On 2017/04/03 10:37:40, Bernhard (slow until Apr 6) wrote:
> > > > Remove this now that we explicitly pass in DEFAULT_STORE here? Or maybe
> > > replace
> > > > it with a DCHECK that DEFAULT_STORE is included?
> > > 
> > > Done.
> > 
> > Not really? :)
> > 
> > Could you also add a comment that explains why we modify the set of expected
> > pref stores?
> 
> I changed my mind after replying to this comment in the last round and now
I've
> changed my mind again. Here's what I've ended up with:
> 
> The user and default stores are required by the service itself, since it needs
> somewhere to write prefs and all prefs are required to be registered together
> with their defaults. The question is just how explicit the user of
> PrefStoreManagerImpl has to be. Here are the options when constructing the
> object:
> 
> 1. The user must mention the user/default stores explicitly. Pros: we make
sure
> the users understands what they are getting by explicitly stating it.
> 2. The user must not mention the user/default stores explicitly. Pros: the
user
> doesn't have to provide redundant information.
> 3. The user can chose. Pros: it "just works".
> 
> I think all are OK choices (the end result is the same), but I've opted for
(1)
> in the name of explicitness. I've added a DCHECK in the PrefStoreManagerImpl
> constructor to enforce this.
> 
> I've also updated the docs in the .h file and the corresponding test to
reflect
> this choice.
> 
> P.S. I also explicitly require connecting users to not list the user/default
> stores in the |already_connected_types| because all users, including Chrome,
> should access these through the service instead of some in-process backdoor.
> 
>
https://codereview.chromium.org/2791903003/diff/100001/services/preferences/p...
> File services/preferences/pref_store_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/2791903003/diff/100001/services/preferences/p...
> services/preferences/pref_store_manager_impl.cc:253:
> std::find(already_connected_types.begin(),
> On 2017/04/05 10:04:32, Bernhard (slow until Apr 6) wrote:
> > Nit: base::ContainsValue() (from base/stl_util.h) does this, and looks
> slightly
> > more concise.
> 
> Done.

I missed that you already lgtmed it. I made the changes I think you wanted so
I'm submitting this.

[tibell, 2017-04-06 05:02:23]

The CQ bit was checked by tibell@chromium.org

[tibell, 2017-04-06 05:02:23]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org,
mbarbella@chromium.org, sky@chromium.org, bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2791903003/#ps140001
(title: "ProfilePrefStoreManagerTest: explicit list stores to connect to")

[commit-bot: I haz the power, 2017-04-06 05:02:40]

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

[tibell, 2017-04-06 05:22:40]

The CQ bit was unchecked by tibell@chromium.org

[tibell, 2017-04-06 05:22:50]

The CQ bit was checked by tibell@chromium.org

[commit-bot: I haz the power, 2017-04-06 05:23:07]

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-04-06 05:31:20]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491456170234860,
"parent_rev": "4b7c48089ce5ee9d1a2e4530a82f6bd31e398bbd", "commit_rev":
"2961ff4f598924f8132e9fc1171009cc0ee0b7d6"}

[commit-bot: I haz the power, 2017-04-06 05:32:30]

Description was changed from

==========
Pref service: have Mash and Chrome connect to the right pref stores

Chrome contains the implementation of all pref stores, except the
default pref store, in-process and thus only needs to connect to
one store. Mash needs to connect to all pref stores.

This is achieved by having clients specify which pref stores they
already know of in-process, rather than the other way
around. That way e.g. mash doesn't need to know which pref stores
Chrome exposes to it.

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

to

==========
Pref service: have Mash and Chrome connect to the right pref stores

Chrome contains the implementation of all pref stores, except the
default pref store, in-process and thus only needs to connect to
one store. Mash needs to connect to all pref stores.

This is achieved by having clients specify which pref stores they
already know of in-process, rather than the other way
around. That way e.g. mash doesn't need to know which pref stores
Chrome exposes to it.

BUG=654988

Review-Url: https://codereview.chromium.org/2791903003
Cr-Commit-Position: refs/heads/master@{#462364}
Committed:
https://chromium.googlesource.com/chromium/src/+/2961ff4f598924f8132e9fc11710...
==========

[commit-bot: I haz the power, 2017-04-06 05:32:32]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2961ff4f598924f8132e9fc11710...