[session_manager] Move user session initialization code out of ExistingUserController

[session_manager] Move user session initialization code out of ExistingUserController

Added UserSessionManager::InitializeUserSession()
(1) Launches browser for most common case
(2) Continues new user sign in with TOS (public sessions)/image screen (new users)
(3) For kiosk flow delegates launching app to the existing kiosk initialization flow

(1) Currently results in LoginUtils::Get()->DoBrowserLaunch() which may postpone launching browser for these reasons:
(a) There's custom user login flow defined like SupervisedUserCreationFlow. In that case login UI continues to live and custom flow UI is launched in that context.
(b) User has different app locale which requires reloading resource_bundle prior to launching browser
(c) User has custom flags set (or user flags are different from login screen flags defined by the owner), this requires restarting Chrome which will be launched in the active user session.

Small refactoring in ExistingUserController, added

* PerformPreLoginActions() - performs sets of actions right prior to login has been started.
* PerformLoginFinishedActions() - performs set of actions when login has been completed or has been cancelled.

BUG=370175
Committed: https://crrev.com/609c2d1de4a65aa066e3b518186100b0a76ef58e
Cr-Commit-Position: refs/heads/master@{#300708}

[Nikita (slow), 2014-10-16 11:27:11]

nkostylev@chromium.org changed reviewers:
+ antrim@chromium.org

[Nikita (slow), 2014-10-16 11:27:12]

https://codereview.chromium.org/656283002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/existing_user_controller.cc (left):

https://codereview.chromium.org/656283002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller.cc:882:
user_manager::UserManager* user_manager = user_manager::UserManager::Get();
This block is moved to SupervisedUserLoginFlow.

https://codereview.chromium.org/656283002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller.cc:889: if
(user_manager->IsCurrentUserNew() &&
This block is moved to UserSessionManager::InitializeUserSession

[Denis Kuznetsov (DE-MUC), 2014-10-16 12:21:42]

https://chromiumcodereview.appspot.com/656283002/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://chromiumcodereview.appspot.com/656283002/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/existing_user_controller.cc:879:
auth_status_consumer_->OnAuthSuccess(UserContext());
Why do we pass empty user context here, what if consumer expects at least user
id there?

https://chromiumcodereview.appspot.com/656283002/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/login_utils.h (right):

https://chromiumcodereview.appspot.com/656283002/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/login_utils.h:33: bool browser_launched) = 0;
Please add comment about parameter.
It is not clear by the current comment if it will be called two times with false
then true, or if it will be called just once.

https://chromiumcodereview.appspot.com/656283002/diff/20001/chrome/browser/ch...
File chrome/browser/chromeos/login/user_flow.h (right):

https://chromiumcodereview.appspot.com/656283002/diff/20001/chrome/browser/ch...
chrome/browser/chromeos/login/user_flow.h:30: 
Do we need empty line here?

[Nikita (slow), 2014-10-17 13:17:42]

I still need to figure out why tests are failing, but I've addressed all your
comments.

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

https://codereview.chromium.org/656283002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/existing_user_controller.cc:879:
auth_status_consumer_->OnAuthSuccess(UserContext());
On 2014/10/16 12:21:42, Denis Kuznetsov wrote:
> Why do we pass empty user context here, what if consumer expects at least user
> id there?

Done.

https://codereview.chromium.org/656283002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/login_utils.h (right):

https://codereview.chromium.org/656283002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/login_utils.h:33: bool browser_launched) = 0;
On 2014/10/16 12:21:42, Denis Kuznetsov wrote:
> Please add comment about parameter.
> It is not clear by the current comment if it will be called two times with
false
> then true, or if it will be called just once.

Done.

https://codereview.chromium.org/656283002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/user_flow.h (right):

https://codereview.chromium.org/656283002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/user_flow.h:30: 
On 2014/10/16 12:21:42, Denis Kuznetsov wrote:
> Do we need empty line here?

Done.

[Nikita (slow), 2014-10-17 16:39:08]

Patchset #4 (id:60001) has been deleted

[Nikita (slow), 2014-10-20 05:47:46]

ptal, all tests were fixed (changes in FakeLoginUtils + kiosk case separately).

[Nikita (slow), 2014-10-20 10:15:34]

Patchset #6 (id:120001) has been deleted

[Nikita (slow), 2014-10-20 10:16:29]

nkostylev@chromium.org changed reviewers:
+ rogerta@chromium.org

[Nikita (slow), 2014-10-20 10:16:30]

+rogerta@ for google_apis/gaia/*

[Nikita (slow), 2014-10-20 12:32:01]

nkostylev@chromium.org changed reviewers:
+ tnagel@chromium.org

[Nikita (slow), 2014-10-20 12:32:01]

nkostylev@chromium.org changed required reviewers:
+ antrim@chromium.org, rogerta@chromium.org

[Nikita (slow), 2014-10-20 12:40:48]

+ tnagel@ for couple of clarifications (see comments).

https://codereview.chromium.org/656283002/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/656283002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:476:
PerformLoginFinishedActions(false /* don't start public session timer */);
Thiemo, I think starting public session timer is not needed in this case. This
is to be consistent with similar case (line 530).

https://codereview.chromium.org/656283002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:909:
PerformLoginFinishedActions(false /* don't start public session timer */);
Thiemo, I think starting public session timer is not needed in this case i.e.
when policy failed to load.

[Denis Kuznetsov (DE-MUC), 2014-10-20 13:38:59]

https://chromiumcodereview.appspot.com/656283002/diff/100001/chrome/browser/c...
File chrome/browser/chromeos/login/fake_login_utils.cc (right):

https://chromiumcodereview.appspot.com/656283002/diff/100001/chrome/browser/c...
chrome/browser/chromeos/login/fake_login_utils.cc:101:
LoginDisplayHostImpl::default_host());
browser_launch = true?

