mash: switch to the new pref service

mash: use the new prefs service

Adds ActiveProfilePrefService to allow mash, which currently
supports only one profile and runs its services as the root user,
to talk to that profile. Replaces PreferencesConnectionManager,
which achieves the equivalent goal in the old pref service.

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

[tibell, 2017-03-23 06:45:53]

Connecting to the service now works

[tibell, 2017-03-23 06:45:55]

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

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

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-23 07:01:42]

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

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

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

[tibell, 2017-03-27 03:02:59]

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

[commit-bot: I haz the power, 2017-03-27 03:03: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-27 03:38:34]

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

[tibell, 2017-03-27 03:38:35]

Please take a first pass.

[tibell, 2017-03-27 03:48:08]

Description was changed from

==========
mash: use the new prefs service

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

to

==========
mash: use the new prefs service

Adds ActiveProfilePrefService to allow mash, which currently supports only one
profile and runs its services as the root user, to talk to that profile.
Replaces PreferencesConnectionManager, which achieves the equivalent goal in the
old pref service.

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

[tibell, 2017-03-27 03:48:31]

Description was changed from

==========
mash: use the new prefs service

Adds ActiveProfilePrefService to allow mash, which currently supports only one
profile and runs its services as the root user, to talk to that profile.
Replaces PreferencesConnectionManager, which achieves the equivalent goal in the
old pref service.

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

to

==========
mash: use the new prefs service

Adds ActiveProfilePrefService to allow mash, which currently
supports only one profile and runs its services as the root user,
to talk to that profile. Replaces PreferencesConnectionManager,
which achieves the equivalent goal in the old pref service.

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

[commit-bot: I haz the power, 2017-03-27 03:49:31]

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

[commit-bot: I haz the power, 2017-03-27 03:49:31]

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

[tibell, 2017-03-27 03:53:31]

tibell@chromium.org changed reviewers:
+ jonross@chromium.org, sky@chromium.org

[tibell, 2017-03-27 03:53:31]

jonross@chromium.org: Please review changes in 
- ash/

sky@chromium.org: Please review changes in 
- chrome/ (I'm not sure of the right place for PrefServiceEnabled())
- ash/ (jonross@, who did the previous pref service impl, can be the main
reviewer)

sammc@chromium.org: Please review changes in 
- services/preferences (in particular)
- everything else

[tibell, 2017-03-27 03:55:19]

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

[tibell, 2017-03-27 03:55:19]

mbarbella@chromium.org please look at
- mojom
- ActiveProfilePrefService - this class is needed to allow mash to talk to the
right profile, even though mash services don't run with the right service user
IDs (ideally mash will address this eventually).

[tibell, 2017-03-27 03:59:34]

Remove no longer needed tests

[tibell, 2017-03-27 03:59:36]

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

[commit-bot: I haz the power, 2017-03-27 03:59:47]

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-27 05:00:00]

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

[commit-bot: I haz the power, 2017-03-27 05:00:01]

Dry run: This issue passed the CQ dry run.

[Sam McNally, 2017-03-27 05:42:11]

