cros: Defer primary user meta update until active

chrome/browser/ui/ash/session_controller_client.cc

Index: chrome/browser/ui/ash/session_controller_client.cc
diff --git a/chrome/browser/ui/ash/session_controller_client.cc b/chrome/browser/ui/ash/session_controller_client.cc
index 905fb906afdde41c96d460e67ec2af1c98604a01..7ca59164f109ff86ba648dae018c90c74e10c714 100644
--- a/chrome/browser/ui/ash/session_controller_client.cc
+++ b/chrome/browser/ui/ash/session_controller_client.cc
@@ -38,6 +38,7 @@
 
 using session_manager::Session;
 using session_manager::SessionManager;
+using session_manager::SessionState;
 using user_manager::UserManager;
 using user_manager::User;
 using user_manager::UserList;
@@ -188,12 +189,18 @@ void SessionControllerClient::CycleActiveUser(
 void SessionControllerClient::ActiveUserChanged(const User* active_user) {
   SendSessionInfoIfChanged();
 
-  // UserAddedToSession is not called for the primary user session so send its
-  // meta data here once.
+  // UserAddedToSession is not called for the primary user session so its meta

[James Cook, 2017/05/08 16:23:02]

It would be very nice if it was called. Hopefully that is part of the work for
crbug.com/657149

The state changes in this class are very confusing right now.

[xiyuan, 2017/05/08 16:46:19]

Agree. I don't know why we send out ActiveUserChanged so early for the primary
user but not for secondary users. Probably because this was a simple replacement
for UserLoggedIn when we introduce multiprofile feature. This makes the
dependent code confusing and difficult to get right. The refactor should address
this.

+  // data here needs to be sent to ash before setting user session order.
+  // However, ActiveUserChanged happens at different timing for primary user
+  // and secondary users. For primary user, it happens before user profile load.
+  // For secondary users, it happens after user profile load. This caused
+  // confusing down the path. Bail out here to defer the primary user session
+  // meta sent until it becomes active so that ash side could expect a

[James Cook, 2017/05/08 16:23:02]

super nit: "meta" -> "metadata"?

[xiyuan, 2017/05/08 16:46:20]

Done.

+  // consistent state.
+  // TODO(xiyuan): Get rid of this after http://crbug.com/657149 refactoring.
   if (!primary_user_session_sent_ &&
       UserManager::Get()->GetPrimaryUser() == active_user) {
-    primary_user_session_sent_ = true;
-    SendUserSession(*active_user);
+    return;
   }
 
   SendUserSessionOrder();
@@ -315,6 +322,17 @@ void SessionControllerClient::FlushForTesting() {
 }
 
 void SessionControllerClient::OnSessionStateChanged() {
+  // Sent the primary user meta and order that are deferred from

[James Cook, 2017/05/08 16:23:02]

super nit: "meta" -> "metadata"?

[xiyuan, 2017/05/08 16:46:19]

Done.

+  // ActiveUserChanged before update session state.
+  if (!primary_user_session_sent_ &&
+      SessionManager::Get()->session_state() == SessionState::ACTIVE) {
+    DCHECK_EQ(UserManager::Get()->GetPrimaryUser(),
+              UserManager::Get()->GetActiveUser());
+    primary_user_session_sent_ = true;
+    SendUserSession(*UserManager::Get()->GetPrimaryUser());
+    SendUserSessionOrder();
+  }
+
   SendSessionInfoIfChanged();
 }