session_manager: Tracks user sessions

session_manager: Tracks user sessions

- Add SessionManager::CreateSession to track user sessions.
- Wire UserSessionManager::CreateUserSession with it;
- Wire crash-and-restart code to call it too;
- Add ChromeSessionManagerTest and update crash restore tests to
  cover user session tracking;

BUG=657149
TEST=ChromeSessionManagerTest;CrashRestoreSimpleTest;CrashRestoreComplexTest;

Committed: https://crrev.com/30e213c88354e03ea92fa04853150a9e46da077c
Cr-Commit-Position: refs/heads/master@{#429080}

[xiyuan, 2016-10-31 16:20:14]

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

[xiyuan, 2016-10-31 16:20:19]

xiyuan@chromium.org changed reviewers:
+ achuith@chromium.org, dmazzoni@chromium.org, rogerta@chromium.org

[xiyuan, 2016-10-31 16:20:20]

achuith@, please drive the review.

rogerta@, for owner ack of making account_id as session_manager's deps

dmazzoni@, for owner ack of braille_display_private_apitest.cc.

Thanks.

[commit-bot: I haz the power, 2016-10-31 16:20:29]

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-31 16:38:30]

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

[commit-bot: I haz the power, 2016-10-31 16:38:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Roger Tawa OOO till Jul 10th, 2016-10-31 17:20:46]

DEPS lgtm

[xiyuan, 2016-10-31 17:58:55]

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

[commit-bot: I haz the power, 2016-10-31 17:59:13]

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-31 18:32:47]

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

[commit-bot: I haz the power, 2016-10-31 18:32:49]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dmazzoni, 2016-10-31 19:46:51]

lgtm for accessibility and braille_display_private

[achuithb, 2016-10-31 21:55:57]

Overall looks fine; only real concern is the use of the default argument. Tests
look good!

https://codereview.chromium.org/2468483002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc
(right):

