[cros] UserManager cleanup regarding stub/demo users.

[cros] UserManager cleanup regarding stub/demo users.

*) Add DCHECK for a second user login attempt.
*) Slightly refactor demo and guest users.


BUG=None
TEST=trybots


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147434

[Nikita (slow), 2012-07-18 12:28:12]

lgtm

http://codereview.chromium.org/10805007/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user.h (right):

http://codereview.chromium.org/10805007/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.h:149: // Is this a guest account?
Please remove fields.

[Ivan Korotkov, 2012-07-18 12:48:54]

http://codereview.chromium.org/10805007/diff/1/chrome/browser/chromeos/login/...
File chrome/browser/chromeos/login/user.h (right):

http://codereview.chromium.org/10805007/diff/1/chrome/browser/chromeos/login/...
chrome/browser/chromeos/login/user.h:149: // Is this a guest account?
On 2012/07/18 12:28:12, Nikita Kostylev wrote:
> Please remove fields.

Done.

[commit-bot: I haz the power, 2012-07-18 14:53:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/10805007/7001

[commit-bot: I haz the power, 2012-07-18 14:53:24]

Presubmit check for 10805007-7001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/chromeos/cros
    chrome/browser/policy

Presubmit checks took 1.2s to calculate.

[Ivan Korotkov, 2012-07-18 17:20:14]

OWNERS approval needed:
cros: oshima
policy: pastarmovj

[pastarmovj, 2012-07-18 19:03:36]

LGTM provided you have checked what I asked for in the first question and you
might want to consider the style nit in my second one.

http://codereview.chromium.org/10805007/diff/7001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

http://codereview.chromium.org/10805007/diff/7001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:251:
DCHECK(!IsUserLoggedIn());
I remember we had some issues with this check the last time I touched this code
with testing on Linux because there the browser starts logged in with the stub
user and can then transition to another user logging in.
Have you checked if this case won't trigger this DCHECK still?

http://codereview.chromium.org/10805007/diff/7001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:289: logged_in_user_ =
CreateUser(email, /* is_ephemeral= */ false);
Do you need those comments here? I can see how you wanted to make it clear this
is not the flag for guest but for ephemeral but I think whoever needs to know
that will read the function definition and know it as long it is well documented
there.

[oshima, 2012-07-18 23:38:50]

lgtm

[Ivan Korotkov, 2012-07-19 12:20:20]

http://codereview.chromium.org/10805007/diff/7001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

http://codereview.chromium.org/10805007/diff/7001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:251:
DCHECK(!IsUserLoggedIn());
On 2012/07/18 19:03:36, pastarmovj wrote:
> I remember we had some issues with this check the last time I touched this
code
> with testing on Linux because there the browser starts logged in with the stub
> user and can then transition to another user logging in.
> Have you checked if this case won't trigger this DCHECK still?

Right. I've recently submitted  which slightly changes this behaviour (stub
login is performed from ChromeBrowserMainPartsChromeos now) and we're
refactoring all tests to add flag --login-manager if they're doing a manual
login later. So this CHECK is to guarantee that no such tests are left :)

http://codereview.chromium.org/10805007/diff/7001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/user_manager_impl.cc:289: logged_in_user_ =
CreateUser(email, /* is_ephemeral= */ false);
On 2012/07/18 19:03:36, pastarmovj wrote:
> Do you need those comments here? I can see how you wanted to make it clear
this
> is not the flag for guest but for ephemeral but I think whoever needs to know
> that will read the function definition and know it as long it is well
documented
> there.

I'd rather leave them because bool arguments always look confusing without a
clear context, like here.

[commit-bot: I haz the power, 2012-07-19 12:20:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/10805007/7001

[commit-bot: I haz the power, 2012-07-19 13:17:44]

Change committed as 147434