Kiosk mode: SetFirstLoginPrefs() should be called for kiosk sessions.

Kiosk mode: SetFirstLoginPrefs() should be called for kiosk sessions.

SetFirstLoginPrefs() should be called for kiosk sessions.

BUG=445544
TEST=manual

Committed: https://crrev.com/e66a496041854c832b7de03184fe445ed3314613
Cr-Commit-Position: refs/heads/master@{#318239}

[Alexander Alekseev, 2015-02-25 19:49:29]

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

[Alexander Alekseev, 2015-02-25 19:49:29]

First login initialization was never done for kiosk sessions.
This CL fixes it.

Please review.

[xiyuan, 2015-02-25 20:18:43]

https://codereview.chromium.org/954033003/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right):

https://codereview.chromium.org/954033003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:724:
SetIsCurrentUserNew(true);
KioskAppLoggedIn is called on every kiosk app launch. App is like regular user
and it's cryptohome/profile is persisted. So it is not always a "new" user.
Would lying here have side effects?

We had problems with input before. At that time, the bug is fixed
 by explicitly calling InitChromeOSPreferences() on kiosk launch code path in
AppLaunchController::OnProfileLoaded [1]. Could we do something similar and call
SetFirstLoginPrefs with proper args there?

[1]:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

[Alexander Alekseev, 2015-02-25 20:48:47]

https://codereview.chromium.org/954033003/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right):

https://codereview.chromium.org/954033003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:724:
SetIsCurrentUserNew(true);
On 2015/02/25 20:18:43, xiyuan wrote:
> KioskAppLoggedIn is called on every kiosk app launch. App is like regular user
> and it's cryptohome/profile is persisted. So it is not always a "new" user.
> Would lying here have side effects?
> 
> We had problems with input before. At that time, the bug is fixed
>  by explicitly calling InitChromeOSPreferences() on kiosk launch code path in
> AppLaunchController::OnProfileLoaded [1]. Could we do something similar and
call
> SetFirstLoginPrefs with proper args there?
> 
> [1]:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> 

We only need to call SetFirstLoginPrefs for the first Kiosk session.
Is there way to catch first Kiosk session?

[xiyuan, 2015-02-25 21:20:50]

https://codereview.chromium.org/954033003/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right):

https://codereview.chromium.org/954033003/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:724:
SetIsCurrentUserNew(true);
On 2015/02/25 20:48:47, Alexander Alekseev wrote:
> On 2015/02/25 20:18:43, xiyuan wrote:
> > KioskAppLoggedIn is called on every kiosk app launch. App is like regular
user
> > and it's cryptohome/profile is persisted. So it is not always a "new" user.
> > Would lying here have side effects?
> > 
> > We had problems with input before. At that time, the bug is fixed
> >  by explicitly calling InitChromeOSPreferences() on kiosk launch code path
in
> > AppLaunchController::OnProfileLoaded [1]. Could we do something similar and
> call
> > SetFirstLoginPrefs with proper args there?
> > 
> > [1]:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> > 
> 
> We only need to call SetFirstLoginPrefs for the first Kiosk session.
> Is there way to catch first Kiosk session?

Think we can use Profile::IsNewProfile to check whether this is the first kiosk
run.

[Alexander Alekseev, 2015-02-26 14:34:02]

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

[Alexander Alekseev, 2015-02-26 14:34:02]

I've updated initialization so that it now happens only once for Kiosk profile.

Please review.

[xiyuan, 2015-02-26 15:13:18]

LGTM with nits

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

https://codereview.chromium.org/954033003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/user_session_manager.cc:844: }
Think the code should be moved into InitProfilePreferences.

https://codereview.chromium.org/954033003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/user_session_manager.cc:847: } break;
nit: "break" on a separate line.

[Nikita (slow), 2015-02-26 15:16:44]

lgtm, with Xiyuan comments addressed

[Alexander Alekseev, 2015-02-26 15:35:44]

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

https://codereview.chromium.org/954033003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/user_session_manager.cc:844: }
On 2015/02/26 15:13:18, xiyuan wrote:
> Think the code should be moved into InitProfilePreferences.

Done.

https://codereview.chromium.org/954033003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/user_session_manager.cc:847: } break;
On 2015/02/26 15:13:18, xiyuan wrote:
> nit: "break" on a separate line.

Done.

[Alexander Alekseev, 2015-02-26 15:36:32]

The CQ bit was checked by alemate@chromium.org

[Alexander Alekseev, 2015-02-26 15:36:32]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
nkostylev@chromium.org
Link to the patchset: https://codereview.chromium.org/954033003/#ps40001 (title:
"Update after review.")

[commit-bot: I haz the power, 2015-02-26 15:37:35]

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

[commit-bot: I haz the power, 2015-02-26 16:12:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-26 16:13:01]

Patchset 3 (id:??) landed as
https://crrev.com/e66a496041854c832b7de03184fe445ed3314613
Cr-Commit-Position: refs/heads/master@{#318239}