Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11419184:
Add public accounts to UserManager (Closed)
|
Created:
8 years ago by bartfab (slow) Modified:
8 years ago CC:
chromium-reviews, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org, xiyuan Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd public accounts to UserManager
This CL extends the UserManager to handle public accounts defined through
policy. User pods are dynamically added and removed when the list of
public accounts in policy changes. Any data belonging to obsolete accounts
is also removed, taking care not to remove it prematurely if a user is
currently logged into the account.
The CL also makes the user list handling more robust by checking for
duplicate entries in the user list prefs and logging these as errors.
The pods added for public accounts are not functional yet. The login flow
for public accounts will be the topic of another CL.
BUG=158509
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170507
Patch Set 1 #Patch Set 2 : Use weak_ptr to handle UserManagerImpl/CrosSettings shutdown race. Fix unit tests. #Patch Set 3 : Re-upload against the correct upstream commit. #
Total comments: 28
Patch Set 4 : Comments addressed. Note that browser tests still hang... still investigating. #Patch Set 5 : Back to mutable, avoiding a dangerous and ugly const_cast in UserManagerImpl::GetUsers(). #Patch Set 6 : Fix WallpaperManagerBrowserTest. #Patch Set 7 : Quickly fix comment in typo before anyone notices... #
Total comments: 34
Patch Set 8 : Comments addressed. Replaced all references to "device-local accounts" with "public accounts". #Patch Set 9 : Comments addressed. #Patch Set 10 : Removed an unnecessary variable. #
Total comments: 4
Patch Set 11 : Comment addressed. #Patch Set 12 : Made comment easier to understand. #Messages
Total messages: 25 (0 generated)
Hi Ivan, Could you please review? Hi Xiyuan, FYI, this is my implementation of public accounts in the UserManager C++ backend.
Hi Julian, Could you take a look at the chrome/browser/chromeos/settings changes?
On 2012/11/27 17:58:19, bartfab wrote: > Hi Julian, > > Could you take a look at the chrome/browser/chromeos/settings changes? There is still something in this CL that makes some browser tests hang. I will investigate that tomorrow. Feel free to start reviewing in the meantime.
A few comments from me. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/existing_user_controller.cc:192: // TODO(xiyuan): Clean user profile whose email is not in whitelist. Don't you do the clean up from this TODO in UseRManagerImpl now. Maybe you can clean up this todo if this is the case. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:246: // Make sure the modified data is persisted to Local State. I am not convinced we need all those CommitPendingWrites all over this file. The local state is persisted automatically often enough and those writes can only slow down login for new users. I guess they have been added in an attempt to prevent crashes in the browser from wiping new users but I think that instead of doing that we should be more diligent in not allowing crashes in the first place. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:278: UpdateRegularUsers(email, false); I have a proposal about the second parameter of this function. Why not make it an enum so that you can have more descriptive names for the operations. It seems quite strange to me that depending on this param being true or false the behavior changes so vastly. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:571: LOG(ERROR) << "Corrupt entry in user list."; Please print the index here too. Could be helpful for some debugging. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:574: if ((existing_users.find(email) != existing_users.end()) || I think you don't need the extra parenthesis around a != b. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:593: if (!g_browser_process) Swap the two ifs and split them with an empty line please. The one is a precondition and the other is part of the functionality of this proc. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:692: cros_settings_->AddSettingsObserver(kAccountsPrefDeviceLocalAccounts, Is there any reason you don't simply do this in the constructor and spare the bool flag? https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:779: if (!user) { Please also document that this function will actually create a new user if it doesn't exist. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... File chrome/browser/chromeos/login/user_manager_impl.h (right): https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.h:108: const ListValue& users_list, nit: move |users_vector| and |users_set| to be last arguments. It seems that there is weak common preference to have src,dst argument ordering. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.h:149: // front, as specified by |move_to_front|. Can you please put some description why is this needed because it sounds like a very arbitrary operation from this description. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.h:153: // that data stored outside theit cryptohomes is removed for any users who are s/theit/their/ https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.h:168: // |UpdateLocalUsers|. Write a justification for having this member mutable please. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... File chrome/browser/chromeos/settings/cros_settings.cc (right): https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/settings/cros_settings.cc:304: if (observer) { I don't see much sense in this. Why do you need this check and in general this seems like a wrong use of the observer interface if you have to catch this case like that. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... File chrome/browser/chromeos/settings/cros_settings.h (right): https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/settings/cros_settings.h:110: typedef ObserverList<content::NotificationObserver, false> Why did you do that? Is this observer not removed properly?
https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... File chrome/browser/chromeos/login/existing_user_controller.cc (right): https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/existing_user_controller.cc:192: // TODO(xiyuan): Clean user profile whose email is not in whitelist. On 2012/11/28 09:57:16, pastarmovj wrote: > Don't you do the clean up from this TODO in UseRManagerImpl now. Maybe you can > clean up this todo if this is the case. No, I only clean up device-local accounts that no longer exist. The UserManager does not reason about the whitelist and hence, will not clean up the users referenced in this TODO. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:246: // Make sure the modified data is persisted to Local State. On 2012/11/28 09:57:16, pastarmovj wrote: > I am not convinced we need all those CommitPendingWrites all over this file. The > local state is persisted automatically often enough and those writes can only > slow down login for new users. I guess they have been added in an attempt to > prevent crashes in the browser from wiping new users but I think that instead of > doing that we should be more diligent in not allowing crashes in the first > place. I removed the CommitPendingWrite() calls. I agree that relying on prefs doing the right thing is much more elegant. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:278: UpdateRegularUsers(email, false); On 2012/11/28 09:57:16, pastarmovj wrote: > I have a proposal about the second parameter of this function. Why not make it > an enum so that you can have more descriptive names for the operations. It seems > quite strange to me that depending on this param being true or false the > behavior changes so vastly. I refactored the code. There is now a |RemoveUserFromListInternal| method that does what it says on the tin only. If the user needs to be added at the front of the list, that is done explicitly outside the method. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:571: LOG(ERROR) << "Corrupt entry in user list."; On 2012/11/28 09:57:16, pastarmovj wrote: > Please print the index here too. Could be helpful for some debugging. Done. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:574: if ((existing_users.find(email) != existing_users.end()) || On 2012/11/28 09:57:16, pastarmovj wrote: > I think you don't need the extra parenthesis around a != b. Correct. I added them for readability only. Removed now. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:593: if (!g_browser_process) On 2012/11/28 09:57:16, pastarmovj wrote: > Swap the two ifs and split them with an empty line please. The one is a > precondition and the other is part of the functionality of this proc. Done. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:692: cros_settings_->AddSettingsObserver(kAccountsPrefDeviceLocalAccounts, On 2012/11/28 09:57:16, pastarmovj wrote: > Is there any reason you don't simply do this in the constructor and spare the > bool flag? The RetrieveTrustedDevicePolicies() will always run when policy first becomes available. If I registered as an observer in the constructor, I would also receive a notification that the list of device-local accounts has changed at that point, running RetrieveTrustedDevicePolicies() once more for no good reason. By registering only after RetrieveTrustedDevicePolicies() has successfully run once, I never receive the first notification. I switched away from WeakPtr and as such, no longer need the bool. I simplified the code accordingly. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.cc:779: if (!user) { On 2012/11/28 09:57:16, pastarmovj wrote: > Please also document that this function will actually create a new user if it > doesn't exist. I refactored the code so that this function no longer creates users. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... File chrome/browser/chromeos/login/user_manager_impl.h (right): https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.h:108: const ListValue& users_list, On 2012/11/28 09:57:16, pastarmovj wrote: > nit: move |users_vector| and |users_set| to be last arguments. It seems that > there is weak common preference to have src,dst argument ordering. Done. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.h:149: // front, as specified by |move_to_front|. On 2012/11/28 09:57:16, pastarmovj wrote: > Can you please put some description why is this needed because it sounds like a > very arbitrary operation from this description. Done. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.h:153: // that data stored outside theit cryptohomes is removed for any users who are On 2012/11/28 09:57:16, pastarmovj wrote: > s/theit/their/ Done. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/login/user_manager_impl.h:168: // |UpdateLocalUsers|. On 2012/11/28 09:57:16, pastarmovj wrote: > Write a justification for having this member mutable please. Actually, there is no justification. This seems to have been modified from within a const method at some point in the past... but it never is anymore. I removed the mutable. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... File chrome/browser/chromeos/settings/cros_settings.cc (right): https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/settings/cros_settings.cc:304: if (observer) { On 2012/11/28 09:57:16, pastarmovj wrote: > Why did you do that? Is this observer not removed properly? As discussed offline: I need UserManager to be a CrosSettings observer. Since UserManager and CrosSettings are both singletons (with undefined shutdown order), I had to handle both shutdown orders: * If CrosSettings goes down first, it must accept the fact that UserManager is still registered as an observer. * If UserManager goes down first, it must unregister itself as an observer (problem: it does not know whether CrosSettings is still alive) or invalidate the observer in some other way (I used WeakPtr for this, necessitating the check here). These points are now moot as I added a UserManager::Shutdown() method that gets called during shutdown and allows the UserManager to unregister cleanly. https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... File chrome/browser/chromeos/settings/cros_settings.h (right): https://chromiumcodereview.appspot.com/11419184/diff/11003/chrome/browser/chr... chrome/browser/chromeos/settings/cros_settings.h:110: typedef ObserverList<content::NotificationObserver, false> On 2012/11/28 09:57:16, pastarmovj wrote: > Why did you do that? Is this observer not removed properly? As discussed offline: I need UserManager to be a CrosSettings observer. Since UserManager and CrosSettings are both singletons (with undefined shutdown order), I had to handle both shutdown orders: * If CrosSettings goes down first, it must accept the fact that UserManager is still registered as an observer (thus the |false| here). * If UserManager goes down first, it must unregister itself as an observer (problem: it does not know whether CrosSettings is still alive) or invalidate the observer in some other way (I used WeakPtr for this). These points are now moot as I added a UserManager::Shutdown() method that gets called during shutdown and allows the UserManager to unregister cleanly.
Hi Nikita, Could you review the chrome_browser_main_chromeos.cc change?
On 2012/11/28 12:31:38, bartfab wrote: > Hi Nikita, > > Could you review the chrome_browser_main_chromeos.cc change? It appears that the browser test hangs I noticed earlier are not related to this CL. They are some general flakiness that I get with ToT as well. Thus, the CL is fully ready for review.
lgtm
I've executed cros_ trybots.
On 2012/11/28 18:27:38, Nikita Kostylev wrote: > I've executed cros_ trybots. cros_amd64 flaked out. Trying that one again.
https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (left): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:211: Why are you removing this bit? https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:256: // Make sure we persist new user data to Local State. And this one. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:49: const char kRegularUsers[] = "LoggedInUsers"; I'm very concerned about splitting users into several lists. Not sure whether we'll end up showing local users always first or mixed with regular, but we still should keep an LRU list of *all* users to prevent future migrations forth and back depending on the UI decisions. So I suggest either: (*) keeping *all* users in LoggedInUsers and only local users in LocalUsers or (*) keeping 3 lists (LoggedInUsers, GaiaUsers, LocalUsers or smth like that). https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:563: bool UserManagerImpl::ParseUserList( Since this is a utility method that has nothing to do with UserManager's state, better make it a local function in namespace {} https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:643: users_.push_back(User::CreatePublicAccountUser(*it)); Maybe CreateGaiaAccountUser? Public sounds somewhat counterintuitive. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:759: User *UserManagerImpl::RemoveUserFromListInternal(const std::string& email) { This looks like it really is RemoveRegularUser...? https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.h (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.h:114: void EnsureUsersLoaded() const; const here doesn't look right. In GetUsers() it's OK, since it's hiding the lazy loading, but here it implies that EnsureUsersLoaded actually doesn't do any state change. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.h:155: bool UpdateLocalUsers(const base::ListValue& local_users_list); Please rename to indicate that pending removal is done here. Maybe something like UpdateAndCleanupLocalUsers? https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:124: wallpaper_manager->SetUserWallpaperInfo(UserManager::kStubUser, info, true); Why do you need the kTestUser1 -> kStubUser replacement? Now it looks like we're testing some special users, not "just a test user" https://codereview.chromium.org/11419184/diff/14005/chrome/browser/ui/webui/c... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/ui/webui/c... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:71: const char kKeyPublicAccount[] = "publicAccount"; Like I said before, "public" sounds strange so maybe better "gaia"
Sorry, I've realised that public accounts are not gaia accounts but a different thing. Please disregard my comments about their naming :)
https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (left): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:211: On 2012/11/28 21:40:53, Ivan Korotkov wrote: > Why are you removing this bit? To give more context: this call ensures that if sign out is done exactly after new user sign in we would not lost that user from login screen. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:256: // Make sure we persist new user data to Local State. On 2012/11/28 21:40:53, Ivan Korotkov wrote: > And this one. Same here: ensures that new user avatar is saved immediately. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:49: const char kRegularUsers[] = "LoggedInUsers"; On 2012/11/28 21:40:53, Ivan Korotkov wrote: > I'm very concerned about splitting users into several lists. > Not sure whether we'll end up showing local users always first or mixed with > regular, but we still should keep an LRU list of *all* users to prevent future > migrations forth and back depending on the UI decisions. > > So I suggest either: > (*) keeping *all* users in LoggedInUsers and only local users in LocalUsers or > (*) keeping 3 lists (LoggedInUsers, GaiaUsers, LocalUsers or smth like that). I agree with Ivan. We should have single LoggedInUsers that has users in LRU. Do you intend to place public accounts in some specific way i.e. always first one / always last one? https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:183: if (IsEphemeralUser(email)) { Public accounts will be handled here, right? If that's the case I could not figure out how is_current_user_ephemeral_ is set for them because IsEphemeralUser relied on that value.
https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:49: const char kRegularUsers[] = "LoggedInUsers"; On 2012/11/29 12:00:50, Nikita Kostylev wrote: > On 2012/11/28 21:40:53, Ivan Korotkov wrote: > > I'm very concerned about splitting users into several lists. > > Not sure whether we'll end up showing local users always first or mixed with > > regular, but we still should keep an LRU list of *all* users to prevent future > > migrations forth and back depending on the UI decisions. > > > > So I suggest either: > > (*) keeping *all* users in LoggedInUsers and only local users in LocalUsers or > > (*) keeping 3 lists (LoggedInUsers, GaiaUsers, LocalUsers or smth like that). > > I agree with Ivan. We should have single LoggedInUsers that has users in LRU. > > Do you intend to place public accounts in some specific way i.e. always first > one / always last one? As discussed over chat, this CL actually adds support for public accounts, which have fixed display order and don't need to be put into LRU list. So it's OK to have kPublicAccounts (fixed order) and a mixed kSomethingUsers (LRU order) with all other user types (gaia, local with cryptohome).
https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (left): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:211: On 2012/11/29 12:00:50, Nikita Kostylev wrote: > On 2012/11/28 21:40:53, Ivan Korotkov wrote: > > Why are you removing this bit? > > To give more context: this call ensures that if sign out is done exactly after > new user sign in we would not lost that user from login screen. I removed this at Julian's request. The local state is always flushed to disk on browser shutdown, even if the user logs out straight after logging in (see ShutdownPreThreadsStop() in browser_shutdown.cc). https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:256: // Make sure we persist new user data to Local State. On 2012/11/29 12:00:50, Nikita Kostylev wrote: > On 2012/11/28 21:40:53, Ivan Korotkov wrote: > > And this one. > > Same here: ensures that new user avatar is saved immediately. Same as above. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:49: const char kRegularUsers[] = "LoggedInUsers"; On 2012/11/29 12:06:31, Ivan Korotkov wrote: > On 2012/11/29 12:00:50, Nikita Kostylev wrote: > > On 2012/11/28 21:40:53, Ivan Korotkov wrote: > > > I'm very concerned about splitting users into several lists. > > > Not sure whether we'll end up showing local users always first or mixed with > > > regular, but we still should keep an LRU list of *all* users to prevent > future > > > migrations forth and back depending on the UI decisions. > > > > > > So I suggest either: > > > (*) keeping *all* users in LoggedInUsers and only local users in LocalUsers > or > > > (*) keeping 3 lists (LoggedInUsers, GaiaUsers, LocalUsers or smth like > that). > > > > I agree with Ivan. We should have single LoggedInUsers that has users in LRU. > > > > Do you intend to place public accounts in some specific way i.e. always first > > one / always last one? > > As discussed over chat, this CL actually adds support for public accounts, which > have fixed display order and don't need to be put into LRU list. > > So it's OK to have kPublicAccounts (fixed order) and a mixed kSomethingUsers > (LRU order) with all other user types (gaia, local with cryptohome). I renamed |LocalUsers| to |PublicAccounts|. I left |RegularUsers| as-is for now. Once local accounts are added, we can rethink the name - I cannot think of anything better that signifies "GAIA plus local, persistent" right now. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:183: if (IsEphemeralUser(email)) { On 2012/11/29 12:00:50, Nikita Kostylev wrote: > Public accounts will be handled here, right? > > If that's the case I could not figure out how is_current_user_ephemeral_ is set > for them because IsEphemeralUser relied on that value. Public accounts will be treated separately because e.g. their user policy works slightly differently. I will handle the login part in a separate CL. Right now, clicking on a public account pod does not let you log in at all. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:563: bool UserManagerImpl::ParseUserList( On 2012/11/28 21:40:53, Ivan Korotkov wrote: > Since this is a utility method that has nothing to do with UserManager's state, > better make it a local function in namespace {} Done. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:643: users_.push_back(User::CreatePublicAccountUser(*it)); On 2012/11/28 21:40:53, Ivan Korotkov wrote: > Maybe CreateGaiaAccountUser? Public sounds somewhat counterintuitive. As discussed offline, public accounts are non-GAIA. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:759: User *UserManagerImpl::RemoveUserFromListInternal(const std::string& email) { On 2012/11/28 21:40:53, Ivan Korotkov wrote: > This looks like it really is RemoveRegularUser...? Done. Though note that the method will handle other user types properly - but you are right, it is intended to be used with regular users. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.h (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.h:114: void EnsureUsersLoaded() const; On 2012/11/28 21:40:53, Ivan Korotkov wrote: > const here doesn't look right. In GetUsers() it's OK, since it's hiding the lazy > loading, but here it implies that EnsureUsersLoaded actually doesn't do any > state change. This was requested by Julian in his review. I agree that the "const" implies something that the function does not keep. But using |const_cast| is even uglier and more dangerous. If you prefer the |const_cast|, I can undo this change. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.h:155: bool UpdateLocalUsers(const base::ListValue& local_users_list); On 2012/11/28 21:40:53, Ivan Korotkov wrote: > Please rename to indicate that pending removal is done here. Maybe something > like UpdateAndCleanupLocalUsers? Done. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:124: wallpaper_manager->SetUserWallpaperInfo(UserManager::kStubUser, info, true); On 2012/11/28 21:40:53, Ivan Korotkov wrote: > Why do you need the kTestUser1 -> kStubUser replacement? Now it looks like we're > testing some special users, not "just a test user" The test relied on a bug in UserManager: EnsureUsersLoaded() should run only once and then never again. Instead, it would run repeatedly as long as there were no users. The test exploited this by adding a user to the |LoggedInUsers| pref at run-time and having EnsureUsersLoaded() pick it up from there. I fixed the bug in EnsureUsersLoaded(). You can no longer inject a user at runtime by modifying the pref. This leaves the test with two options: 1/ Use a user that exists already. The stub user fits this bill. 2/ Add the user to the |LoggedInUsers| pref in a PRE_ method. The |UserImageManagerTest| class uses this technique. If you prefer, I can switch the test from 1/ to 2/. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/ui/webui/c... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/ui/webui/c... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:71: const char kKeyPublicAccount[] = "publicAccount"; On 2012/11/28 21:40:53, Ivan Korotkov wrote: > Like I said before, "public" sounds strange so maybe better "gaia" As discussed, leaving as-is after clarifying that public != GAIA.
unit_tests and interactive_ui_tests passed locally. I will run some trybots to make sure they are all happy.
https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:783: std::string logged_in_user; Please rename to logged_in_user_email so that you don't confuse with logged_in_user_
LGTM but please leave the CommitPendingWrite lines in UserManager https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (left): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:211: On 2012/11/29 14:18:09, bartfab wrote: > On 2012/11/29 12:00:50, Nikita Kostylev wrote: > > On 2012/11/28 21:40:53, Ivan Korotkov wrote: > > > Why are you removing this bit? > > > > To give more context: this call ensures that if sign out is done exactly after > > new user sign in we would not lost that user from login screen. > > I removed this at Julian's request. The local state is always flushed to disk on > browser shutdown, even if the user logs out straight after logging in (see > ShutdownPreThreadsStop() in browser_shutdown.cc). Right, but if browser crashes in the first few seconds after session start, user will be not saved. That's the reason this line was added in the first place. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.h (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.h:114: void EnsureUsersLoaded() const; On 2012/11/29 14:18:09, bartfab wrote: > On 2012/11/28 21:40:53, Ivan Korotkov wrote: > > const here doesn't look right. In GetUsers() it's OK, since it's hiding the > lazy > > loading, but here it implies that EnsureUsersLoaded actually doesn't do any > > state change. > > This was requested by Julian in his review. I agree that the "const" implies > something that the function does not keep. But using |const_cast| is even uglier > and more dangerous. If you prefer the |const_cast|, I can undo this change. Yes, I still think it's better to leave it non-const to be explicit. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/wallpaper_manager_browsertest.cc:124: wallpaper_manager->SetUserWallpaperInfo(UserManager::kStubUser, info, true); On 2012/11/29 14:18:09, bartfab wrote: > On 2012/11/28 21:40:53, Ivan Korotkov wrote: > > Why do you need the kTestUser1 -> kStubUser replacement? Now it looks like > we're > > testing some special users, not "just a test user" > > The test relied on a bug in UserManager: EnsureUsersLoaded() should run only > once and then never again. Instead, it would run repeatedly as long as there > were no users. The test exploited this by adding a user to the |LoggedInUsers| > pref at run-time and having EnsureUsersLoaded() pick it up from there. > > I fixed the bug in EnsureUsersLoaded(). You can no longer inject a user at > runtime by modifying the pref. This leaves the test with two options: > 1/ Use a user that exists already. The stub user fits this bill. > 2/ Add the user to the |LoggedInUsers| pref in a PRE_ method. The > |UserImageManagerTest| class uses this technique. > > If you prefer, I can switch the test from 1/ to 2/. Ah, I see. It's ok then.
https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (left): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:211: On 2012/11/29 17:25:33, Ivan Korotkov wrote: > On 2012/11/29 14:18:09, bartfab wrote: > > On 2012/11/29 12:00:50, Nikita Kostylev wrote: > > > On 2012/11/28 21:40:53, Ivan Korotkov wrote: > > > > Why are you removing this bit? > > > > > > To give more context: this call ensures that if sign out is done exactly > after > > > new user sign in we would not lost that user from login screen. > > > > I removed this at Julian's request. The local state is always flushed to disk > on > > browser shutdown, even if the user logs out straight after logging in (see > > ShutdownPreThreadsStop() in browser_shutdown.cc). > > Right, but if browser crashes in the first few seconds after session start, user > will be not saved. That's the reason this line was added in the first place. Done. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:256: // Make sure we persist new user data to Local State. On 2012/11/29 14:18:09, bartfab wrote: > On 2012/11/29 12:00:50, Nikita Kostylev wrote: > > On 2012/11/28 21:40:53, Ivan Korotkov wrote: > > > And this one. > > > > Same here: ensures that new user avatar is saved immediately. > > Same as above. Done. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.cc:783: std::string logged_in_user; On 2012/11/29 16:42:50, Nikita Kostylev wrote: > Please rename to logged_in_user_email so that you don't confuse with > logged_in_user_ Done. https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... File chrome/browser/chromeos/login/user_manager_impl.h (right): https://codereview.chromium.org/11419184/diff/14005/chrome/browser/chromeos/l... chrome/browser/chromeos/login/user_manager_impl.h:114: void EnsureUsersLoaded() const; On 2012/11/29 17:25:33, Ivan Korotkov wrote: > On 2012/11/29 14:18:09, bartfab wrote: > > On 2012/11/28 21:40:53, Ivan Korotkov wrote: > > > const here doesn't look right. In GetUsers() it's OK, since it's hiding the > > lazy > > > loading, but here it implies that EnsureUsersLoaded actually doesn't do any > > > state change. > > > > This was requested by Julian in his review. I agree that the "const" implies > > something that the function does not keep. But using |const_cast| is even > uglier > > and more dangerous. If you prefer the |const_cast|, I can undo this change. > > Yes, I still think it's better to leave it non-const to be explicit. Done.
Nikita, ping, I still need your approval before I can land this.
lgtm https://codereview.chromium.org/11419184/diff/4026/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/user_manager_impl.h (right): https://codereview.chromium.org/11419184/diff/4026/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/user_manager_impl.h:141: // Replaces the list of public accounts with |public_accounts|. Ensures that Could you please add more comments that describe in what cases this function is used and why.
https://codereview.chromium.org/11419184/diff/4026/chrome/browser/chromeos/lo... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://codereview.chromium.org/11419184/diff/4026/chrome/browser/chromeos/lo... chrome/browser/chromeos/login/user_manager_impl.cc:128: LOG(ERROR) << "Corrupt entry in user list at index " << i << "."; Wouldn't that be the case for public accounts or they will always some kind of email defined?
https://chromiumcodereview.appspot.com/11419184/diff/4026/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager_impl.cc (right): https://chromiumcodereview.appspot.com/11419184/diff/4026/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_impl.cc:128: LOG(ERROR) << "Corrupt entry in user list at index " << i << "."; On 2012/11/30 14:32:57, Nikita Kostylev wrote: > Wouldn't that be the case for public accounts or they will always some kind of > email defined? Public accounts are defined via (fake) e-mail addresses. So when this function is fed a list of public accounts, it will happily parse them. It will only report errors if the list of public accounts is corrupt or there are duplicates (including a public account whose fake e-mail collides with an existing GAIA account). https://chromiumcodereview.appspot.com/11419184/diff/4026/chrome/browser/chro... File chrome/browser/chromeos/login/user_manager_impl.h (right): https://chromiumcodereview.appspot.com/11419184/diff/4026/chrome/browser/chro... chrome/browser/chromeos/login/user_manager_impl.h:141: // Replaces the list of public accounts with |public_accounts|. Ensures that On 2012/11/30 14:20:02, Nikita Kostylev wrote: > Could you please add more comments that describe in what cases this function is > used and why. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bartfab@chromium.org/11419184/12026
Message was sent while issue was closed.
Change committed as 170507 |
