ProfileManager doesn't depend on "--login-profile" switch.

ProfileManager doesn't depend on "--login-profile" switch.

Chrome OS doesn't treat value of "login-profile" as a name
for user's profile dir anymore, however ProfileManager used its
value to let some old tests keep working. After removal
"login-profile" from ProfileManager these tests were fixed.

The significant change is that
TestingProfileManager::CreateTestingProfile treats its
|profile_name| argument as an user's id on CrOS, and creates
profile with a correct path ("u-" +
user_id_hash(profile_name)). This makes testing environment
closer to real, so communication between ProfileManager <-->
chromeos::ProfileHelper <--> chromeos::UserManager works
without additional set up in tests.

BUG=294628
TEST=all browser_tests and unit_tests are passing

Committed: https://crrev.com/0298d729c605a27f041da3c709e865d1ac607ecb
Cr-Commit-Position: refs/heads/master@{#291895}

[dzhioev (left Google), 2014-08-15 12:37:50]

Hi all, please review.

General review:
skuhne@: c/b/profiles/*
nkostylev@: c/b/chromeos/{login,profiles}/*
jcivelli@: testing_profile_manager

Affected unit tests review:
* satorux@: file_system_util_unittest.cc, path_util_unittest.cc,
service_unittest.cc
* shuchen@: input_method_persistence_unittest.cc
* xiyuan@: profile_list_chromeos_unittest.cc
* mukai@ for message_center_settings_controller_unittest.cc
* mattm@ for incident_reporting_service_unittest.cc

[Shu Chen, 2014-08-15 12:54:44]

lgtm for input_method_persistence_unittest.cc

[Mr4D (OOO till 08-26), 2014-08-15 14:27:56]

https://codereview.chromium.org/442043002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/442043002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:464: if (logged_in_) {
I was wondering about guest mode. Looking at the GetActiveUserProfileDir I was
not able to see a change which suggests that that has changed (but then again I
might have looked at the wrong place). Did you test that guest mode still works?

https://codereview.chromium.org/442043002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/442043002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_unittest.cc:152: // for him with a
correct path.
remove "for him".

https://codereview.chromium.org/442043002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_unittest.cc:177: #endif
Could you add DISALLOW_COPY_AND_ASSIGN here?

[dzhioev (left Google), 2014-08-15 16:11:26]

https://codereview.chromium.org/442043002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/442043002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:464: if (logged_in_) {
On 2014/08/15 14:27:56, Mr4D wrote:
> I was wondering about guest mode. Looking at the GetActiveUserProfileDir I was
> not able to see a change which suggests that that has changed (but then again
I
> might have looked at the wrong place). Did you test that guest mode still
works?
Yep, guest mode still works. Guest mode starts in a same way as a session
restore. Session manager passes "--login-user=$guest --
login-profile=[hash_of_mounted_cryptohome]". Then in
ChromeBrowserMainPartsChromeos::PreProfileInit we call UserManager::UserLoggedIn
for the Guest user. It notifies ProfileHelper that active user hash has changed
to Guest user's hash. So GetActiveUserProfileDir starts to return guest's
profile dir from this point.

https://codereview.chromium.org/442043002/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/442043002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_unittest.cc:152: // for him with a
correct path.
On 2014/08/15 14:27:56, Mr4D wrote:
> remove "for him".

Done.

https://codereview.chromium.org/442043002/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_unittest.cc:177: #endif
On 2014/08/15 14:27:56, Mr4D wrote:
> Could you add DISALLOW_COPY_AND_ASSIGN here?

Done.

[Jun Mukai, 2014-08-15 16:16:15]

lgtm: message_center_settings_controller_unittest.cc

[Mr4D (OOO till 08-26), 2014-08-15 16:20:29]

Thanks! LGTM

[xiyuan, 2014-08-15 16:38:43]

LGTM

+asavage since this might affect the chromeos-chrome on linux.

One of my debugging flow is to run directly as a logged-in session. And by
supplying "--login-profile=user", I could specify where the user profile is.
Does this still work with this CL? If not, is there a way?

https://codereview.chromium.org/442043002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.h (right):

https://codereview.chromium.org/442043002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.h:122: std::string
GetUserIdHashByUserIdForTests(const std::string& user_id) const;
nit: Could we make this a static helper so that test code could call it without
ProfileHelper instance.

[Jay Civelli, 2014-08-15 16:48:48]

LGTM for chrome/test/base/testing_profile_manager.cc

[mattm, 2014-08-15 19:57:18]

incident_reporting_service_unittest.cc lgtm (note you'll need to rebase again,
it moved into incident_reporting/ subdirectory).

[dzhioev (left Google), 2014-08-18 16:50:41]

On 2014/08/15 16:38:43, xiyuan wrote:
> LGTM
> 
> +asavage since this might affect the chromeos-chrome on linux.
> 
> One of my debugging flow is to run directly as a logged-in session. And by
> supplying "--login-profile=user", I could specify where the user profile is.
> Does this still work with this CL? If not, is there a way?
Yes, this still works. But take in mind, that this only works for two dedicated
login-profile
values: "user" and "test-user". For all other login-profile values we add "u-"
prefix to construct profile path.
If you just need stub user session you can drop "login-profile" value.
"test-user" directory will
be used in this case.

>
https://codereview.chromium.org/442043002/diff/160001/chrome/browser/chromeos...
> File chrome/browser/chromeos/profiles/profile_helper.h (right):
> 
>
https://codereview.chromium.org/442043002/diff/160001/chrome/browser/chromeos...
> chrome/browser/chromeos/profiles/profile_helper.h:122: std::string
> GetUserIdHashByUserIdForTests(const std::string& user_id) const;
> nit: Could we make this a static helper so that test code could call it
without
> ProfileHelper instance.

[xiyuan, 2014-08-18 16:51:54]

On 2014/08/18 16:50:41, dzhioev wrote:
> On 2014/08/15 16:38:43, xiyuan wrote:
> > LGTM
> > 
> > +asavage since this might affect the chromeos-chrome on linux.
> > 
> > One of my debugging flow is to run directly as a logged-in session. And by
> > supplying "--login-profile=user", I could specify where the user profile is.
> > Does this still work with this CL? If not, is there a way?
> Yes, this still works. But take in mind, that this only works for two
dedicated
> login-profile
> values: "user" and "test-user". For all other login-profile values we add "u-"
> prefix to construct profile path.
> If you just need stub user session you can drop "login-profile" value.
> "test-user" directory will
> be used in this case.
> 

Cool. Thanks for checking it out.

[Nikita (slow), 2014-08-19 11:09:48]

lgtm

https://codereview.chromium.org/442043002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_list_chromeos_unittest.cc (right):

https://codereview.chromium.org/442043002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_list_chromeos_unittest.cc:80: }
nit: drop {}

[dzhioev (left Google), 2014-08-20 20:21:36]

https://codereview.chromium.org/442043002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.h (right):

https://codereview.chromium.org/442043002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.h:122: std::string
GetUserIdHashByUserIdForTests(const std::string& user_id) const;
On 2014/08/15 16:38:43, xiyuan wrote:
> nit: Could we make this a static helper so that test code could call it
without
> ProfileHelper instance.

Done.

https://codereview.chromium.org/442043002/diff/160001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_list_chromeos_unittest.cc (right):

https://codereview.chromium.org/442043002/diff/160001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_list_chromeos_unittest.cc:80: }
On 2014/08/19 11:09:48, Nikita (OOO Aug 22 - Sep 8) wrote:
> nit: drop {}

Done.

[satorux1, 2014-08-21 07:18:14]

defer to mtomasz@ for file system related changes.

[mtomasz, 2014-08-22 05:43:40]

https://codereview.chromium.org/442043002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/drive/file_system_util_unittest.cc (right):

https://codereview.chromium.org/442043002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/file_system_util_unittest.cc:13: #include
"chrome/common/chrome_constants.h"
The chrome_constants.h include is not used anymore. Please remove.

https://codereview.chromium.org/442043002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/file_system_util_unittest.cc:45: Profile*
CreateProfileWithName(const std::string& name) {
nit: This utility method became trivial, so we should remove it.

https://codereview.chromium.org/442043002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/file_system_util_unittest.cc:92: std::string
user1_id_hash =
optional: const would be nice, but it seems that it is not used in this file, so
maybe better to keep consistency.

https://codereview.chromium.org/442043002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/path_util_unittest.cc (right):

https://codereview.chromium.org/442043002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/path_util_unittest.cc:10: #include
"chrome/common/chrome_constants.h"
ditto

[dzhioev (left Google), 2014-08-25 17:39:11]

https://codereview.chromium.org/442043002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/drive/file_system_util_unittest.cc (right):

https://codereview.chromium.org/442043002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/file_system_util_unittest.cc:13: #include
"chrome/common/chrome_constants.h"
On 2014/08/22 05:43:40, mtomasz wrote:
> The chrome_constants.h include is not used anymore. Please remove.

Done.

https://codereview.chromium.org/442043002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/file_system_util_unittest.cc:45: Profile*
CreateProfileWithName(const std::string& name) {
On 2014/08/22 05:43:40, mtomasz wrote:
> nit: This utility method became trivial, so we should remove it.

Done.

https://codereview.chromium.org/442043002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/drive/file_system_util_unittest.cc:92: std::string
user1_id_hash =
On 2014/08/22 05:43:40, mtomasz wrote:
> optional: const would be nice, but it seems that it is not used in this file,
so
> maybe better to keep consistency.

Done.

https://codereview.chromium.org/442043002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_manager/path_util_unittest.cc (right):

https://codereview.chromium.org/442043002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_manager/path_util_unittest.cc:10: #include
"chrome/common/chrome_constants.h"
On 2014/08/22 05:43:40, mtomasz wrote:
> ditto

Done.

[mtomasz, 2014-08-26 00:50:31]

lgtm

[dzhioev (left Google), 2014-08-26 12:11:46]

The CQ bit was checked by dzhioev@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dzhioev@chromium.org/442043002/220001

[commit-bot: I haz the power, 2014-08-26 13:19:21]

Committed patchset #12 (220001) as 716b680741cb71630f789e9fe711aa7bc5cbd295

[commit-bot: I haz the power, 2014-09-10 02:42:32]

Patchset 12 (id:??) landed as
https://crrev.com/0298d729c605a27f041da3c709e865d1ac607ecb
Cr-Commit-Position: refs/heads/master@{#291895}