https://chromiumcodereview.appspot.com/656283002/diff/100001/chrome/browser/c...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://chromiumcodereview.appspot.com/656283002/diff/100001/chrome/browser/c...
chrome/browser/chromeos/login/session/user_session_manager.cc:947: // Kiosk apps
has their on session initialization pipeline.
on->own?

https://chromiumcodereview.appspot.com/656283002/diff/140001/chrome/browser/c...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://chromiumcodereview.appspot.com/656283002/diff/140001/chrome/browser/c...
chrome/browser/chromeos/login/existing_user_controller.cc:447:
PerformPreLoginActions(UserContext(chromeos::login::kRetailModeUserName));
You might want to use 
UserContext(user_manager::UserType user_type, const std::string& user_id)
constructor here and below.

https://chromiumcodereview.appspot.com/656283002/diff/140001/chrome/browser/c...
chrome/browser/chromeos/login/existing_user_controller.cc:909:
PerformLoginFinishedActions(true);
Check if it makes sense to restart timer in policy failed/permanently untrusted
states.

https://chromiumcodereview.appspot.com/656283002/diff/140001/chrome/browser/c...
chrome/browser/chromeos/login/existing_user_controller.cc:1140:
is_login_in_progress_ = true;
Reorder with last statement.

https://chromiumcodereview.appspot.com/656283002/diff/140001/google_apis/gaia...
File google_apis/gaia/gaia_auth_util.cc (right):

https://chromiumcodereview.appspot.com/656283002/diff/140001/google_apis/gaia...
google_apis/gaia/gaia_auth_util.cc:29: << (parts.size() ? parts.size() - 1 :
parts.size())
Use 0 in else-case (it look weird otherwise). And condition can use empty()
instead of size().

[Nikita (slow), 2014-10-20 13:52:11]

https://codereview.chromium.org/656283002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/fake_login_utils.cc (right):

https://codereview.chromium.org/656283002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/fake_login_utils.cc:101:
LoginDisplayHostImpl::default_host());
On 2014/10/20 13:38:59, Denis Kuznetsov wrote:
> browser_launch = true?

Done.

