Providing more information on why certain users can't be added to multi-profile

chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc

Index: chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc
diff --git a/chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc b/chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc
index 5f3258f7f72607924e0161008bbfc5fd572fb00e..fb1c3752d2f970088b34a2cf9876e1e4d529e3e5 100644
--- a/chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc
+++ b/chrome/browser/chromeos/login/users/multi_profile_user_controller_unittest.cc
@@ -35,54 +35,63 @@ struct BehaviorTestCase {
   const char* primary;
   const char* secondary;
   MultiProfileUserController::UserAllowedInSessionReason expected_allowed;
+  MultiProfileUserController::UserAllowedInSessionReason

[James Cook, 2014/08/12 20:08:30]

I would reorder these two member variables. Then the test cases read like this:

primary behavior,
secondary behavior,
was it allowed initially?
was it allowed in the end?

Otherwise you get weird test data like line 74, which reads like "it was blocked
but then it was re-allowed".

[Roman Sorokin (ftl), 2014/08/13 09:11:52]

Done.

+      expected_primary_policy;
 };
 
 const BehaviorTestCase kBehaviorTestCases[] = {
-  {
-    MultiProfileUserController::kBehaviorUnrestricted,
-    MultiProfileUserController::kBehaviorUnrestricted,
-    MultiProfileUserController::ALLOWED,
-  },
-  {
-    MultiProfileUserController::kBehaviorUnrestricted,
-    MultiProfileUserController::kBehaviorPrimaryOnly,
-    MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
-  },
-  {
-    MultiProfileUserController::kBehaviorUnrestricted,
-    MultiProfileUserController::kBehaviorNotAllowed,
-    MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
-  },
-  {
-    MultiProfileUserController::kBehaviorPrimaryOnly,
-    MultiProfileUserController::kBehaviorUnrestricted,
-    MultiProfileUserController::ALLOWED,
-  },
-  {
-    MultiProfileUserController::kBehaviorPrimaryOnly,
-    MultiProfileUserController::kBehaviorPrimaryOnly,
-    MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
-  },
-  {
-    MultiProfileUserController::kBehaviorPrimaryOnly,
-    MultiProfileUserController::kBehaviorNotAllowed,
-    MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
-  },
-  {
-    MultiProfileUserController::kBehaviorNotAllowed,
-    MultiProfileUserController::kBehaviorUnrestricted,
-    MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
-  },
-  {
-    MultiProfileUserController::kBehaviorNotAllowed,
-    MultiProfileUserController::kBehaviorPrimaryOnly,
-    MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
-  },
-  {
-    MultiProfileUserController::kBehaviorNotAllowed,
-    MultiProfileUserController::kBehaviorNotAllowed,
-    MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
-  },
+    {
+     MultiProfileUserController::kBehaviorUnrestricted,

[James Cook, 2014/08/12 20:08:29]

Is this the indentation "git cl format" uses? It looks odd, but if that's what
the auto-formatter does then use it. If not, please run it through the
formatter.

[Roman Sorokin (ftl), 2014/08/13 09:11:52]

Yes, it is auto-formatter
On 2014/08/12 20:08:29, James Cook wrote:

+     MultiProfileUserController::kBehaviorUnrestricted,
+     MultiProfileUserController::ALLOWED, MultiProfileUserController::ALLOWED,
+    },
+    {
+     MultiProfileUserController::kBehaviorUnrestricted,
+     MultiProfileUserController::kBehaviorPrimaryOnly,
+     MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
+     MultiProfileUserController::ALLOWED,
+    },
+    {
+     MultiProfileUserController::kBehaviorUnrestricted,
+     MultiProfileUserController::kBehaviorNotAllowed,
+     MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
+     MultiProfileUserController::ALLOWED,
+    },
+    {
+     MultiProfileUserController::kBehaviorPrimaryOnly,
+     MultiProfileUserController::kBehaviorUnrestricted,
+     MultiProfileUserController::ALLOWED, MultiProfileUserController::ALLOWED,
+    },
+    {
+     MultiProfileUserController::kBehaviorPrimaryOnly,
+     MultiProfileUserController::kBehaviorPrimaryOnly,
+     MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
+     MultiProfileUserController::ALLOWED,
+    },
+    {
+     MultiProfileUserController::kBehaviorPrimaryOnly,
+     MultiProfileUserController::kBehaviorNotAllowed,
+     MultiProfileUserController::NOT_ALLOWED_POLICY_FORBIDS,
+     MultiProfileUserController::ALLOWED,
+    },
+    {
+     MultiProfileUserController::kBehaviorNotAllowed,
+     MultiProfileUserController::kBehaviorUnrestricted,
+     MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
+     MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
+    },
+    {
+     MultiProfileUserController::kBehaviorNotAllowed,
+     MultiProfileUserController::kBehaviorPrimaryOnly,
+     MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
+     MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
+    },
+    {
+     MultiProfileUserController::kBehaviorNotAllowed,
+     MultiProfileUserController::kBehaviorNotAllowed,
+     MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
+     MultiProfileUserController::NOT_ALLOWED_PRIMARY_USER_POLICY_FORBIDS,
+    },
 };
 
 // Weak ptr to PolicyCertVerifier - object is freed in test destructor once
