Make PrefService available in *ash

Make PrefService available in *ash

This CL adds a temporary solution to make the active user's pref
service available in *ash.

BUG=713934
Review-Url: https://codereview.chromium.org/2827193004
Cr-Commit-Position: refs/heads/master@{#467576}
Committed: https://chromium.googlesource.com/chromium/src/+/15eb928d886a49c90111f25af454be9df4fba3a7

[afakhry, 2017-04-21 16:51:12]

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

[commit-bot: I haz the power, 2017-04-21 16:51:34]

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-21 17:55:59]

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

[commit-bot: I haz the power, 2017-04-21 17:56:00]

Dry run: This issue passed the CQ dry run.

[sky, 2017-04-21 20:37:32]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2017-04-21 20:37:32]

Two thoughts here:

1. I'm assuming you want this as Shell is going to start relying on a pref
service, right? What happens if ash can't connect to Chrome (this will only
happen when ash is run in Config::MASH and Chrome isn't built. This is only for
testing. I believe in this case PrefService is null.

2. My understanding is you need this for classic ash case. For that case chrome
can reach into ash directly. Can it set the prefservice directly? We'll need
this for mushrome case too (Config::MUS).

[afakhry, 2017-04-24 16:20:16]

On 2017/04/21 20:37:32, sky wrote:
> Two thoughts here:
> 
> 1. I'm assuming you want this as Shell is going to start relying on a pref
> service, right? What happens if ash can't connect to Chrome (this will only
> happen when ash is run in Config::MASH and Chrome isn't built. This is only
for
> testing. I believe in this case PrefService is null.
 
I think this is expected. The method I added to Shell,
GetActiveUserPrefService(), is expected to return null if there's no active
user, and also the case you mentioned above, if ash can't connect to Chrome in
MASH. So in this case, a simple null check should be done.

> 2. My understanding is you need this for classic ash case. For that case
chrome
> can reach into ash directly. Can it set the prefservice directly? We'll need
> this for mushrome case too (Config::MUS).

I believe a getter through the ShellDelegate makes more sense in this case,
since we want the *active* user's prefservice. This is much simpler than having
Chrome inject the prefservice every time the active user changes. WDYT?

[sky, 2017-04-24 19:54:21]

On 2017/04/24 16:20:16, afakhry wrote:
> On 2017/04/21 20:37:32, sky wrote:
> > Two thoughts here:
> > 
> > 1. I'm assuming you want this as Shell is going to start relying on a pref
> > service, right? What happens if ash can't connect to Chrome (this will only
> > happen when ash is run in Config::MASH and Chrome isn't built. This is only
> for
> > testing. I believe in this case PrefService is null.
>  
> I think this is expected. The method I added to Shell,
> GetActiveUserPrefService(), is expected to return null if there's no active
> user, and also the case you mentioned above, if ash can't connect to Chrome in
> MASH. So in this case, a simple null check should be done.

This is ok as long as the code isn't going to rely on a non-null prefservice. As
long as a null prefservice is ok, that's fine.

> > 2. My understanding is you need this for classic ash case. For that case
> chrome
> > can reach into ash directly. Can it set the prefservice directly? We'll need
> > this for mushrome case too (Config::MUS).
> 
> I believe a getter through the ShellDelegate makes more sense in this case,
> since we want the *active* user's prefservice. This is much simpler than
having
> Chrome inject the prefservice every time the active user changes. WDYT?

Currently the pref service itself doesn't change, rather the prefservice that is
returned to mash redirects to the appropriate prefer service. This code is in
ActiveProfilePrefService.

[afakhry, 2017-04-25 00:40:07]

