session_manager: Create ChromeSessionManager early

session_manager: Create ChromeSessionManager early

- Create ChromeSessionManager early in PreProfileInit so that it can
  be used to track crash-n-restart session that starts there;
- Remove SessionManagerDelgate interface since it is not useful;
- Rename existing SessionManaerDelegate impls to xxxSessionInitializer
  to better reflect what they do;
- Add a ChromeSessionManager::Initialize to replace the
  BrowserProcessPlatformPart::InitializeSessionManager and call one of
  initializers;

BUG=657149
Committed: https://crrev.com/485258770bc096a21594236f7c3ed51492eb651f
Cr-Commit-Position: refs/heads/master@{#428234}

[xiyuan, 2016-10-27 06:00:29]

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

[commit-bot: I haz the power, 2016-10-27 06:00: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, 2016-10-27 06:18:49]

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

[commit-bot: I haz the power, 2016-10-27 06:18:50]

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

[xiyuan, 2016-10-27 07:06:40]

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

[commit-bot: I haz the power, 2016-10-27 07:07:00]

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, 2016-10-27 08:39:35]

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

[commit-bot: I haz the power, 2016-10-27 08:39:36]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2016-10-27 15:51:29]

xiyuan@chromium.org changed reviewers:
+ achuith@chromium.org

[achuithb, 2016-10-27 19:12:45]

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.cc (left):

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:91:
DCHECK(CalledOnValidThread());
Is this DCHECK no longer important?

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

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.h:24: // Initialize
and start user sessions as well as responsible on launching
The grammar here isn't quite right. I don't quite understand what the intent
here is, so I can't help :/

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/session/kiosk_auto_launcher_session_initializer.h
(right):

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/kiosk_auto_launcher_session_initializer.h:17:
void Start();
This is really just a static C-style function invocation, right? The class
doesn't really do anything.

Should we be creating separate classes for each of these Start() methods?

[achuithb, 2016-10-27 19:13:32]

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/session/kiosk_auto_launcher_session_initializer.h
(right):

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/kiosk_auto_launcher_session_initializer.h:17:
void Start();
Why not StartKioskSession(), StartOOBESession(),
StartRestoreAfterCrashSession(), StartStubLoginSession()

[xiyuan, 2016-10-27 21:10:43]

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.cc (left):

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:91:
DCHECK(CalledOnValidThread());
On 2016/10/27 19:12:45, achuithb wrote:
> Is this DCHECK no longer important?

I am not sure why we need it in first place. Original CL
(https://codereview.chromium.org/363613004) is created by following the existing
paradigm without justification. I feel it is redundant since most chrome code
assumes running on UI thread. And other accessors (e.g.
automatic_reboot_manager(), user_manager() etc) do not check thread.

I think this should be okay. And if we do need to guard this, we can make
SessionManager NonThreadSafe and check in its methods.

WDYT?

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

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.h:24: // Initialize
and start user sessions as well as responsible on launching
On 2016/10/27 19:12:45, achuithb wrote:
> The grammar here isn't quite right. I don't quite understand what the intent
> here is, so I can't help :/

Copy-n-pasted from existing BrowserProcessPlatformPart::SessionManager.
Re-phrased it to make it more readable.

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/session/kiosk_auto_launcher_session_initializer.h
(right):

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/kiosk_auto_launcher_session_initializer.h:17:
void Start();
On 2016/10/27 19:13:32, achuithb wrote:
> Why not StartKioskSession(), StartOOBESession(),
> StartRestoreAfterCrashSession(), StartStubLoginSession()

Agree.

Removed those initializer classes and moved the code into functions into
chrome_session_manager.cc.

[xiyuan, 2016-10-27 21:10:49]

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

[commit-bot: I haz the power, 2016-10-27 21:11:51]

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, 2016-10-27 21:16:52]

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

[commit-bot: I haz the power, 2016-10-27 21:16:52]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[achuithb, 2016-10-27 21:18:52]

lgtm

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_process_platform_part_chromeos.cc (left):

