Pref service: add a factory to create the client lib

Pref service: add a factory to create the client lib

The client lib is how most code will interact with the service,
rather than using its Mojo interfaces directly.

ConnectToPrefService: This is the main entry point for users.

ServiceTest: Change the destruction order (of the message loop)
to match the creation order.

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

[tibell, 2017-03-22 03:03:24]

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

[commit-bot: I haz the power, 2017-03-22 03:04:31]

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-22 03:17:39]

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

[commit-bot: I haz the power, 2017-03-22 03:17:40]

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

[tibell, 2017-03-22 03:25:27]

Don't build tests on iOS

[tibell, 2017-03-22 03:25:30]

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

[commit-bot: I haz the power, 2017-03-22 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-22 03:31:16]

Description was changed from

==========
Pref service: add a factory to create the client lib

The client lib is how most code will interact with the service, rather than
using its Mojo interfaces directly.

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

to

==========
Pref service: add a factory to create the client lib

The client lib is how most code will interact with the service,
rather than using its Mojo interfaces directly.

CreatePrefService: This is the main entry point for users.

ServiceTest: Change the destruction order (of the message loop)
to match the creation order.

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

[tibell, 2017-03-22 03:34:48]

Patchset #2 (id:20001) has been deleted

[tibell, 2017-03-22 03:35:37]

Don't build tests on iOS

[tibell, 2017-03-22 03:35:40]

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

[commit-bot: I haz the power, 2017-03-22 03:35:50]

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-22 04:02:48]

Make tests compile on Windows

[tibell, 2017-03-22 04:02:51]

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

[commit-bot: I haz the power, 2017-03-22 04:03:19]

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

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