On 2017/04/24 19:54:21, sky wrote:
> On 2017/04/24 16:20:16, afakhry wrote:
> > On 2017/04/21 20:37:32, sky wrote:
> > > Two thoughts here:
> > > 
> > > 1. I'm assuming you want this as Shell is going to start relying on a pref
> > > service, right? What happens if ash can't connect to Chrome (this will
only
> > > happen when ash is run in Config::MASH and Chrome isn't built. This is
only
> > for
> > > testing. I believe in this case PrefService is null.
> >  
> > I think this is expected. The method I added to Shell,
> > GetActiveUserPrefService(), is expected to return null if there's no active
> > user, and also the case you mentioned above, if ash can't connect to Chrome
in
> > MASH. So in this case, a simple null check should be done.
> 
> This is ok as long as the code isn't going to rely on a non-null prefservice.
As
> long as a null prefservice is ok, that's fine.
> 
> > > 2. My understanding is you need this for classic ash case. For that case
> > chrome
> > > can reach into ash directly. Can it set the prefservice directly? We'll
need
> > > this for mushrome case too (Config::MUS).
> > 
> > I believe a getter through the ShellDelegate makes more sense in this case,
> > since we want the *active* user's prefservice. This is much simpler than
> having
> > Chrome inject the prefservice every time the active user changes. WDYT?
> 
> Currently the pref service itself doesn't change, rather the prefservice that
is
> returned to mash redirects to the appropriate prefer service. This code is in
> ActiveProfilePrefService.

In the case of mash, yes, but what about the non-mash case? Shouldn't we have a
working alternative in classic ash too?

[sky, 2017-04-25 17:11:24]

Wouldn't classic ash use the same thing?

  -Scott

On Mon, Apr 24, 2017 at 5:40 PM, <afakhry@chromium.org> wrote:

> On 2017/04/24 19:54:21, sky wrote:
> > On 2017/04/24 16:20:16, afakhry wrote:
> > > On 2017/04/21 20:37:32, sky wrote:
> > > > Two thoughts here:
> > > >
> > > > 1. I'm assuming you want this as Shell is going to start relying on
> a pref
> > > > service, right? What happens if ash can't connect to Chrome (this
> will
> only
> > > > happen when ash is run in Config::MASH and Chrome isn't built. This
> is
> only
> > > for
> > > > testing. I believe in this case PrefService is null.
> > >
> > > I think this is expected. The method I added to Shell,
> > > GetActiveUserPrefService(), is expected to return null if there's no
> active
> > > user, and also the case you mentioned above, if ash can't connect to
> Chrome
> in
> > > MASH. So in this case, a simple null check should be done.
> >
> > This is ok as long as the code isn't going to rely on a non-null
> prefservice.
> As
> > long as a null prefservice is ok, that's fine.
> >
> > > > 2. My understanding is you need this for classic ash case. For that
> case
> > > chrome
> > > > can reach into ash directly. Can it set the prefservice directly?
> We'll
> need
> > > > this for mushrome case too (Config::MUS).
> > >
> > > I believe a getter through the ShellDelegate makes more sense in this
> case,
> > > since we want the *active* user's prefservice. This is much simpler
> than
> > having
> > > Chrome inject the prefservice every time the active user changes. WDYT?
> >
> > Currently the pref service itself doesn't change, rather the prefservice
> that
> is
> > returned to mash redirects to the appropriate prefer service. This code
> is in
> > ActiveProfilePrefService.
>
> In the case of mash, yes, but what about the non-mash case? Shouldn't we
> have a
> working alternative in classic ash too?
>
> https://codereview.chromium.org/2827193004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[afakhry, 2017-04-25 18:05:43]

On 2017/04/25 17:11:24, sky wrote:
> Wouldn't classic ash use the same thing?
> 
>   -Scott

Not yet. At the moment, pref_service_ in classic ash will always be nullptr. The
code that connects to the pref service is only running in Config::MASH:
https://cs.chromium.org/chromium/src/ash/shell.cc?q=%22config+%3D%3D+Config::...

I understand that long term we want to consolidate the code paths, but right
now, in classic ash, we don't have access to the active user's pref_service.
This CL is a temporary solution for that until the consolidation work is done.