https://codereview.chromium.org/2444383008/diff/20001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:91:
DCHECK(CalledOnValidThread());
On 2016/10/27 21:10:43, xiyuan wrote:
> On 2016/10/27 19:12:45, achuithb wrote:
> > Is this DCHECK no longer important?
> 
> I am not sure why we need it in first place. Original CL
> (https://codereview.chromium.org/363613004) is created by following the
existing
> paradigm without justification. I feel it is redundant since most chrome code
> assumes running on UI thread. And other accessors (e.g.
> automatic_reboot_manager(), user_manager() etc) do not check thread.
> 
> I think this should be okay. And if we do need to guard this, we can make
> SessionManager NonThreadSafe and check in its methods.
> 
> WDYT?

Acknowledged.

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

https://codereview.chromium.org/2444383008/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:54: void
StartKioskSession() {
Let's add a short function comment for each of these?

https://codereview.chromium.org/2444383008/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:86: //
TODO(nkostylev): Identify tests that do not set this kLoginUser flag but
Let's remove nkostylev here.

[xiyuan, 2016-10-27 21:29:25]

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

[commit-bot: I haz the power, 2016-10-27 21:30:20]

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

[xiyuan, 2016-10-27 21:30:59]

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

[xiyuan, 2016-10-27 21:31:13]

Thanks.

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

https://codereview.chromium.org/2444383008/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:54: void
StartKioskSession() {
On 2016/10/27 21:18:52, achuithb wrote:
> Let's add a short function comment for each of these?

Done.

https://codereview.chromium.org/2444383008/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:86: //
TODO(nkostylev): Identify tests that do not set this kLoginUser flag but
On 2016/10/27 21:18:52, achuithb wrote:
> Let's remove nkostylev here.

Done.

[commit-bot: I haz the power, 2016-10-27 21:32:17]

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

[xiyuan, 2016-10-27 21:32:49]

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

[xiyuan, 2016-10-27 21:32:49]

+sky for owner review:

chrome/browser/browser_process_platform_part_chromeos.*

Thanks.

[sky, 2016-10-27 22:19:16]

LGTM

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

https://codereview.chromium.org/2444383008/diff/60001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:77:
session_manager_.reset(new chromeos::ChromeSessionManager);
Use base::MakeUnique (see threads on chromium-dev for rationale).

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

https://codereview.chromium.org/2444383008/diff/60001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.h:54: void
CreateSessionManager();
optional: For consistency with the other functions above this how about
InitializeSessionManager?

[commit-bot: I haz the power, 2016-10-27 22:58:38]

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

[commit-bot: I haz the power, 2016-10-27 22:58:39]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2016-10-27 22:59:55]

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

https://codereview.chromium.org/2444383008/diff/60001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.cc:77:
session_manager_.reset(new chromeos::ChromeSessionManager);
On 2016/10/27 22:19:16, sky wrote:
> Use base::MakeUnique (see threads on chromium-dev for rationale).

Done.

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

https://codereview.chromium.org/2444383008/diff/60001/chrome/browser/browser_...
chrome/browser/browser_process_platform_part_chromeos.h:54: void
CreateSessionManager();
On 2016/10/27 22:19:16, sky wrote:
> optional: For consistency with the other functions above this how about
> InitializeSessionManager?

Done.

[xiyuan, 2016-10-27 23:12:59]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-10-27 23:12:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2444383008/#ps80001
(title: "for #4")

[commit-bot: I haz the power, 2016-10-27 23:14:11]

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, 2016-10-28 01:38:58]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-28 01:43:25]

Description was changed from

==========
session_manager: Create ChromeSessionManager early

- Create ChromeSessionManager early in PreProfileInit so that it can
  be used to track crash-n-restart session that starts there;
- Remove SessionManagerDelgate interface since it is not useful;
- Rename existing SessionManaerDelegate impls to xxxSessionInitializer
  to better reflect what they do;
- Add a ChromeSessionManager::Initialize to replace the
  BrowserProcessPlatformPart::InitializeSessionManager and call one of
  initializers;

BUG=657149
==========

to

==========
session_manager: Create ChromeSessionManager early

- Create ChromeSessionManager early in PreProfileInit so that it can
  be used to track crash-n-restart session that starts there;
- Remove SessionManagerDelgate interface since it is not useful;
- Rename existing SessionManaerDelegate impls to xxxSessionInitializer
  to better reflect what they do;
- Add a ChromeSessionManager::Initialize to replace the
  BrowserProcessPlatformPart::InitializeSessionManager and call one of
  initializers;

BUG=657149

Committed: https://crrev.com/485258770bc096a21594236f7c3ed51492eb651f
Cr-Commit-Position: refs/heads/master@{#428234}
==========

[commit-bot: I haz the power, 2016-10-28 01:43:26]

Patchset 5 (id:??) landed as
https://crrev.com/485258770bc096a21594236f7c3ed51492eb651f
Cr-Commit-Position: refs/heads/master@{#428234}