[cros] Define session_manager component with SessionManager base class

[cros] Define session_manager component with SessionManager base class

SessionManager is responsible for performing Chrome OS-specific steps to re-launch user session (after crash/stub or in tests) or pre-session UI such as out-of-box or login.

ChromeSessionManager is chrome/browser implementation of SessionManager.

SessionManager is initialized with specific delegate that is reponsible for initial behavior. These delegates are introduced, see ChromeSessionManager::CreateSessionManager():

* LoginOobeSessionManagerDelegate - launches either out-of-box or login UI,
actual branching still happens in ShowLoginWizard()

* RestoreAfterCrashSessionManagerDelegate - responsible for re-launching Chrome into existing user session,
happens after browser process crash or in "stub user" session.

* StubLoginSessionManagerDelegate - starts "stub user" session, when executed on non-CrOS machine w/o parameters or in tests. Extends RestoreAfterCrashSessionManagerDelegate.

* KioskAutoLauncherSessionManagerDelegate - automatically starts kiosk app session.

Code move in ChromeBrowserMainPartsChromeos:
* OptionallyRunChromeOSLoginManager() -> ChromeSessionManager::CreateSessionManager()
* RunAutoLaunchKioskApp() -> KioskAutoLauncherSessionManagerDelegate
* Session restore code in PostProfileInit() -> RestoreAfterCrashSessionManagerDelegate
* Blocks in OptionallyRunChromeOSLoginManager -> to delegates.

BUG=387610
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283437

[Nikita (slow), 2014-07-08 13:48:28]

patchset #18, cros trybots passed
https://uberchromegw.corp.google.com/i/chromiumos.tryserver/builders/x86-gene...
https://uberchromegw.corp.google.com/i/chromiumos.tryserver/builders/amd64-ge...

[Nikita (slow), 2014-07-09 09:10:33]

oshima@, I think this CL is ready for review, 
please take a look. I've resolved all issues with tests that were broken while
working on this change. Mostly by breaking existing assumptions while trying in
unify configurations that tests use.

Turns out some tests don't pass --login-user but still run in a "stub user"
session but w/o user_id set. I've added a TODO for now to look at this later in
the code.

This CL is already big enough even though it is mostly about moving code into
few SessionManagerDelegates.

I wanted to add SessionManager session_state change ovservers but then decided
to postponed it to limit scope of this CL.

[oshima, 2014-07-10 20:07:42]

I'm not big fun of BrowserProcess. I'm ok to use it for now, but I wonder if it
makes sense to use ::Get() pattern instead?

https://codereview.chromium.org/363613004/diff/410001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/363613004/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:26:
chromeos::KioskAppManager* app_manager = chromeos::KioskAppManager::Get();
you're already in chromeos namespace, so you don't need chromeos:: here and the
rest.

https://codereview.chromium.org/363613004/diff/410001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/chrome_session_manager.h (right):

https://codereview.chromium.org/363613004/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.h:28: virtual
~ChromeSessionManager();
can these be in private?

https://codereview.chromium.org/363613004/diff/410001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/session/restore_after_crash_session_manager_delegate.h
(right):

https://codereview.chromium.org/363613004/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/restore_after_crash_session_manager_delegate.h:30:
const std::string& login_user_id() { return login_user_id_; }
const method

https://codereview.chromium.org/363613004/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/restore_after_crash_session_manager_delegate.h:34:
std::string login_user_id_;
can this be const too?

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
File components/session_manager/core/session_manager.h (right):

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
components/session_manager/core/session_manager.h:21: SESSION_STATE_UNKNOWN = 0,
is it important to assign values explicitly?

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
components/session_manager/core/session_manager.h:48: SessionState
session_state() { return session_state_; }
const

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
components/session_manager/core/session_manager.h:62: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
components/session_manager/core/session_manager.h:77: };
ditto

[Nikita (slow), 2014-07-11 06:58:44]

On 2014/07/10 20:07:42, oshima wrote:
> I'm not big fun of BrowserProcess. I'm ok to use it for now, but I wonder if
it
> makes sense to use ::Get() pattern instead?