> 
> On Mon, Apr 24, 2017 at 5:40 PM, <mailto:afakhry@chromium.org> wrote:
> 
> > On 2017/04/24 19:54:21, sky wrote:
> > > On 2017/04/24 16:20:16, afakhry wrote:
> > > > On 2017/04/21 20:37:32, sky wrote:
> > > > > Two thoughts here:
> > > > >
> > > > > 1. I'm assuming you want this as Shell is going to start relying on
> > a pref
> > > > > service, right? What happens if ash can't connect to Chrome (this
> > will
> > only
> > > > > happen when ash is run in Config::MASH and Chrome isn't built. This
> > is
> > only
> > > > for
> > > > > testing. I believe in this case PrefService is null.
> > > >
> > > > I think this is expected. The method I added to Shell,
> > > > GetActiveUserPrefService(), is expected to return null if there's no
> > active
> > > > user, and also the case you mentioned above, if ash can't connect to
> > Chrome
> > in
> > > > MASH. So in this case, a simple null check should be done.
> > >
> > > This is ok as long as the code isn't going to rely on a non-null
> > prefservice.
> > As
> > > long as a null prefservice is ok, that's fine.
> > >
> > > > > 2. My understanding is you need this for classic ash case. For that
> > case
> > > > chrome
> > > > > can reach into ash directly. Can it set the prefservice directly?
> > We'll
> > need
> > > > > this for mushrome case too (Config::MUS).
> > > >
> > > > I believe a getter through the ShellDelegate makes more sense in this
> > case,
> > > > since we want the *active* user's prefservice. This is much simpler
> > than
> > > having
> > > > Chrome inject the prefservice every time the active user changes. WDYT?
> > >
> > > Currently the pref service itself doesn't change, rather the prefservice
> > that
> > is
> > > returned to mash redirects to the appropriate prefer service. This code
> > is in
> > > ActiveProfilePrefService.
> >
> > In the case of mash, yes, but what about the non-mash case? Shouldn't we
> > have a
> > working alternative in classic ash too?
> >
> > https://codereview.chromium.org/2827193004/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-04-25 20:23:29]

On 2017/04/25 18:05:43, afakhry wrote:
> On 2017/04/25 17:11:24, sky wrote:
> > Wouldn't classic ash use the same thing?
> > 
> >   -Scott
> 
> Not yet. At the moment, pref_service_ in classic ash will always be nullptr.
The
> code that connects to the pref service is only running in Config::MASH:
>
https://cs.chromium.org/chromium/src/ash/shell.cc?q=%22config+%3D%3D+Config::...
> 
> I understand that long term we want to consolidate the code paths, but right
> now, in classic ash, we don't have access to the active user's pref_service.
> This CL is a temporary solution for that until the consolidation work is done.
> 
> > 
> > On Mon, Apr 24, 2017 at 5:40 PM, <mailto:afakhry@chromium.org> wrote:
> > 
> > > On 2017/04/24 19:54:21, sky wrote:
> > > > On 2017/04/24 16:20:16, afakhry wrote:
> > > > > On 2017/04/21 20:37:32, sky wrote:
> > > > > > Two thoughts here:
> > > > > >
> > > > > > 1. I'm assuming you want this as Shell is going to start relying on
> > > a pref
> > > > > > service, right? What happens if ash can't connect to Chrome (this
> > > will
> > > only
> > > > > > happen when ash is run in Config::MASH and Chrome isn't built. This
> > > is
> > > only
> > > > > for
> > > > > > testing. I believe in this case PrefService is null.
> > > > >
> > > > > I think this is expected. The method I added to Shell,
> > > > > GetActiveUserPrefService(), is expected to return null if there's no
> > > active
> > > > > user, and also the case you mentioned above, if ash can't connect to
> > > Chrome
> > > in
> > > > > MASH. So in this case, a simple null check should be done.
> > > >
> > > > This is ok as long as the code isn't going to rely on a non-null
> > > prefservice.
> > > As
> > > > long as a null prefservice is ok, that's fine.
> > > >
> > > > > > 2. My understanding is you need this for classic ash case. For that
> > > case
> > > > > chrome
> > > > > > can reach into ash directly. Can it set the prefservice directly?
> > > We'll
> > > need
> > > > > > this for mushrome case too (Config::MUS).
> > > > >
> > > > > I believe a getter through the ShellDelegate makes more sense in this
> > > case,
> > > > > since we want the *active* user's prefservice. This is much simpler
> > > than
> > > > having
> > > > > Chrome inject the prefservice every time the active user changes.
WDYT?
> > > >
> > > > Currently the pref service itself doesn't change, rather the prefservice
> > > that
> > > is
> > > > returned to mash redirects to the appropriate prefer service. This code
> > > is in
> > > > ActiveProfilePrefService.
> > >
> > > In the case of mash, yes, but what about the non-mash case? Shouldn't we
> > > have a
> > > working alternative in classic ash too?
> > >
> > > https://codereview.chromium.org/2827193004/
> > >
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.