https://codereview.chromium.org/2468483002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc:368:
session_manager::SessionManager::Get()->CreateSession(test_account_id_,
Maybe:
auto* session_manager = session_manager::SessionManager::Get();
session_manager->CreateSession()
session_manager->SessionStarted();

Here and elsewhere in the file?

https://codereview.chromium.org/2468483002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/crash_restore_browsertest.cc (right):

https://codereview.chromium.org/2468483002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/crash_restore_browsertest.cc:80:
session_manager::SessionManager* session_manager =
I think auto* is ok here since the type is clear?

https://codereview.chromium.org/2468483002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/crash_restore_browsertest.cc:175:
session_manager::SessionManager* session_manager =
ditto

https://codereview.chromium.org/2468483002/diff/20001/components/session_mana...
File components/session_manager/core/session_manager.cc (right):

https://codereview.chromium.org/2468483002/diff/20001/components/session_mana...
components/session_manager/core/session_manager.cc:47:
DCHECK(std::find_if(sessions_.begin(), sessions_.end(),
Nice

https://codereview.chromium.org/2468483002/diff/20001/components/session_mana...
components/session_manager/core/session_manager.cc:67:
user_manager::UserManager* user_manager = user_manager::UserManager::Get();
auto* may be fine here

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

https://codereview.chromium.org/2468483002/diff/20001/components/session_mana...
components/session_manager/core/session_manager.h:32: bool browser_restart =
false);
I think we're not supposed to use default arguments in C++, right? Especially
for virtual functions:
https://google.github.io/styleguide/cppguide.html#Default_Arguments

[xiyuan, 2016-10-31 22:29:45]

https://codereview.chromium.org/2468483002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc
(right):

https://codereview.chromium.org/2468483002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager_browsertest.cc:368:
session_manager::SessionManager::Get()->CreateSession(test_account_id_,
On 2016/10/31 21:55:56, achuithb wrote:
> Maybe:
> auto* session_manager = session_manager::SessionManager::Get();
> session_manager->CreateSession()
> session_manager->SessionStarted();
> 
> Here and elsewhere in the file?

Done.

https://codereview.chromium.org/2468483002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/crash_restore_browsertest.cc (right):

https://codereview.chromium.org/2468483002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/crash_restore_browsertest.cc:80:
session_manager::SessionManager* session_manager =
On 2016/10/31 21:55:56, achuithb wrote:
> I think auto* is ok here since the type is clear?

Done.

https://codereview.chromium.org/2468483002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/crash_restore_browsertest.cc:175:
session_manager::SessionManager* session_manager =
On 2016/10/31 21:55:56, achuithb wrote:
> ditto

Done.

https://codereview.chromium.org/2468483002/diff/20001/components/session_mana...
File components/session_manager/core/session_manager.cc (right):

https://codereview.chromium.org/2468483002/diff/20001/components/session_mana...
components/session_manager/core/session_manager.cc:67:
user_manager::UserManager* user_manager = user_manager::UserManager::Get();
On 2016/10/31 21:55:57, achuithb wrote:
> auto* may be fine here

Done.

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

https://codereview.chromium.org/2468483002/diff/20001/components/session_mana...
components/session_manager/core/session_manager.h:32: bool browser_restart =
false);
On 2016/10/31 21:55:57, achuithb wrote:
> I think we're not supposed to use default arguments in C++, right? Especially
> for virtual functions:
> https://google.github.io/styleguide/cppguide.html#Default_Arguments

Changed to use function overloading and removed "virtual" since there is no need
to override it ATM.

[achuithb, 2016-10-31 22:41:36]

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

https://codereview.chromium.org/2468483002/diff/40001/components/session_mana...
components/session_manager/core/session_manager.h:31: void CreateSession(const
AccountId& user_account_id,
I feel like I'm being nitpicky, and am fine with this if you strongly prefer
this, but I guess I don't understand why this function is necessary. 

There's only a few callsites that need to be updated, right? And you're already
updating them in this CL. Why not just set the boolean to false at the
callsites?

I think we avoid overloads by using different names. So maybe the second one
could be called CreateSessionAfterRestart for instance, if we want to make a
distinction.

https://google.github.io/styleguide/cppguide.html#Function_Overloading

[xiyuan, 2016-11-01 16:24:17]

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

[xiyuan, 2016-11-01 16:24:31]

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

https://codereview.chromium.org/2468483002/diff/40001/components/session_mana...
components/session_manager/core/session_manager.h:31: void CreateSession(const
AccountId& user_account_id,
On 2016/10/31 22:41:36, achuithb wrote:
> I feel like I'm being nitpicky, and am fine with this if you strongly prefer
> this, but I guess I don't understand why this function is necessary. 
> 
> There's only a few callsites that need to be updated, right? And you're
already
> updating them in this CL. Why not just set the boolean to false at the
> callsites?
> 
> I think we avoid overloads by using different names. So maybe the second one
> could be called CreateSessionAfterRestart for instance, if we want to make a
> distinction.
> 
> https://google.github.io/styleguide/cppguide.html#Function_Overloading
> 

All sites except one are passing a false for the flag. So it feels redundant.

Changed to use a separate name for restart case per discussion.

[commit-bot: I haz the power, 2016-11-01 16:24: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, 2016-11-01 17:12:31]

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

[commit-bot: I haz the power, 2016-11-01 17:12:32]

Dry run: This issue passed the CQ dry run.

[achuithb, 2016-11-01 19:41:25]

Sorry about all the back and forth - the current patch seems more archetypal in
chromium from my experience.

LGTM!

[xiyuan, 2016-11-01 19:54:38]

On 2016/11/01 19:41:25, achuithb wrote:
> Sorry about all the back and forth - the current patch seems more archetypal
in
> chromium from my experience.
> 
> LGTM!

No problem at all. Thank you for the review and comments. It is very helpful.

[xiyuan, 2016-11-01 19:54:58]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-11-01 19:54:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2468483002/#ps60001
(title: "replace func overload with better names")

[commit-bot: I haz the power, 2016-11-01 19:55:16]

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-11-01 20:03:32]

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

[commit-bot: I haz the power, 2016-11-01 20:03:42]

Failed to apply patch for components/session_manager/core/DEPS:
While running git apply --index -3 -p1;
  error: repository lacks the necessary blob to fall back on 3-way merge.
  error: chrome/browser/ui/webui/options/DEPS: patch does not apply

Patch:   NR 
chrome/browser/ui/webui/options/DEPS->components/session_manager/core/DEPS
Index: components/session_manager/core/DEPS
diff --git a/chrome/browser/ui/webui/options/DEPS
b/components/session_manager/core/DEPS
similarity index 100%
copy from chrome/browser/ui/webui/options/DEPS
copy to components/session_manager/core/DEPS

[commit-bot: I haz the power, 2016-11-01 20:05:54]

Description was changed from

==========
session_manager: Tracks user sessions

- Add SessionManager::CreateSession to track user sessions.
- Wire UserSessionManager::CreateUserSession with it;
- Wire crash-and-restart code to call it too;
- Add ChromeSessionManagerTest and update crash restore tests to
  cover user session tracking;

BUG=657149
TEST=ChromeSessionManagerTest;CrashRestoreSimpleTest;CrashRestoreComplexTest;
==========

to

==========
session_manager: Tracks user sessions

- Add SessionManager::CreateSession to track user sessions.
- Wire UserSessionManager::CreateUserSession with it;
- Wire crash-and-restart code to call it too;
- Add ChromeSessionManagerTest and update crash restore tests to
  cover user session tracking;

BUG=657149
TEST=ChromeSessionManagerTest;CrashRestoreSimpleTest;CrashRestoreComplexTest;

Committed: https://crrev.com/30e213c88354e03ea92fa04853150a9e46da077c
Cr-Commit-Position: refs/heads/master@{#429080}
==========

[commit-bot: I haz the power, 2016-11-01 20:05:55]

Patchset 4 (id:??) landed as
https://crrev.com/30e213c88354e03ea92fa04853150a9e46da077c
Cr-Commit-Position: refs/heads/master@{#429080}