Sure I can change that. Do you think it would be ok that platform_part still
have an explicit ownership of this or
are you suggesting singleton?

[Nikita (slow), 2014-07-11 06:59:54]

On 2014/07/11 06:58:44, Nikita Kostylev wrote:
> On 2014/07/10 20:07:42, oshima wrote:
> > I'm not big fun of BrowserProcess. I'm ok to use it for now, but I wonder if
> it
> > makes sense to use ::Get() pattern instead?
> 
> Sure I can change that. Do you think it would be ok that platform_part still
> have an explicit ownership of this or
> are you suggesting singleton?

If we make that a singleton then components may use SessionManager::Get() in
both ash and athena.

[Nikita (slow), 2014-07-11 07:00:49]

> If we make that a singleton then components may use SessionManager::Get() in
> both ash and athena.

Since as far as I understood in athena there's no such place as ash::Shell to
have ownership of objects like SessionManager.

[Nikita (slow), 2014-07-14 16:30:11]

https://codereview.chromium.org/363613004/diff/410001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/363613004/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:26:
chromeos::KioskAppManager* app_manager = chromeos::KioskAppManager::Get();
On 2014/07/10 20:07:41, oshima wrote:
> you're already in chromeos namespace, so you don't need chromeos:: here and
the
> rest.

Done.

https://codereview.chromium.org/363613004/diff/410001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/session/restore_after_crash_session_manager_delegate.h
(right):

https://codereview.chromium.org/363613004/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/restore_after_crash_session_manager_delegate.h:30:
const std::string& login_user_id() { return login_user_id_; }
On 2014/07/10 20:07:41, oshima wrote:
> const method

Done.

https://codereview.chromium.org/363613004/diff/410001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/restore_after_crash_session_manager_delegate.h:34:
std::string login_user_id_;
On 2014/07/10 20:07:41, oshima wrote:
> can this be const too?

Done.

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
File components/session_manager/core/session_manager.h (right):

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
components/session_manager/core/session_manager.h:21: SESSION_STATE_UNKNOWN = 0,
On 2014/07/10 20:07:42, oshima wrote:
> is it important to assign values explicitly?

Just to simplify debugging I guess.

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
components/session_manager/core/session_manager.h:48: SessionState
session_state() { return session_state_; }
On 2014/07/10 20:07:42, oshima wrote:
> const

Done.

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
components/session_manager/core/session_manager.h:62: };
On 2014/07/10 20:07:42, oshima wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
components/session_manager/core/session_manager.h:77: };
On 2014/07/10 20:07:42, oshima wrote:
> ditto

Done.

[oshima, 2014-07-14 20:03:54]

lgtm with nits

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
File components/session_manager/core/session_manager.h (right):

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
components/session_manager/core/session_manager.h:21: SESSION_STATE_UNKNOWN = 0,
On 2014/07/14 16:30:11, Nikita Kostylev wrote:
> On 2014/07/10 20:07:42, oshima wrote:
> > is it important to assign values explicitly?
> 
> Just to simplify debugging I guess.

enum automatically initialize the next one with +1. If you want to make sure
UNKONWN to be zero, then you can omit the rest.

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.cc (right):

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:52:
DCHECK(CalledOnValidThread());
Do you know if there is any particular reason why chromeos'es
BrowserProcessPlatformPart is using base::NonThreadSafe instead of checking if
it's on UI thread?