[Sam McNally, 2017-03-22 04:34:03]

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/BU...
File services/preferences/BUILD.gn (right):

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/BU...
services/preferences/BUILD.gn:59: catalog("tests_catalog") {
testonly = true

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
File services/preferences/pref_service_factory_unittest.cc (right):

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
services/preferences/pref_service_factory_unittest.cc:31: explicit
ServiceTestClient(
Remove explicit.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
services/preferences/pref_service_factory_unittest.cc:105: void
WillDestroyCurrentMessageLoop() override { worker_pool_owner_.reset(); }
:(

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
services/preferences/pref_service_factory_unittest.cc:108: static void
OnInit(const base::Closure& quit_closure, bool success) {
private?

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
services/preferences/pref_service_factory_unittest.cc:113: static void
OnCreate(const base::Closure& quit_closure,
private?

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
services/preferences/pref_service_factory_unittest.cc:131:
base::Unretained(&pref_service)));
Is the Unretained() necessary?

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
services/preferences/pref_service_factory_unittest.cc:136: static void
OnPrefChanged(const base::Closure& quit_closure,
private?

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
File services/preferences/public/cpp/BUILD.gn (right):

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
services/preferences/public/cpp/BUILD.gn:11: "pref_service_factory.cc",
.h?

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
File services/preferences/public/cpp/pref_service_factory.cc (right):

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
services/preferences/public/cpp/pref_service_factory.cc:53: // |unused| exists
so that the InterfacePtr won't go out of scope and close
They aren't unused anymore.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
services/preferences/public/cpp/pref_service_factory.cc:86:
connections.count(PrefValueStore::MANAGED_STORE)
Could each of these be a function call instead? This is a bit hard to read.
Also, don't do a count and a find.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
File services/preferences/public/cpp/pref_service_factory.h (right):

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
services/preferences/public/cpp/pref_service_factory.h:37: void
CreatePrefService(service_manager::Connector* connector,
This clashes with the CreatePrefService that creates the service side.

[tibell, 2017-03-22 05:29:30]

Address sammc's review comments

[tibell, 2017-03-22 05:29:32]

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

[tibell, 2017-03-22 05:29:39]

Patchset #4 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-03-22 05:29:50]

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-22 05:31:47]

Address sammc's review comments

[tibell, 2017-03-22 05:31:50]

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

[tibell, 2017-03-22 05:32:04]

PTAL

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/BU...
File services/preferences/BUILD.gn (right):

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/BU...
services/preferences/BUILD.gn:59: catalog("tests_catalog") {
On 2017/03/22 04:34:03, Sam McNally wrote:
> testonly = true

Done.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
File services/preferences/pref_service_factory_unittest.cc (right):

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
services/preferences/pref_service_factory_unittest.cc:31: explicit
ServiceTestClient(
On 2017/03/22 04:34:03, Sam McNally wrote:
> Remove explicit.

Done.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
services/preferences/pref_service_factory_unittest.cc:105: void
WillDestroyCurrentMessageLoop() override { worker_pool_owner_.reset(); }
On 2017/03/22 04:34:03, Sam McNally wrote:
> :(

Acknowledged.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
services/preferences/pref_service_factory_unittest.cc:108: static void
OnInit(const base::Closure& quit_closure, bool success) {
On 2017/03/22 04:34:03, Sam McNally wrote:
> private?

Done.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
services/preferences/pref_service_factory_unittest.cc:113: static void
OnCreate(const base::Closure& quit_closure,
On 2017/03/22 04:34:03, Sam McNally wrote:
> private?

Done.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
services/preferences/pref_service_factory_unittest.cc:131:
base::Unretained(&pref_service)));
On 2017/03/22 04:34:03, Sam McNally wrote:
> Is the Unretained() necessary?

Done.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pr...
services/preferences/pref_service_factory_unittest.cc:136: static void
OnPrefChanged(const base::Closure& quit_closure,
On 2017/03/22 04:34:03, Sam McNally wrote:
> private?

Done.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
File services/preferences/public/cpp/BUILD.gn (right):

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
services/preferences/public/cpp/BUILD.gn:11: "pref_service_factory.cc",
On 2017/03/22 04:34:03, Sam McNally wrote:
> .h?

Done.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
File services/preferences/public/cpp/pref_service_factory.cc (right):

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
services/preferences/public/cpp/pref_service_factory.cc:53: // |unused| exists
so that the InterfacePtr won't go out of scope and close
On 2017/03/22 04:34:03, Sam McNally wrote:
> They aren't unused anymore.

Done.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
services/preferences/public/cpp/pref_service_factory.cc:86:
connections.count(PrefValueStore::MANAGED_STORE)
On 2017/03/22 04:34:03, Sam McNally wrote:
> Could each of these be a function call instead? This is a bit hard to read.
> Also, don't do a count and a find.

Done.

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
File services/preferences/public/cpp/pref_service_factory.h (right):

https://codereview.chromium.org/2762333003/diff/60001/services/preferences/pu...
services/preferences/public/cpp/pref_service_factory.h:37: void
CreatePrefService(service_manager::Connector* connector,
On 2017/03/22 04:34:03, Sam McNally wrote:
> This clashes with the CreatePrefService that creates the service side.

Done.

[commit-bot: I haz the power, 2017-03-22 05:32:43]

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-22 05:33:28]

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

[tibell, 2017-03-22 05:33:29]

rockot@chromium.org: Please review changes in
- services/BUILD.gn
- services/service_manager

jam@chromium.org: Please review changes in 
- BUILD.gn

[tibell, 2017-03-22 05:36:12]

Description was changed from

==========
Pref service: add a factory to create the client lib

The client lib is how most code will interact with the service,
rather than using its Mojo interfaces directly.

CreatePrefService: This is the main entry point for users.

ServiceTest: Change the destruction order (of the message loop)
to match the creation order.

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

to

==========
Pref service: add a factory to create the client lib

The client lib is how most code will interact with the service,
rather than using its Mojo interfaces directly.

ConnectToPrefService: This is the main entry point for users.

ServiceTest: Change the destruction order (of the message loop)
to match the creation order.

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

[tibell, 2017-03-22 05:38:34]

Rename CreateCallback to match function it belongs to

[tibell, 2017-03-22 05:38:37]

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

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

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-22 05:43:04]

lgtm

https://codereview.chromium.org/2762333003/diff/120001/services/preferences/p...
File services/preferences/public/cpp/pref_service_factory.cc (right):

https://codereview.chromium.org/2762333003/diff/120001/services/preferences/p...
services/preferences/public/cpp/pref_service_factory.cc:72:
scoped_refptr<PrefStore> ConnectionBarrier::CreatePrefStore(
Can this be static or (better yet) a non-member function?

[tibell, 2017-03-22 05:54:12]

Address more review comments from sammc@

[tibell, 2017-03-22 05:54:14]

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

[commit-bot: I haz the power, 2017-03-22 05:54:36]

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), 2017-03-22 06:21:01]

Said files LGTM.

BTW I think for toplevel BUILD.gn per //OWNERS you want someone from
//build/OWNERS, not //OWNERS.

https://codereview.chromium.org/2762333003/diff/140001/services/preferences/p...
File services/preferences/public/cpp/pref_service_factory.h (right):

https://codereview.chromium.org/2762333003/diff/140001/services/preferences/p...
services/preferences/public/cpp/pref_service_factory.h:11: #ifndef
SERVICES_PREFERENCES_PUBLIC_CPP_PREF_SERVICE_FACTORY_H_
I have to say it's kind of a bummer that the pref service's client library has a
"pref service factory". I realize why this is true (different meaning of
"PrefService"), and I'm not asking you to change it because it's unfortunately
correct, but... ew. Just ew.

[tibell, 2017-03-22 06:26:12]

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

[tibell, 2017-03-22 06:26:13]

scottmg@chromium.org: Please review top-level BUILD.gn

https://codereview.chromium.org/2762333003/diff/140001/services/preferences/p...
File services/preferences/public/cpp/pref_service_factory.h (right):

https://codereview.chromium.org/2762333003/diff/140001/services/preferences/p...
services/preferences/public/cpp/pref_service_factory.h:11: #ifndef
SERVICES_PREFERENCES_PUBLIC_CPP_PREF_SERVICE_FACTORY_H_
On 2017/03/22 06:21:01, Ken Rockot wrote:
> I have to say it's kind of a bummer that the pref service's client library has
a
> "pref service factory". I realize why this is true (different meaning of
> "PrefService"), and I'm not asking you to change it because it's unfortunately
> correct, but... ew. Just ew.

I know. I know. :) When this service has taken over the world I will still be
thinking of a better way of naming this. All I can say is that will give it a
better name if I ever come up with one. :)

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

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

[commit-bot: I haz the power, 2017-03-22 07:41:27]

Dry run: This issue passed the CQ dry run.

[scottmg, 2017-03-22 14:45:14]

BUILD.gn lgtm

[tibell, 2017-03-22 22:36:39]

The CQ bit was checked by tibell@chromium.org

[tibell, 2017-03-22 22:36:39]

The patchset sent to the CQ was uploaded after l-g-t-m from sammc@chromium.org
Link to the patchset: https://codereview.chromium.org/2762333003/#ps140001
(title: "Address more review comments from sammc@")

[commit-bot: I haz the power, 2017-03-22 22:37:14]

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-22 22:53:00]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490222199213480,
"parent_rev": "efb50e0f9602868db59d926245e27246988fb210", "commit_rev":
"d993a77b16b1f937bd09cb10399a9c7bea0ad46a"}

[commit-bot: I haz the power, 2017-03-22 22:53:47]

Description was changed from

==========
Pref service: add a factory to create the client lib

The client lib is how most code will interact with the service,
rather than using its Mojo interfaces directly.

ConnectToPrefService: This is the main entry point for users.

ServiceTest: Change the destruction order (of the message loop)
to match the creation order.

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

to

==========
Pref service: add a factory to create the client lib

The client lib is how most code will interact with the service,
rather than using its Mojo interfaces directly.

ConnectToPrefService: This is the main entry point for users.

ServiceTest: Change the destruction order (of the message loop)
to match the creation order.

BUG=654988

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

[commit-bot: I haz the power, 2017-03-22 22:53:48]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/d993a77b16b1f937bd09cb10399a...