This is only for the short term and only for classic ash, so why not use the
setter? The code can call the setter at the appropriate point, right?

[afakhry, 2017-04-25 21:40:06]

On 2017/04/25 20:23:29, sky wrote:
> On 2017/04/25 18:05:43, afakhry wrote:
> > On 2017/04/25 17:11:24, sky wrote:
> > > Wouldn't classic ash use the same thing?
> > > 
> > >   -Scott
> > 
> > Not yet. At the moment, pref_service_ in classic ash will always be nullptr.
> The
> > code that connects to the pref service is only running in Config::MASH:
> >
>
https://cs.chromium.org/chromium/src/ash/shell.cc?q=%22config+%3D%3D+Config::...
> > 
> > I understand that long term we want to consolidate the code paths, but right
> > now, in classic ash, we don't have access to the active user's pref_service.
> > This CL is a temporary solution for that until the consolidation work is
done.
> > 
> > > 
> > > On Mon, Apr 24, 2017 at 5:40 PM, <mailto:afakhry@chromium.org> wrote:
> > > 
> > > > On 2017/04/24 19:54:21, sky wrote:
> > > > > On 2017/04/24 16:20:16, afakhry wrote:
> > > > > > On 2017/04/21 20:37:32, sky wrote:
> > > > > > > Two thoughts here:
> > > > > > >
> > > > > > > 1. I'm assuming you want this as Shell is going to start relying
on
> > > > a pref
> > > > > > > service, right? What happens if ash can't connect to Chrome (this
> > > > will
> > > > only
> > > > > > > happen when ash is run in Config::MASH and Chrome isn't built.
This
> > > > is
> > > > only
> > > > > > for
> > > > > > > testing. I believe in this case PrefService is null.
> > > > > >
> > > > > > I think this is expected. The method I added to Shell,
> > > > > > GetActiveUserPrefService(), is expected to return null if there's no
> > > > active
> > > > > > user, and also the case you mentioned above, if ash can't connect to
> > > > Chrome
> > > > in
> > > > > > MASH. So in this case, a simple null check should be done.
> > > > >
> > > > > This is ok as long as the code isn't going to rely on a non-null
> > > > prefservice.
> > > > As
> > > > > long as a null prefservice is ok, that's fine.
> > > > >
> > > > > > > 2. My understanding is you need this for classic ash case. For
that
> > > > case
> > > > > > chrome
> > > > > > > can reach into ash directly. Can it set the prefservice directly?
> > > > We'll
> > > > need
> > > > > > > this for mushrome case too (Config::MUS).
> > > > > >
> > > > > > I believe a getter through the ShellDelegate makes more sense in
this
> > > > case,
> > > > > > since we want the *active* user's prefservice. This is much simpler
> > > > than
> > > > > having
> > > > > > Chrome inject the prefservice every time the active user changes.
> WDYT?
> > > > >
> > > > > Currently the pref service itself doesn't change, rather the
prefservice
> > > > that
> > > > is
> > > > > returned to mash redirects to the appropriate prefer service. This
code
> > > > is in
> > > > > ActiveProfilePrefService.
> > > >
> > > > In the case of mash, yes, but what about the non-mash case? Shouldn't we
> > > > have a
> > > > working alternative in classic ash too?
> > > >
> > > > https://codereview.chromium.org/2827193004/
> > > >
> > > 
> > > -- 
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> This is only for the short term and only for classic ash, so why not use the
> setter? The code can call the setter at the appropriate point, right?