https://codereview.chromium.org/656283002/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/656283002/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:947: // Kiosk apps
has their on session initialization pipeline.
On 2014/10/20 13:38:59, Denis Kuznetsov wrote:
> on->own?

Done.

https://codereview.chromium.org/656283002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/656283002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:447:
PerformPreLoginActions(UserContext(chromeos::login::kRetailModeUserName));
On 2014/10/20 13:38:59, Denis Kuznetsov wrote:
> You might want to use 
> UserContext(user_manager::UserType user_type, const std::string& user_id)
> constructor here and below.

Done.

https://codereview.chromium.org/656283002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:909:
PerformLoginFinishedActions(true);
On 2014/10/20 13:38:59, Denis Kuznetsov wrote:
> Check if it makes sense to restart timer in policy failed/permanently
untrusted
> states.

Done.

https://codereview.chromium.org/656283002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:1140:
is_login_in_progress_ = true;
On 2014/10/20 13:38:59, Denis Kuznetsov wrote:
> Reorder with last statement.

Done.

https://codereview.chromium.org/656283002/diff/140001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_util.cc (right):

https://codereview.chromium.org/656283002/diff/140001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_util.cc:29: << (parts.size() ? parts.size() - 1 :
parts.size())
On 2014/10/20 13:38:59, Denis Kuznetsov wrote:
> Use 0 in else-case (it look weird otherwise). And condition can use empty()
> instead of size().

Done.

[Roger Tawa OOO till Jul 10th, 2014-10-20 14:32:00]

google_apis lgtm

[Denis Kuznetsov (DE-MUC), 2014-10-21 14:58:52]

lgtm

[Nikita (slow), 2014-10-21 15:04:33]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-10-21 15:05:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/656283002/260001

[commit-bot: I haz the power, 2014-10-21 16:36:26]

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

[commit-bot: I haz the power, 2014-10-21 16:36:30]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Thiemo Nagel, 2014-10-22 08:40:01]

May I ask to fully spell out "EUC" in the title of the CL?  The acronym is hard
to understand for people that are not familiar with the code.

https://codereview.chromium.org/656283002/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/656283002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:476:
PerformLoginFinishedActions(false /* don't start public session timer */);
I'm the wrong person to ask about this, I've never worked on auto-login. 
Looking at the history of the file, dconnelly@ or bartfab@ might know more.

[Nikita (slow), 2014-10-22 08:43:32]

nkostylev@chromium.org changed reviewers:
+ dconnelly@chromium.org

[Nikita (slow), 2014-10-22 10:22:25]

https://codereview.chromium.org/656283002/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/existing_user_controller.cc (right):

https://codereview.chromium.org/656283002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:476:
PerformLoginFinishedActions(false /* don't start public session timer */);
On 2014/10/22 08:40:01, Thiemo Nagel wrote:
> I'm the wrong person to ask about this, I've never worked on auto-login. 
> Looking at the history of the file, dconnelly@ or bartfab@ might know more.

+dconnelly@

https://codereview.chromium.org/656283002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/existing_user_controller.cc:909:
PerformLoginFinishedActions(false /* don't start public session timer */);
On 2014/10/20 12:40:47, Nikita Kostylev wrote:
> Thiemo, I think starting public session timer is not needed in this case i.e.
> when policy failed to load.

+dconnelly@

[Nikita (slow), 2014-10-22 12:00:00]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-10-22 12:01:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/656283002/280001

[commit-bot: I haz the power, 2014-10-22 12:08:01]

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

[commit-bot: I haz the power, 2014-10-22 12:08:06]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Nikita (slow), 2014-10-22 14:22:18]

The CQ bit was checked by nkostylev@chromium.org

[commit-bot: I haz the power, 2014-10-22 14:23:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/656283002/280001

[commit-bot: I haz the power, 2014-10-22 18:14:27]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2014-10-22 18:15:12]

Patchset 13 (id:??) landed as
https://crrev.com/609c2d1de4a65aa066e3b518186100b0a76ef58e
Cr-Commit-Position: refs/heads/master@{#300708}