https://codereview.chromium.org/2772673002/diff/60001/chrome/app/BUILD.gn
File chrome/app/BUILD.gn (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/app/BUILD.gn#new...
chrome/app/BUILD.gn:369: "//services/preferences:forwarder_manifest",
if (is_chromeos)

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:960: "prefs/active_profile_pref_service.cc",
if (is_chromeos)

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
File chrome/browser/prefs/active_profile_pref_service.h (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
chrome/browser/prefs/active_profile_pref_service.h:14: namespace prefs {
Remove.

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
chrome/browser/prefs/active_profile_pref_service.h:18: // root user, to talk to
prefs.
Add a TODO to not do this forwarding at all.

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:311: bool PrefServiceEnabled() {
DON'T ADD RANDOM STUFF HERE.

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
File chrome/common/chrome_features.h (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
chrome/common/chrome_features.h:175: bool PrefServiceEnabled();
DON'T ADD RANDOM STUFF HERE.

https://codereview.chromium.org/2772673002/diff/60001/services/preferences/fo...
File services/preferences/forwarder_manifest.json (right):

https://codereview.chromium.org/2772673002/diff/60001/services/preferences/fo...
services/preferences/forwarder_manifest.json:1: {
Move this into //chrome.

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

https://codereview.chromium.org/2772673002/diff/60001/services/preferences/pu...
services/preferences/public/cpp/pref_service_factory.h:42: const std::string&
service_name = mojom::kPrefStoreServiceName);
base::StringPiece

[jonross, 2017-03-27 14:26:38]

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc#newcode780
ash/shell.cc:780: if (wm_shell_->IsRunningInMash() &&
shell_delegate_->GetShellConnector()) {
More detailed notes given for PrefServiceEnabled(), I don't like the restriction
to mash only.

We have features being worked on which want to use the preference service both
in mash and classic ash modes.

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc#newcode784
ash/shell.cc:784: base::Bind(&Shell::OnPrefServiceInitialized,
base::Unretained(this)),
Slightly concerned about an Unretained ref.

'Should' be fine in normal running. Tests implementing a ShellConnector might
run into issues with this if they aren't pumping the MessageLoop.

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc#newcode1233
ash/shell.cc:1233: std::unique_ptr<::PrefService> pref_service) {
We could protect against the base::Unretained by exiting early on a null
instance_

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.h#newcode361
ash/shell.h:361: ::PrefService* pref_service() { return pref_service_.get(); }
Global namespace?

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.h#newcode626
ash/shell.h:626: void OnPrefServiceInitialized(std::unique_ptr<::PrefService>
pref_service);
Nit: doc that this is a callback for prefs::ConnectToPrefService

https://codereview.chromium.org/2772673002/diff/60001/chrome/app/BUILD.gn
File chrome/app/BUILD.gn (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/app/BUILD.gn#new...
chrome/app/BUILD.gn:369: "//services/preferences:forwarder_manifest",
On 2017/03/27 05:42:10, Sam McNally wrote:
> if (is_chromeos)

Nice catch. 

While we are targeting the majority of the mash work at chromeos, we have also
been testing/developing on windows. We'll probably need to visit the cros-only
restriction in the future.

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
File chrome/browser/prefs/active_profile_pref_service.cc (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
chrome/browser/prefs/active_profile_pref_service.cc:25:
&ActiveProfilePrefService::OnConnectError, base::Unretained(this)));
Shutdown can lead to the deletion of ActiveProfilePrefService while messages may
be waiting in the run loop.

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:315: switches::kMusConfig) == switches::kMash;
I'm a bit concerned about restricting this feature to mash only.

We have been using the IsRunningInMash() pattern a lot as a stopgap. To allow us
to turn off features which we haven't had the chance to implement mojoms for.
This has allowed us to test the system faster.

Once features have been ready we've had them utilize mojo apis both in mash, as
well as in classic ash settings.

There is some upcoming wallpaper and display settings changes with plans to use
the pref service in both modes.

[sky, 2017-03-27 16:00:11]

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc#newcode780
ash/shell.cc:780: if (wm_shell_->IsRunningInMash() &&
shell_delegate_->GetShellConnector()) {
How come the pref-service is specific to mash?

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
File chrome/browser/prefs/active_profile_pref_service.h (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
chrome/browser/prefs/active_profile_pref_service.h:17: // from the oot service
to the active profile. Used by mash, which runs as the
oot -> root?

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
chrome/browser/prefs/active_profile_pref_service.h:17: // from the oot service
to the active profile. Used by mash, which runs as the
If this is specific to chromeos, please move to a chromeos directory.

[tibell, 2017-03-28 00:32:59]

Address review comments

[tibell, 2017-03-28 00:33:01]

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

[commit-bot: I haz the power, 2017-03-28 00:33:45]

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-28 00:34:32]

PTAL

jonross@chromium.org: the most important thing I need from you is to know which
predicate to use (see my inline comment). I will update the CL accordingly once
I know that.

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc#newcode780
ash/shell.cc:780: if (wm_shell_->IsRunningInMash() &&
shell_delegate_->GetShellConnector()) {
On 2017/03/27 16:00:10, sky wrote:
> How come the pref-service is specific to mash?

I didn't explain this clearly enough.

The pref service itself is always running*. One instance per profile is created
by the factory registered in ProfileImpl::RegisterInProcessServices. However, we
need to deal with mash (and ash?) currently running all its services as the root
user but wanting to talk to the pref service instance running for the active
profile. The preferences_forwarder services (implemented by
ActiveProfilePrefService, using exactly the same interface as the regular pref
service) lets mash pretend it's talking to the pref service under the right user
ID.

* It's actually behind a feature flag right now, until we've validated that
performance is good enough to turn it on everywhere, but in principle it will be
always running.

See my reply to jonross@ above for more details on when which service should
run.

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc#newcode780
ash/shell.cc:780: if (wm_shell_->IsRunningInMash() &&
shell_delegate_->GetShellConnector()) {
On 2017/03/27 14:26:38, jonross wrote:
> More detailed notes given for PrefServiceEnabled(), I don't like the
restriction
> to mash only.
> 
> We have features being worked on which want to use the preference service both
> in mash and classic ash modes.

I'm happy to use whatever predicate you prefer. It just needs to be possible to
evaluate in all the cases it's used in this CL (e.g. both in chrome/ and ash/).
Let me know what the right predicate is, given these constraints:

* We don't want to run the preferences_forwarder service unless needed. In a
servicified world services which want to access profile data should be running
as the user ID of the profile. What we're doing here is a workaround until
that's true everywhere.
* The pref service is behind a feature flag for the moment, just so we can make
sure there are no performance implications for Chrome before we turn it on
everywhere.

I don't think the predicate can be <always on for Chrome OS> before we make the
above feature flag on by default, which we expect to do in April. Could --mash
implying --features=PrefService be enough until then? I don't think anyone is
actually use the PrefService instance in shell.h yet.

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc#newcode784
ash/shell.cc:784: base::Bind(&Shell::OnPrefServiceInitialized,
base::Unretained(this)),
On 2017/03/27 14:26:38, jonross wrote:
> Slightly concerned about an Unretained ref.
> 
> 'Should' be fine in normal running. Tests implementing a ShellConnector might
> run into issues with this if they aren't pumping the MessageLoop.

Acknowledged.

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc#newcode1233
ash/shell.cc:1233: std::unique_ptr<::PrefService> pref_service) {
On 2017/03/27 14:26:38, jonross wrote:
> We could protect against the base::Unretained by exiting early on a null
> instance_

Done.

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.h#newcode361
ash/shell.h:361: ::PrefService* pref_service() { return pref_service_.get(); }
On 2017/03/27 14:26:38, jonross wrote:
> Global namespace?

Done.

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.h#newcode626
ash/shell.h:626: void OnPrefServiceInitialized(std::unique_ptr<::PrefService>
pref_service);
On 2017/03/27 14:26:38, jonross wrote:
> Nit: doc that this is a callback for prefs::ConnectToPrefService

Done.

https://codereview.chromium.org/2772673002/diff/60001/chrome/app/BUILD.gn
File chrome/app/BUILD.gn (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/app/BUILD.gn#new...
chrome/app/BUILD.gn:369: "//services/preferences:forwarder_manifest",
On 2017/03/27 05:42:10, Sam McNally wrote:
> if (is_chromeos)

Done.

https://codereview.chromium.org/2772673002/diff/60001/chrome/app/BUILD.gn#new...
chrome/app/BUILD.gn:369: "//services/preferences:forwarder_manifest",
On 2017/03/27 14:26:38, jonross wrote:
> On 2017/03/27 05:42:10, Sam McNally wrote:
> > if (is_chromeos)
> 
> Nice catch. 
> 
> While we are targeting the majority of the mash work at chromeos, we have also
> been testing/developing on windows. We'll probably need to visit the cros-only
> restriction in the future.

Acknowledged.

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:960: "prefs/active_profile_pref_service.cc",
On 2017/03/27 05:42:10, Sam McNally wrote:
> if (is_chromeos)

Done.

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
File chrome/browser/prefs/active_profile_pref_service.cc (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
chrome/browser/prefs/active_profile_pref_service.cc:25:
&ActiveProfilePrefService::OnConnectError, base::Unretained(this)));
On 2017/03/27 14:26:38, jonross wrote:
> Shutdown can lead to the deletion of ActiveProfilePrefService while messages
may
> be waiting in the run loop.

sammc@ who is wiser in the world of Mojo than me guarantees that this can never
happen. Once the connector_ptr_ is delete no callbacks can never happen on it
(including error ones).

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
File chrome/browser/prefs/active_profile_pref_service.h (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
chrome/browser/prefs/active_profile_pref_service.h:14: namespace prefs {
On 2017/03/27 05:42:10, Sam McNally wrote:
> Remove.

Done.

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
chrome/browser/prefs/active_profile_pref_service.h:17: // from the oot service
to the active profile. Used by mash, which runs as the
On 2017/03/27 16:00:11, sky wrote:
> oot -> root?

Done.

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
chrome/browser/prefs/active_profile_pref_service.h:17: // from the oot service
to the active profile. Used by mash, which runs as the
On 2017/03/27 16:00:11, sky wrote:
> If this is specific to chromeos, please move to a chromeos directory.

jonross@ said in his comment in chrome/app/BUILD.gn that they experimenting with
targeting Windows as well. We need to make a decision one way or the other. I
didn't find a good place for it in chromeos/ so if it's OK with you I'll leave
it here for above said experimentation. The comment itself is mostly there to
provide some pointers to readers why this code exists to begin with.

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
chrome/browser/prefs/active_profile_pref_service.h:18: // root user, to talk to
prefs.
On 2017/03/27 05:42:10, Sam McNally wrote:
> Add a TODO to not do this forwarding at all.

Done.

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:311: bool PrefServiceEnabled() {
On 2017/03/27 05:42:10, Sam McNally wrote:
> DON'T ADD RANDOM STUFF HERE.

ditto

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:315: switches::kMusConfig) == switches::kMash;
On 2017/03/27 14:26:38, jonross wrote:
> I'm a bit concerned about restricting this feature to mash only.
> 
> We have been using the IsRunningInMash() pattern a lot as a stopgap. To allow
us
> to turn off features which we haven't had the chance to implement mojoms for.
> This has allowed us to test the system faster.
> 
> Once features have been ready we've had them utilize mojo apis both in mash,
as
> well as in classic ash settings.
> 
> There is some upcoming wallpaper and display settings changes with plans to
use
> the pref service in both modes.

See my reply in shell.cc

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
File chrome/common/chrome_features.h (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
chrome/common/chrome_features.h:175: bool PrefServiceEnabled();
On 2017/03/27 05:42:10, Sam McNally wrote:
> DON'T ADD RANDOM STUFF HERE.

As discussed offline, this is the right place (i.e. there are no other
predicates in this file so a separate block for them makes sense), assuming this
is the right file to begin with. :)

https://codereview.chromium.org/2772673002/diff/60001/services/preferences/fo...
File services/preferences/forwarder_manifest.json (right):

https://codereview.chromium.org/2772673002/diff/60001/services/preferences/fo...
services/preferences/forwarder_manifest.json:1: {
On 2017/03/27 05:42:10, Sam McNally wrote:
> Move this into //chrome.

Done.

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

https://codereview.chromium.org/2772673002/diff/60001/services/preferences/pu...
services/preferences/public/cpp/pref_service_factory.h:42: const std::string&
service_name = mojom::kPrefStoreServiceName);
On 2017/03/27 05:42:11, Sam McNally wrote:
> base::StringPiece

Done.

[commit-bot: I haz the power, 2017-03-28 01:51:23]

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

[commit-bot: I haz the power, 2017-03-28 01:51:24]

Dry run: This issue passed the CQ dry run.

[sky, 2017-03-28 03:45:20]

LGTM

[jonross, 2017-03-28 13:57:16]

On 2017/03/28 03:45:20, sky wrote:
> LGTM

I replied to tibell@ comments discussing how to predicate the usage of this
feature. While there appears to be a timeline difference any potential changes
to the predicate can be addressed in a follow up.

This change LGTM

[jonross, 2017-03-28 13:57:26]

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc#newcode780
ash/shell.cc:780: if (wm_shell_->IsRunningInMash() &&
shell_delegate_->GetShellConnector()) {
On 2017/03/28 00:34:31, tibell wrote:
> On 2017/03/27 16:00:10, sky wrote:
> > How come the pref-service is specific to mash?
> 
> I didn't explain this clearly enough.
> 
> The pref service itself is always running*. One instance per profile is
created
> by the factory registered in ProfileImpl::RegisterInProcessServices. However,
we
> need to deal with mash (and ash?) currently running all its services as the
root
> user but wanting to talk to the pref service instance running for the active
> profile. The preferences_forwarder services (implemented by
> ActiveProfilePrefService, using exactly the same interface as the regular pref
> service) lets mash pretend it's talking to the pref service under the right
user
> ID.
> 
> * It's actually behind a feature flag right now, until we've validated that
> performance is good enough to turn it on everywhere, but in principle it will
be
> always running.
> 
> See my reply to jonross@ above for more details on when which service should
> run.

I hadn't realized the concerns around performance. Nor the plan to enable the
flag by default for April.

The Ash WallpaperController was actually beginning to hook up to the version
being replaced by this CL, targeting M59. Which would be ahead of the goal of
end of April, and would be predicate on the service always being available for
Chrome OS.

However if that is not feasible they could look at an alternate solution for
M59.

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
File chrome/browser/prefs/active_profile_pref_service.cc (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/browser/prefs/ac...
chrome/browser/prefs/active_profile_pref_service.cc:25:
&ActiveProfilePrefService::OnConnectError, base::Unretained(this)));
On 2017/03/28 00:34:31, tibell wrote:
> On 2017/03/27 14:26:38, jonross wrote:
> > Shutdown can lead to the deletion of ActiveProfilePrefService while messages
> may
> > be waiting in the run loop.
> 
> sammc@ who is wiser in the world of Mojo than me guarantees that this can
never
> happen. Once the connector_ptr_ is delete no callbacks can never happen on it
> (including error ones).

Acknowledged.

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/2772673002/diff/60001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:315: switches::kMusConfig) == switches::kMash;
On 2017/03/28 00:34:31, tibell wrote:
> On 2017/03/27 14:26:38, jonross wrote:
> > I'm a bit concerned about restricting this feature to mash only.
> > 
> > We have been using the IsRunningInMash() pattern a lot as a stopgap. To
allow
> us
> > to turn off features which we haven't had the chance to implement mojoms
for.
> > This has allowed us to test the system faster.
> > 
> > Once features have been ready we've had them utilize mojo apis both in mash,
> as
> > well as in classic ash settings.
> > 
> > There is some upcoming wallpaper and display settings changes with plans to
> use
> > the pref service in both modes.
> 
> See my reply in shell.cc

Acknowledged.

[Martin Barbella, 2017-03-28 17:14:16]

mojom lgtm

[Sam McNally, 2017-03-28 23:09:52]

lgtm

[tibell, 2017-03-29 00:11:14]

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

[commit-bot: I haz the power, 2017-03-29 00:12:17]

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-29 00:34:05]

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

[commit-bot: I haz the power, 2017-03-29 00:34:06]

Dry run: This issue passed the CQ dry run.

[tibell, 2017-03-29 02:59:15]

Going ahead and submitting as soon as the dependent CLs land.

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2772673002/diff/60001/ash/shell.cc#newcode780
ash/shell.cc:780: if (wm_shell_->IsRunningInMash() &&
shell_delegate_->GetShellConnector()) {
On 2017/03/28 13:57:26, jonross wrote:
> On 2017/03/28 00:34:31, tibell wrote:
> > On 2017/03/27 16:00:10, sky wrote:
> > > How come the pref-service is specific to mash?
> > 
> > I didn't explain this clearly enough.
> > 
> > The pref service itself is always running*. One instance per profile is
> created
> > by the factory registered in ProfileImpl::RegisterInProcessServices.
However,
> we
> > need to deal with mash (and ash?) currently running all its services as the
> root
> > user but wanting to talk to the pref service instance running for the active
> > profile. The preferences_forwarder services (implemented by
> > ActiveProfilePrefService, using exactly the same interface as the regular
pref
> > service) lets mash pretend it's talking to the pref service under the right
> user
> > ID.
> > 
> > * It's actually behind a feature flag right now, until we've validated that
> > performance is good enough to turn it on everywhere, but in principle it
will
> be
> > always running.
> > 
> > See my reply to jonross@ above for more details on when which service should
> > run.
> 
> I hadn't realized the concerns around performance. Nor the plan to enable the
> flag by default for April.
> 
> The Ash WallpaperController was actually beginning to hook up to the version
> being replaced by this CL, targeting M59. Which would be ahead of the goal of
> end of April, and would be predicate on the service always being available for
> Chrome OS.
> 
> However if that is not feasible they could look at an alternate solution for
> M59.
> 

For the record, jonross@ and I discussed this offline. We'll go ahead and submit
this and try to get it ready in April, but even if we don't the alternative
solution mentioned above is OK.

[tibell, 2017-03-29 04:18:37]

The CQ bit was checked by tibell@chromium.org

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

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-29 04:27:45]

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

[commit-bot: I haz the power, 2017-03-29 04:27:48]

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

[tibell, 2017-03-29 04:44:47]

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

[tibell, 2017-03-29 04:44:48]

Hi bauerb@chromium.org!

I missed that I need approval for DEPS on components/pref.

[Bernhard Bauer, 2017-03-29 08:47:44]

DEPS change LGTM.

[tibell, 2017-03-29 22:21:46]

The CQ bit was checked by tibell@chromium.org

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

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-30 00:45:14]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490826106196790,
"parent_rev": "0b5f234c63a2e2c628d400290b343da70df12d2b", "commit_rev":
"2ffd3909f8a5b5772b20505b0eaf401c4f480199"}

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

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490826106196790,
"parent_rev": "abd1360936725f296381ceb3c194307a29137c53", "commit_rev":
"b93c729b78b6b59a5953215c2fbe6fcb551717e1"}

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

Description was changed from

==========
mash: use the new prefs service

Adds ActiveProfilePrefService to allow mash, which currently
supports only one profile and runs its services as the root user,
to talk to that profile. Replaces PreferencesConnectionManager,
which achieves the equivalent goal in the old pref service.

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

to

==========
mash: use the new prefs service

Adds ActiveProfilePrefService to allow mash, which currently
supports only one profile and runs its services as the root user,
to talk to that profile. Replaces PreferencesConnectionManager,
which achieves the equivalent goal in the old pref service.

BUG=654988

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

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

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b93c729b78b6b59a5953215c2fbe...