(I'm fine with it, but just curious)

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:75:
login_user_id)));
indent. (or git cl format)

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:88: } else {
nit: you can omit this else.

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1187: bool
oobe_complete = chromeos::StartupUtils::IsOobeCompleted();
or
next_state = IsOobeCompleted() ? STATE_LOGIN_PRIMARY : STATE_OOBE;

https://codereview.chromium.org/363613004/diff/450001/components/session_mana...
File components/session_manager/core/session_manager.h (right):

https://codereview.chromium.org/363613004/diff/450001/components/session_mana...
components/session_manager/core/session_manager.h:19: enum SessionState {
optional: you may defined this outside of SessionManager, as it's already in
session_manager ns.

[oshima, 2014-07-14 20:08:38]

On 2014/07/11 07:00:49, Nikita Kostylev wrote:
> > If we make that a singleton then components may use SessionManager::Get() in
> > both ash and athena.
> 
> Since as far as I understood in athena there's no such place as ash::Shell to
> have ownership of objects like SessionManager.

SessionManager component can own itself. Shell still can initialize it and then
delete upon its deletion.

ash::Shell currently owns a lot of objects, not because they conceptually owns
it, but just to manage
initialization and destruction, and provide accessors, which do not have to be
in Shell.

[Nikita (slow), 2014-07-15 10:18:43]

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
File components/session_manager/core/session_manager.h (right):

https://codereview.chromium.org/363613004/diff/410001/components/session_mana...
components/session_manager/core/session_manager.h:21: SESSION_STATE_UNKNOWN = 0,
On 2014/07/14 20:03:53, oshima wrote:
> On 2014/07/14 16:30:11, Nikita Kostylev wrote:
> > On 2014/07/10 20:07:42, oshima wrote:
> > > is it important to assign values explicitly?
> > 
> > Just to simplify debugging I guess.
> 
> enum automatically initialize the next one with +1. If you want to make sure
> UNKONWN to be zero, then you can omit the rest.

Done.

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:75:
login_user_id)));
On 2014/07/14 20:03:53, oshima wrote:
> indent. (or git cl format)

Done.

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1187: bool
oobe_complete = chromeos::StartupUtils::IsOobeCompleted();
On 2014/07/14 20:03:53, oshima wrote:
> or
> next_state = IsOobeCompleted() ? STATE_LOGIN_PRIMARY : STATE_OOBE;

Done.

https://codereview.chromium.org/363613004/diff/450001/components/session_mana...
File components/session_manager/core/session_manager.h (right):

