[cros] Fake a stub user login when no --login-manager and --login-user are given.

[cros] Fake a stub user login when no --login-manager and --login-user are given.

This is a reland of 142944 with fixes for tests.

BUG=126765
TEST=Manual on Linux without --login-manager and --login-user


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

[Ivan Korotkov, 2012-06-20 12:46:48]

PTAL
#1 is identical to the previous CL, new changes to tests are in patchsets #2 and
#3.

[Nikita (slow), 2012-06-20 13:28:32]

lgtm

http://codereview.chromium.org/10590004/diff/3013/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/10590004/diff/3013/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:248: // If we're not
running on ChromeOS, and are not showing the login manager
nit: Should be "If we're not running on real ChromeOS hardware / CrOS VM image"
i.e. either build executed on dev workstation or Chromium buildbot.

http://codereview.chromium.org/10590004/diff/3013/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc
(right):

http://codereview.chromium.org/10590004/diff/3013/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:176:
command_line->AppendSwitch(switches::kLoginManager);
Please use NiceMock as discussed to cleanup output.
I expect that adding --login-manager here might pollute output even more.

http://codereview.chromium.org/10590004/diff/3013/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_browsertest.cc (right):

http://codereview.chromium.org/10590004/diff/3013/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_browsertest.cc:67: class LoginProfileTest :
public LoginUserTest {
nit: LoginGuestProfileTest ?

http://codereview.chromium.org/10590004/diff/3013/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_browsertest.cc:70:
command_line->AppendSwitch(switches::kGuestSession);
FYI, Guest session is initialized as

  command_line->AppendSwitch(::switches::kGuestSession);
  command_line->AppendSwitch(::switches::kIncognito);

http://codereview.chromium.org/10590004/diff/3013/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_browsertest.cc:86:
IN_PROC_BROWSER_TEST_F(LoginProfileTest, UserNotPassed) {
Please update test name + comment.
I guess it is testing Guest session case now.

http://codereview.chromium.org/10590004/diff/3013/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/drm_settings.cc (right):

http://codereview.chromium.org/10590004/diff/3013/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/drm_settings.cc:107: // Don't generate files as a
guest, demo user, or a stub.
nit: update comment

[Ivan Korotkov, 2012-06-21 12:25:12]

Updated gmock expectations in MockDbusThreadManager to produce less noise

https://chromiumcodereview.appspot.com/10590004/diff/3013/chrome/browser/chro...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://chromiumcodereview.appspot.com/10590004/diff/3013/chrome/browser/chro...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:248: // If we're not
running on ChromeOS, and are not showing the login manager
On 2012/06/20 13:28:32, Nikita Kostylev wrote:
> nit: Should be "If we're not running on real ChromeOS hardware / CrOS VM
image"
> i.e. either build executed on dev workstation or Chromium buildbot.

Done.

https://chromiumcodereview.appspot.com/10590004/diff/3013/chrome/browser/chro...
File chrome/browser/chromeos/login/existing_user_controller_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10590004/diff/3013/chrome/browser/chro...
chrome/browser/chromeos/login/existing_user_controller_browsertest.cc:176:
command_line->AppendSwitch(switches::kLoginManager);
On 2012/06/20 13:28:32, Nikita Kostylev wrote:
> Please use NiceMock as discussed to cleanup output.
> I expect that adding --login-manager here might pollute output even more.

Done.

https://chromiumcodereview.appspot.com/10590004/diff/3013/chrome/browser/chro...
File chrome/browser/chromeos/login/login_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10590004/diff/3013/chrome/browser/chro...
chrome/browser/chromeos/login/login_browsertest.cc:67: class LoginProfileTest :
public LoginUserTest {
On 2012/06/20 13:28:32, Nikita Kostylev wrote:
> nit: LoginGuestProfileTest ?

Done.

https://chromiumcodereview.appspot.com/10590004/diff/3013/chrome/browser/chro...
chrome/browser/chromeos/login/login_browsertest.cc:70:
command_line->AppendSwitch(switches::kGuestSession);
On 2012/06/20 13:28:32, Nikita Kostylev wrote:
> FYI, Guest session is initialized as
> 
>   command_line->AppendSwitch(::switches::kGuestSession);
>   command_line->AppendSwitch(::switches::kIncognito);

Added --incognito, though it shouldn't have any effect here.

https://chromiumcodereview.appspot.com/10590004/diff/3013/chrome/browser/chro...
chrome/browser/chromeos/login/login_browsertest.cc:86:
IN_PROC_BROWSER_TEST_F(LoginProfileTest, UserNotPassed) {
On 2012/06/20 13:28:32, Nikita Kostylev wrote:
> Please update test name + comment.
> I guess it is testing Guest session case now.

Done.

https://chromiumcodereview.appspot.com/10590004/diff/3013/chrome/browser/chro...
File chrome/browser/chromeos/system/drm_settings.cc (right):

https://chromiumcodereview.appspot.com/10590004/diff/3013/chrome/browser/chro...
chrome/browser/chromeos/system/drm_settings.cc:107: // Don't generate files as a
guest, demo user, or a stub.
On 2012/06/20 13:28:32, Nikita Kostylev wrote:
> nit: update comment

Done.

[Ivan Korotkov, 2012-06-21 16:29:54]

OWNERS, could you please take a look?

sky: ui/prefs/
sail: profiles/

[sky, 2012-06-21 17:59:02]

LGTM

[sail, 2012-06-22 18:46:56]

Hi Nikita, sorry for the slow response. I'm away right now but maybe erg or rlp
could take a look.

http://codereview.chromium.org/10590004/diff/22002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/10590004/diff/22002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_manager.cc:168: #if defined(OS_CHROMEOS)
This code is getting increasingly hard to follow. It would be nice to clean this
up. Some things wrong with this:
  - it duplicates chrome::kInitialProfile
  - it's not referenced by any of the ProfileManager code that uses the default
profile
  - it adds more conditional logic to the profile code wich makes it more
fragile

Some suggestions:
  - can you change the value of chrome::kInitialProfile to "test-user" for
chrome os?
  - if not can you call ProfileManager::GetDafultProfile()->GetPath()->DirName()
  - if not can you make this a function of ProfileManager and update all
references to chrome::kInitialProfile?

[Ivan Korotkov, 2012-06-25 14:32:24]

Hi Sailesh,

I guess there has been some misunderstanding about this CL. It basically adds a
--login-profile=test-user argument to the most of browser_tests that run under
Linux (not on device) for chromeos target.

These tests are assuming that the initial profile dir is "Default" (not using
kInitialProfile) so they started to fail with this change. The constant I've
added is purposed solely for use by browser_tests, ui_tests, etc. Hence it has
nothing to do with kInitialProfile, actually.

I can rename it to kTestUserInitialProfile and move it to the same file
kInitialProfile resides in, for better consistency, WDYT?

[rpetterson, 2012-06-25 17:50:41]

I'm still a bit confused. Wouldn't it make more sense to pass the directory to
the test itself? 

Also, if it's something that is specifically for testing, perhaps it would be
better to put it in testing_profile or testing_profile_manager?

On 2012/06/25 14:32:24, Ivan Korotkov wrote:
> Hi Sailesh,
> 
> I guess there has been some misunderstanding about this CL. It basically adds
a
> --login-profile=test-user argument to the most of browser_tests that run under
> Linux (not on device) for chromeos target.
> 
> These tests are assuming that the initial profile dir is "Default" (not using
> kInitialProfile) so they started to fail with this change. The constant I've
> added is purposed solely for use by browser_tests, ui_tests, etc. Hence it has
> nothing to do with kInitialProfile, actually.
> 
> I can rename it to kTestUserInitialProfile and move it to the same file
> kInitialProfile resides in, for better consistency, WDYT?

[sail, 2012-06-25 18:52:25]

> These tests are assuming that the initial profile dir is "Default" (not using
> kInitialProfile) so they started to fail with this change.

Ahh, makes sense. It seems like the constant should be moved to the code that
creates the profile directory then.

Also, it might be better to change the code that creates the test profile
directory to just use kInitialProfile.

[Ivan Korotkov, 2012-06-26 16:37:44]

On ChromeOS, there's an explicit DCHECK that a logged-in user cannot have
"Default" as profile directory, so we have to use another directory (before this
CL, ChromeOS tests were running in an intermediate no-user/no-login-manager
state).

So, this has to be another dir on ChromeOS. I guess moving it to testing_profile
makes sense.

Rachel, are you suggesting to add method like GetInitialProfileDir() to
InProcessBrowserTest or what?

[rpetterson, 2012-06-28 19:00:38]

I think renaming it as you've done to kTestUserProfile is good. Perhaps
kTestUserProfileDir would be better to indicate that it's a directory? But
that's a nit.

I would like to see it moved to the testing_profile if it's something that is
only used for tests.

On 2012/06/26 16:37:44, Ivan Korotkov wrote:
> On ChromeOS, there's an explicit DCHECK that a logged-in user cannot have
> "Default" as profile directory, so we have to use another directory (before
this
> CL, ChromeOS tests were running in an intermediate no-user/no-login-manager
> state).
> 
> So, this has to be another dir on ChromeOS. I guess moving it to
testing_profile
> makes sense.
> 
> Rachel, are you suggesting to add method like GetInitialProfileDir() to
> InProcessBrowserTest or what?

[Ivan Korotkov, 2012-07-02 12:51:06]

Uploaded new patchset

[rpetterson, 2012-07-09 21:12:22]

LGTM

[commit-bot: I haz the power, 2012-07-17 11:05:12]

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

[commit-bot: I haz the power, 2012-07-17 12:12:02]

Try job failure for 10590004-58001 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-07-17 12:19:01]

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

[commit-bot: I haz the power, 2012-07-17 13:47:27]

Change committed as 146991