@@ -216,6 +225,9 @@ TEST_F(MultiProfileUserControllerTest, AllAllowedBeforeLogin) {
     EXPECT_TRUE(controller()->IsUserAllowedInSession(kUsers[0], &reason))
         << "Case " << i;
     EXPECT_EQ(MultiProfileUserController::ALLOWED, reason) << "Case " << i;
+    EXPECT_EQ(MultiProfileUserController::ALLOWED,

[James Cook, 2014/08/12 20:08:29]

Likewise I would reorder all these, so you first test the primary policy, then
the final decision.

[Roman Sorokin (ftl), 2014/08/13 09:11:52]

Done.

+              MultiProfileUserController::GetPrimaryUserPolicy())
+        << "Case " << i;
   }
 }
 
@@ -275,6 +287,9 @@ TEST_F(MultiProfileUserControllerTest, IsSecondaryAllowed) {
     MultiProfileUserController::UserAllowedInSessionReason reason;
     controller()->IsUserAllowedInSession(kUsers[1], &reason);
     EXPECT_EQ(kBehaviorTestCases[i].expected_allowed, reason) << "Case " << i;
+    EXPECT_EQ(kBehaviorTestCases[i].expected_primary_policy,
+              MultiProfileUserController::GetPrimaryUserPolicy())
+        << "Case " << i;
   }
 }
 
@@ -324,6 +339,8 @@ TEST_F(MultiProfileUserControllerTest,
   EXPECT_EQ(MultiProfileUserController::ALLOWED, reason);
   EXPECT_TRUE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
   EXPECT_EQ(MultiProfileUserController::ALLOWED, reason);
+  EXPECT_EQ(MultiProfileUserController::ALLOWED,
+            MultiProfileUserController::GetPrimaryUserPolicy());
 }
 
 TEST_F(MultiProfileUserControllerTest,
@@ -363,10 +380,14 @@ TEST_F(MultiProfileUserControllerTest,
   EXPECT_FALSE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
   EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
             reason);
+  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
+            MultiProfileUserController::GetPrimaryUserPolicy());
   policy::PolicyCertServiceFactory::SetUsedPolicyCertificates(kUsers[1]);
   EXPECT_FALSE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
   EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_POLICY_CERT_TAINTED,
             reason);
+  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
+            MultiProfileUserController::GetPrimaryUserPolicy());
 
   // Flush tasks posted to IO.
   base::RunLoop().RunUntilIdle();
@@ -395,6 +416,8 @@ TEST_F(MultiProfileUserControllerTest,
   MultiProfileUserController::UserAllowedInSessionReason reason;
   EXPECT_TRUE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
   EXPECT_EQ(MultiProfileUserController::ALLOWED, reason);
+  EXPECT_EQ(MultiProfileUserController::ALLOWED,
+            MultiProfileUserController::GetPrimaryUserPolicy());
 
   net::CertificateList certificates;
   certificates.push_back(new net::X509Certificate(
@@ -404,6 +427,8 @@ TEST_F(MultiProfileUserControllerTest,
   EXPECT_FALSE(controller()->IsUserAllowedInSession(kUsers[1], &reason));
   EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
             reason);
+  EXPECT_EQ(MultiProfileUserController::NOT_ALLOWED_PRIMARY_POLICY_CERT_TAINTED,
+            MultiProfileUserController::GetPrimaryUserPolicy());
 
   // Flush tasks posted to IO.
   base::RunLoop().RunUntilIdle();