Sorry, I think I lost you here. :)
To make sure that we're talking about the same thing, let's rewind. What 'code'
calls what 'setter'?

[sky, 2017-04-26 18:14:00]

Ahmed convinced me the approach here is better than the alternative.

https://codereview.chromium.org/2827193004/diff/1/ash/mus/shell_delegate_mus.cc
File ash/mus/shell_delegate_mus.cc (right):

https://codereview.chromium.org/2827193004/diff/1/ash/mus/shell_delegate_mus....
ash/mus/shell_delegate_mus.cc:159: NOTIMPLEMENTED();
Shouldn't this be a NOTREACHED?

https://codereview.chromium.org/2827193004/diff/1/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2827193004/diff/1/ash/shell.h#newcode428
ash/shell.h:428: PrefService* GetActiveUserPrefService() const;
Please update this description as to how to detect when to reread this.
Specifically around OnActiveUserSessionChanged.

[afakhry, 2017-04-26 18:36:46]

Thanks!

https://codereview.chromium.org/2827193004/diff/1/ash/mus/shell_delegate_mus.cc
File ash/mus/shell_delegate_mus.cc (right):

https://codereview.chromium.org/2827193004/diff/1/ash/mus/shell_delegate_mus....
ash/mus/shell_delegate_mus.cc:159: NOTIMPLEMENTED();
On 2017/04/26 18:14:00, sky wrote:
> Shouldn't this be a NOTREACHED?

Yes, right. Done.

https://codereview.chromium.org/2827193004/diff/1/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2827193004/diff/1/ash/shell.h#newcode428
ash/shell.h:428: PrefService* GetActiveUserPrefService() const;
On 2017/04/26 18:14:00, sky wrote:
> Please update this description as to how to detect when to reread this.
> Specifically around OnActiveUserSessionChanged.

Done.

[sky, 2017-04-26 19:51:57]

LGTM

https://codereview.chromium.org/2827193004/diff/20001/ash/mus/shell_delegate_...
File ash/mus/shell_delegate_mus.cc (right):

https://codereview.chromium.org/2827193004/diff/20001/ash/mus/shell_delegate_...
ash/mus/shell_delegate_mus.cc:159: NOTREACHED();
Please add a comment as to why the NOTREACHED as it isn't obvious from looking
at this code why it's here.

[afakhry, 2017-04-26 22:43:51]

https://codereview.chromium.org/2827193004/diff/20001/ash/mus/shell_delegate_...
File ash/mus/shell_delegate_mus.cc (right):

https://codereview.chromium.org/2827193004/diff/20001/ash/mus/shell_delegate_...
ash/mus/shell_delegate_mus.cc:159: NOTREACHED();
On 2017/04/26 19:51:57, sky wrote:
> Please add a comment as to why the NOTREACHED as it isn't obvious from looking
> at this code why it's here.

Done.

[afakhry, 2017-04-27 00:22:03]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2017-04-27 00:22:04]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2827193004/#ps40001
(title: "Adding NOTREACHED() comment.")

[commit-bot: I haz the power, 2017-04-27 00:22:57]

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-27 04:03:47]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493252523616550,
"parent_rev": "bb4bab6e5cfcaf5b8b8511fa6986d571f23cc034", "commit_rev":
"15eb928d886a49c90111f25af454be9df4fba3a7"}

[commit-bot: I haz the power, 2017-04-27 04:04:56]

Description was changed from

==========
Make PrefService available in *ash

This CL adds a temporary solution to make the active user's pref
service available in *ash.

BUG=713934
==========

to

==========
Make PrefService available in *ash

This CL adds a temporary solution to make the active user's pref
service available in *ash.

BUG=713934

Review-Url: https://codereview.chromium.org/2827193004
Cr-Commit-Position: refs/heads/master@{#467576}
Committed:
https://chromium.googlesource.com/chromium/src/+/15eb928d886a49c90111f25af454...
==========

[commit-bot: I haz the power, 2017-04-27 04:04:57]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/15eb928d886a49c90111f25af454...