https://codereview.chromium.org/363613004/diff/450001/components/session_mana...
components/session_manager/core/session_manager.h:19: enum SessionState {
On 2014/07/14 20:03:53, oshima wrote:
> optional: you may defined this outside of SessionManager, as it's already in
> session_manager ns.

Done.

[Nikita (slow), 2014-07-15 10:19:59]

+blundell@ fro components/*

[Nikita (slow), 2014-07-15 10:26:02]

On 2014/07/14 20:08:38, oshima wrote:
> On 2014/07/11 07:00:49, Nikita Kostylev wrote:
> > > If we make that a singleton then components may use SessionManager::Get()
in
> > > both ash and athena.
> > 
> > Since as far as I understood in athena there's no such place as ash::Shell
to
> > have ownership of objects like SessionManager.
> 
> SessionManager component can own itself. Shell still can initialize it and
then
> delete upon its deletion.
> 
> ash::Shell currently owns a lot of objects, not because they conceptually owns
> it, but just to manage
> initialization and destruction, and provide accessors, which do not have to be
> in Shell.

I need to find a design solution for the following task for both SessionManager
and UserManager.

There's a base interface (or class): SessionManager/UserManager. For other
components and both athena/ash
it would be convenient to get these by getters like UserManager::Get()
As discussed in https://codereview.chromium.org/375413002/

At the same time clients in src/chrome might need to access these by their
subclass like
ChromeSessionManager/ChromeUserManager.

[blundell, 2014-07-15 12:12:18]

//components LGTM

On 2014/07/15 10:26:02, Nikita Kostylev wrote:
> On 2014/07/14 20:08:38, oshima wrote:
> > On 2014/07/11 07:00:49, Nikita Kostylev wrote:
> > > > If we make that a singleton then components may use
SessionManager::Get()
> in
> > > > both ash and athena.
> > > 
> > > Since as far as I understood in athena there's no such place as ash::Shell
> to
> > > have ownership of objects like SessionManager.
> > 
> > SessionManager component can own itself. Shell still can initialize it and
> then
> > delete upon its deletion.
> > 
> > ash::Shell currently owns a lot of objects, not because they conceptually
owns
> > it, but just to manage
> > initialization and destruction, and provide accessors, which do not have to
be
> > in Shell.
> 
> I need to find a design solution for the following task for both
SessionManager
> and UserManager.
> 
> There's a base interface (or class): SessionManager/UserManager. For other
> components and both athena/ash
> it would be convenient to get these by getters like UserManager::Get()
> As discussed in https://codereview.chromium.org/375413002/
> 
> At the same time clients in src/chrome might need to access these by their
> subclass like
> ChromeSessionManager/ChromeUserManager.

 In general we try to use embedders' implementations of component interfaces
just for communication between the embedder and the component, not for
communication within the embedder itself.

[Nikita (slow), 2014-07-15 13:32:08]

On 2014/07/15 12:12:18, blundell wrote:
> //components LGTM
> 
> On 2014/07/15 10:26:02, Nikita Kostylev wrote:
> > On 2014/07/14 20:08:38, oshima wrote:
> > > On 2014/07/11 07:00:49, Nikita Kostylev wrote:
> > > > > If we make that a singleton then components may use
> SessionManager::Get()
> > in
> > > > > both ash and athena.
> > > > 
> > > > Since as far as I understood in athena there's no such place as
ash::Shell
> > to
> > > > have ownership of objects like SessionManager.
> > > 
> > > SessionManager component can own itself. Shell still can initialize it and
> > then
> > > delete upon its deletion.
> > > 
> > > ash::Shell currently owns a lot of objects, not because they conceptually
> owns
> > > it, but just to manage
> > > initialization and destruction, and provide accessors, which do not have
to
> be
> > > in Shell.
> > 
> > I need to find a design solution for the following task for both
> SessionManager
> > and UserManager.
> > 
> > There's a base interface (or class): SessionManager/UserManager. For other
> > components and both athena/ash
> > it would be convenient to get these by getters like UserManager::Get()
> > As discussed in https://codereview.chromium.org/375413002/
> > 
> > At the same time clients in src/chrome might need to access these by their
> > subclass like
> > ChromeSessionManager/ChromeUserManager.
> 
>  In general we try to use embedders' implementations of component interfaces
> just for communication between the embedder and the component, not for
> communication within the embedder itself.

What about having
ChromeUserManager* ChromeUserManager::Get() defined in src/chrome and
UserManager* UserManager::Get() defined in src/components/user_manager?

[blundell, 2014-07-15 13:34:08]

On 2014/07/15 13:32:08, Nikita Kostylev wrote:
> On 2014/07/15 12:12:18, blundell wrote:
> > //components LGTM
> > 
> > On 2014/07/15 10:26:02, Nikita Kostylev wrote:
> > > On 2014/07/14 20:08:38, oshima wrote:
> > > > On 2014/07/11 07:00:49, Nikita Kostylev wrote:
> > > > > > If we make that a singleton then components may use
> > SessionManager::Get()
> > > in
> > > > > > both ash and athena.
> > > > > 
> > > > > Since as far as I understood in athena there's no such place as
> ash::Shell
> > > to
> > > > > have ownership of objects like SessionManager.
> > > > 
> > > > SessionManager component can own itself. Shell still can initialize it
and
> > > then
> > > > delete upon its deletion.
> > > > 
> > > > ash::Shell currently owns a lot of objects, not because they
conceptually
> > owns
> > > > it, but just to manage
> > > > initialization and destruction, and provide accessors, which do not have
> to
> > be
> > > > in Shell.
> > > 
> > > I need to find a design solution for the following task for both
> > SessionManager
> > > and UserManager.
> > > 
> > > There's a base interface (or class): SessionManager/UserManager. For other
> > > components and both athena/ash
> > > it would be convenient to get these by getters like UserManager::Get()
> > > As discussed in https://codereview.chromium.org/375413002/
> > > 
> > > At the same time clients in src/chrome might need to access these by their
> > > subclass like
> > > ChromeSessionManager/ChromeUserManager.
> > 
> >  In general we try to use embedders' implementations of component interfaces
> > just for communication between the embedder and the component, not for
> > communication within the embedder itself.
> 
> What about having
> ChromeUserManager* ChromeUserManager::Get() defined in src/chrome and
> UserManager* UserManager::Get() defined in src/components/user_manager?

The question is, why does //chrome need to get the ChromeUserManager? Presumably
it has an extended API beyond that of UserManager? In general we try to avoid
that.

[Nikita (slow), 2014-07-15 13:38:25]

On 2014/07/15 13:34:08, blundell wrote:
> On 2014/07/15 13:32:08, Nikita Kostylev wrote:
> > > > I need to find a design solution for the following task for both
> > > SessionManager
> > > > and UserManager.
> > > > 
> > > > There's a base interface (or class): SessionManager/UserManager. For
other
> > > > components and both athena/ash
> > > > it would be convenient to get these by getters like UserManager::Get()
> > > > As discussed in https://codereview.chromium.org/375413002/
> > > > 
> > > > At the same time clients in src/chrome might need to access these by
their
> > > > subclass like
> > > > ChromeSessionManager/ChromeUserManager.
> > > 
> > >  In general we try to use embedders' implementations of component
interfaces
> > > just for communication between the embedder and the component, not for
> > > communication within the embedder itself.
> > 
> > What about having
> > ChromeUserManager* ChromeUserManager::Get() defined in src/chrome and
> > UserManager* UserManager::Get() defined in src/components/user_manager?
> 
> The question is, why does //chrome need to get the ChromeUserManager?
Presumably
> it has an extended API beyond that of UserManager? In general we try to avoid
> that.

I understand that but it seem that I might need to implement something like that
for things like UserManager/SessionManager.
The reason is that some objects their current implementations own depend on
other things in src/chrome and won't be moved soon or won't be moved at all.
This means that athena will use some subset of UserManager functionality while
ash (and src/chrome) will use all of it.

Initially things like policy/supervised users will need to stay in
ChromeUserManager.

[blundell, 2014-07-15 13:41:01]

On 2014/07/15 13:38:25, Nikita Kostylev wrote:
> On 2014/07/15 13:34:08, blundell wrote:
> > On 2014/07/15 13:32:08, Nikita Kostylev wrote:
> > > > > I need to find a design solution for the following task for both
> > > > SessionManager
> > > > > and UserManager.
> > > > > 
> > > > > There's a base interface (or class): SessionManager/UserManager. For
> other
> > > > > components and both athena/ash
> > > > > it would be convenient to get these by getters like UserManager::Get()
> > > > > As discussed in https://codereview.chromium.org/375413002/
> > > > > 
> > > > > At the same time clients in src/chrome might need to access these by
> their
> > > > > subclass like
> > > > > ChromeSessionManager/ChromeUserManager.
> > > > 
> > > >  In general we try to use embedders' implementations of component
> interfaces
> > > > just for communication between the embedder and the component, not for
> > > > communication within the embedder itself.
> > > 
> > > What about having
> > > ChromeUserManager* ChromeUserManager::Get() defined in src/chrome and
> > > UserManager* UserManager::Get() defined in src/components/user_manager?
> > 
> > The question is, why does //chrome need to get the ChromeUserManager?
> Presumably
> > it has an extended API beyond that of UserManager? In general we try to
avoid
> > that.
> 
> I understand that but it seem that I might need to implement something like
that
> for things like UserManager/SessionManager.
> The reason is that some objects their current implementations own depend on
> other things in src/chrome and won't be moved soon or won't be moved at all.
> This means that athena will use some subset of UserManager functionality while
> ash (and src/chrome) will use all of it.
> 
> Initially things like policy/supervised users will need to stay in
> ChromeUserManager.

I would push for composition rather than inheritance in this case if possible.

[Nikita (slow), 2014-07-15 14:40:19]

On 2014/07/15 13:41:01, blundell wrote:
> On 2014/07/15 13:38:25, Nikita Kostylev wrote:
> > On 2014/07/15 13:34:08, blundell wrote:
> > > On 2014/07/15 13:32:08, Nikita Kostylev wrote:
> > > > > > I need to find a design solution for the following task for both
> > > > > SessionManager
> > > > > > and UserManager.
> > > > > > 
> > > > > > There's a base interface (or class): SessionManager/UserManager. For
> > other
> > > > > > components and both athena/ash
> > > > > > it would be convenient to get these by getters like
UserManager::Get()
> > > > > > As discussed in https://codereview.chromium.org/375413002/
> > > > > > 
> > > > > > At the same time clients in src/chrome might need to access these by
> > their
> > > > > > subclass like
> > > > > > ChromeSessionManager/ChromeUserManager.
> > > > > 
> > > > >  In general we try to use embedders' implementations of component
> > interfaces
> > > > > just for communication between the embedder and the component, not for
> > > > > communication within the embedder itself.
> > > > 
> > > > What about having
> > > > ChromeUserManager* ChromeUserManager::Get() defined in src/chrome and
> > > > UserManager* UserManager::Get() defined in src/components/user_manager?
> > > 
> > > The question is, why does //chrome need to get the ChromeUserManager?
> > Presumably
> > > it has an extended API beyond that of UserManager? In general we try to
> avoid
> > > that.
> > 
> > I understand that but it seem that I might need to implement something like
> that
> > for things like UserManager/SessionManager.
> > The reason is that some objects their current implementations own depend on
> > other things in src/chrome and won't be moved soon or won't be moved at all.
> > This means that athena will use some subset of UserManager functionality
while
> > ash (and src/chrome) will use all of it.
> > 
> > Initially things like policy/supervised users will need to stay in
> > ChromeUserManager.
> 
> I would push for composition rather than inheritance in this case if possible.

I think that we would still need inheritance for the virtual methods like

class UserManager {
 public:
  virtual PrefService* GetLocalState() = 0;
}

PrefService* ChromeUserManager::GetLocalState() {
  return g_browser_process->local_state();
}

[blundell, 2014-07-15 14:42:18]

On 2014/07/15 14:40:19, Nikita Kostylev wrote:
> On 2014/07/15 13:41:01, blundell wrote:
> > On 2014/07/15 13:38:25, Nikita Kostylev wrote:
> > > On 2014/07/15 13:34:08, blundell wrote:
> > > > On 2014/07/15 13:32:08, Nikita Kostylev wrote:
> > > > > > > I need to find a design solution for the following task for both
> > > > > > SessionManager
> > > > > > > and UserManager.
> > > > > > > 
> > > > > > > There's a base interface (or class): SessionManager/UserManager.
For
> > > other
> > > > > > > components and both athena/ash
> > > > > > > it would be convenient to get these by getters like
> UserManager::Get()
> > > > > > > As discussed in https://codereview.chromium.org/375413002/
> > > > > > > 
> > > > > > > At the same time clients in src/chrome might need to access these
by
> > > their
> > > > > > > subclass like
> > > > > > > ChromeSessionManager/ChromeUserManager.
> > > > > > 
> > > > > >  In general we try to use embedders' implementations of component
> > > interfaces
> > > > > > just for communication between the embedder and the component, not
for
> > > > > > communication within the embedder itself.
> > > > > 
> > > > > What about having
> > > > > ChromeUserManager* ChromeUserManager::Get() defined in src/chrome and
> > > > > UserManager* UserManager::Get() defined in
src/components/user_manager?
> > > > 
> > > > The question is, why does //chrome need to get the ChromeUserManager?
> > > Presumably
> > > > it has an extended API beyond that of UserManager? In general we try to
> > avoid
> > > > that.
> > > 
> > > I understand that but it seem that I might need to implement something
like
> > that
> > > for things like UserManager/SessionManager.
> > > The reason is that some objects their current implementations own depend
on
> > > other things in src/chrome and won't be moved soon or won't be moved at
all.
> > > This means that athena will use some subset of UserManager functionality
> while
> > > ash (and src/chrome) will use all of it.
> > > 
> > > Initially things like policy/supervised users will need to stay in
> > > ChromeUserManager.
> > 
> > I would push for composition rather than inheritance in this case if
possible.
> 
> I think that we would still need inheritance for the virtual methods like
> 
> class UserManager {
>  public:
>   virtual PrefService* GetLocalState() = 0;
> }
> 
> PrefService* ChromeUserManager::GetLocalState() {
>   return g_browser_process->local_state();
> }

Agreed. What I mean is that I would prefer that ChromeUserManager be split into
two classes: one that implements the UserManager interface and another that
contains the //chrome-specific APIs that is still needed.

[Nikita (slow), 2014-07-15 17:47:47]

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.cc (right):

https://codereview.chromium.org/363613004/diff/450001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:52:
DCHECK(CalledOnValidThread());
On 2014/07/14 20:03:53, oshima wrote:
> Do you know if there is any particular reason why chromeos'es
> BrowserProcessPlatformPart is using base::NonThreadSafe instead of checking if
> it's on UI thread?
> 
> (I'm fine with it, but just curious)

Not quite sure, I've added it btw. Probably just a paradigm used elsewhere.

[Nikita (slow), 2014-07-15 17:50:04]

+jam@ for owners review

chrome/browser/DEPS
chrome/browser/browser_process_platform_part_chromeos*

[Nikita (slow), 2014-07-15 17:50:14]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-07-15 17:52:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/363613004/530001

[Nikita (slow), 2014-07-15 18:18:38]

The CQ bit was unchecked by nkostylev@chromium.org

[jam, 2014-07-16 05:42:43]

lgtm with nits

https://codereview.chromium.org/363613004/diff/530001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.h (right):

https://codereview.chromium.org/363613004/diff/530001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.h:56: virtual
session_manager::SessionManager* session_manager();
nit: per style guide, virtual methods should use CamelCase and not unix_hacker

https://codereview.chromium.org/363613004/diff/530001/chrome/browser/chromeos...
File chrome/browser/chromeos/DEPS (right):

https://codereview.chromium.org/363613004/diff/530001/chrome/browser/chromeos...
chrome/browser/chromeos/DEPS:3: "+components/session_manager",
this and the third deps change aren't necessary since you added it in
chrome/browser/DEPS

[jam, 2014-07-16 05:42:44]

lgtm with nits

[Nikita (slow), 2014-07-16 08:29:48]

https://codereview.chromium.org/363613004/diff/530001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.h (right):

https://codereview.chromium.org/363613004/diff/530001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.h:56: virtual
session_manager::SessionManager* session_manager();
On 2014/07/16 05:42:43, jam wrote:
> nit: per style guide, virtual methods should use CamelCase and not unix_hacker

Done.

https://codereview.chromium.org/363613004/diff/530001/chrome/browser/chromeos...
File chrome/browser/chromeos/DEPS (right):

https://codereview.chromium.org/363613004/diff/530001/chrome/browser/chromeos...
chrome/browser/chromeos/DEPS:3: "+components/session_manager",
On 2014/07/16 05:42:43, jam wrote:
> this and the third deps change aren't necessary since you added it in
> chrome/browser/DEPS

Done.

[Nikita (slow), 2014-07-16 08:34:03]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-07-16 08:36:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/363613004/570001

[commit-bot: I haz the power, 2014-07-16 12:25:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-16 14:21:45]

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

[commit-bot: I haz the power, 2014-07-16 14:21:46]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Nikita (slow), 2014-07-16 14:37:08]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-07-16 14:39:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/363613004/570001

[commit-bot: I haz the power, 2014-07-16 15:06:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-07-16 15:28:49]

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

[commit-bot: I haz the power, 2014-07-16 15:28:50]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Nikita (slow), 2014-07-16 15:32:39]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-07-16 15:34:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nkostylev@chromium.org/363613004/570001

[commit-bot: I haz the power, 2014-07-16 18:20:48]

Change committed as 283437