Clarify settings UI in multi-profiles mode

chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc

Index: chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
diff --git a/chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc b/chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
index 0dc8c6110dae4579fa14b1cd66cacbc7b63baf8f..72011ff8ec8f83411b6c8f573e63a827c7cca6e8 100644
--- a/chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
+++ b/chrome/browser/ui/webui/options/chromeos/core_chromeos_options_handler.cc
@@ -6,11 +6,14 @@
 
 #include <string>
 
+#include "ash/session_state_delegate.h"
+#include "ash/shell.h"
 #include "base/bind.h"
 #include "base/prefs/pref_change_registrar.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/sys_info.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
@@ -44,9 +47,7 @@ bool IsSettingOwnerOnly(const std::string& pref) {
 
 // Returns true if |username| is the logged-in owner.
 bool IsLoggedInOwner(const std::string& username) {
-  UserManager* user_manager = UserManager::Get();
-  return user_manager->IsCurrentUserOwner() &&
-      user_manager->GetLoggedInUser()->email() == username;
+  return UserManager::Get()->GetOwnerEmail() == username;

[stevenjb, 2014/02/07 00:40:09]

Double check with Stefan that this is the best way to do this. (It looks fine to
me, but I know there have been a lot of related changes recently, would be good
to not use a method that will be deprecated soon).

[michaelpg, 2014/02/07 02:04:27]

I will double-check, but I think this is solid. The point is to replace
IsCurrentUserOwner, which doesn't do what it sounds like it does and will be
removed soon.

 }
 
 // Creates a user info dictionary to be stored in the |ListValue| that is
@@ -133,6 +134,7 @@ base::Value* CoreChromeOSOptionsHandler::FetchPref(
 
     return value;
   }
+
   if (!CrosSettings::IsCrosSettings(pref_name)) {
     std::string controlling_pref =
         pref_name == prefs::kUseSharedProxies ? prefs::kProxy : std::string();
@@ -152,13 +154,16 @@ base::Value* CoreChromeOSOptionsHandler::FetchPref(
     dict->Set("value", pref_value->DeepCopy());
   policy::BrowserPolicyConnectorChromeOS* connector =
       g_browser_process->platform_part()->browser_policy_connector_chromeos();
-  if (connector->IsEnterpriseManaged())
+  if (connector->IsEnterpriseManaged()) {
+    dict->SetBoolean("disabled", true);
     dict->SetString("controlledBy", "policy");
-  bool disabled_by_owner = IsSettingOwnerOnly(pref_name) &&
-      !ProfileHelper::IsOwnerProfile(Profile::FromWebUI(web_ui()));
-  dict->SetBoolean("disabled", disabled_by_owner);
-  if (disabled_by_owner)
-    dict->SetString("controlledBy", "owner");
+  } else {
+    bool disabled_by_owner = IsSettingOwnerOnly(pref_name) &&
+        !ProfileHelper::IsOwnerProfile(Profile::FromWebUI(web_ui()));

[stevenjb, 2014/02/07 00:40:09]

Should this be 'controlled_by_owner'? It looks like any pref controlled by the
owner is implicitly disabled, and this just checks whether it is controlled?

[michaelpg, 2014/02/07 02:04:27]

That's a better name. Changed

+    dict->SetBoolean("disabled", disabled_by_owner);
+    if (disabled_by_owner)
+      dict->SetString("controlledBy", "owner");
+  }
   return dict;
 }
 
@@ -209,15 +214,98 @@ void CoreChromeOSOptionsHandler::StopObservingPref(const std::string& path) {
     ::options::CoreOptionsHandler::StopObservingPref(path);
 }
 
+base::Value* CoreChromeOSOptionsHandler::CreateValueForPref(
+    const std::string& pref_name,
+    const std::string& controlling_pref_name) {
+  // The screen lock setting is shared if multiple users are logged in and at
+  // least one has chosen to require passwords.
+  if (UserManager::Get()->GetLoggedInUsers().size() > 1 &&
+      controlling_pref_name.empty() &&
+      pref_name == prefs::kEnableAutoScreenLock) {

[stevenjb, 2014/02/07 00:40:09]

nit: Check pref_name first

[michaelpg, 2014/02/07 02:04:27]

Done.

+    PrefService* user_prefs = Profile::FromWebUI(web_ui())->GetPrefs();
+    const PrefService::Preference* pref =
+        user_prefs->FindPreference(prefs::kEnableAutoScreenLock);
+
+    ash::SessionStateDelegate* delegate =
+        ash::Shell::GetInstance()->session_state_delegate();
+    if (pref && pref->IsUserModifiable() &&
+        delegate->ShouldLockScreenBeforeSuspending()) {
+      bool screen_lock = false;
+      pref->GetValue()->GetAsBoolean(&screen_lock);
+      if (!screen_lock) {
+        // Screen lock is enabled, but user's pref is disabled. Show the user's

[stevenjb, 2014/02/07 00:40:09]

user's pref is disabled, or set to 'false' or either?

[michaelpg, 2014/02/07 02:04:27]

set to false.

+        // value in the checkbox, but indicate that the setting is controlled.
+        base::DictionaryValue* dict = new base::DictionaryValue;
+        dict->Set("value", pref->GetValue()->DeepCopy());
+        dict->SetString("controlledBy", "shared");
+        return dict;
+      }
+    }
+  }
+
+  return CoreOptionsHandler::CreateValueForPref(pref_name,
+                                                controlling_pref_name);
+}
+
 void CoreChromeOSOptionsHandler::GetLocalizedValues(
     base::DictionaryValue* localized_strings) {
   DCHECK(localized_strings);
   CoreOptionsHandler::GetLocalizedValues(localized_strings);
 
-  AddAccountUITweaksLocalizedValues(localized_strings,
-                                    Profile::FromWebUI(web_ui()));
-  localized_strings->SetString("controlledSettingOwner",
-      l10n_util::GetStringUTF16(IDS_OPTIONS_CONTROLLED_SETTING_OWNER));
+  Profile* profile = Profile::FromWebUI(web_ui());
+  AddAccountUITweaksLocalizedValues(localized_strings, profile);
+
+  UserManager* user_manager = UserManager::Get();
+  const User* primary_user = user_manager->GetPrimaryUser();
+
+

[stevenjb, 2014/02/07 00:40:09]

one blank line

[michaelpg, 2014/02/07 02:04:27]

Done.

+  // Check at load time whether this is a secondary user in a multi-profile
+  // session.
+  if (user_manager->GetUserByProfile(profile) != primary_user) {
+    // Set secondaryUser to show the shared icon by the network section header.
+    localized_strings->SetBoolean("secondaryUser", true);
+    localized_strings->SetString("secondaryUserBannerText",
+        l10n_util::GetStringFUTF16(
+            IDS_OPTIONS_SETTINGS_SECONDARY_USER_BANNER,
+            base::ASCIIToUTF16(primary_user->email()),
+            primary_user->GetDisplayName()));
+    localized_strings->SetString("controlledSettingShared",
+        l10n_util::GetStringFUTF16(
+            IDS_OPTIONS_CONTROLLED_SETTING_SHARED_MULTIPROFILES,
+            base::ASCIIToUTF16(primary_user->email())));
+  } else {
+    localized_strings->SetBoolean("secondaryUser", false);
+    localized_strings->SetString("secondaryUserBannerText", base::string16());
+    localized_strings->SetString("controlledSettingShared", base::string16());
+  }
+
+  // Screen lock icon can show up as primary or secondary user.
+  localized_strings->SetString("screenLockShared",
+      l10n_util::GetStringUTF16(
+          IDS_OPTIONS_CONTROLLED_SETTING_SHARED_SCREEN_LOCK));
+
+  policy::BrowserPolicyConnectorChromeOS* connector =
+      g_browser_process->platform_part()->browser_policy_connector_chromeos();
+  if (connector->IsEnterpriseManaged()) {
+    // Managed machines have no "owner".
+    localized_strings->SetString("controlledSettingOwner", base::string16());
+  } else if (UserManager::Get()->GetLoggedInUsers().size() > 1) {
+    // For multi-profiles case, show owner email.
+    std::string owner_email = UserManager::Get()->GetOwnerEmail();
+    // Set fake owner email for testing.

[stevenjb, 2014/02/07 00:40:09]

If we want to check this in we should move this logic to UserManager or a
wrapper that we use consistently (and should probably put it behind a flag like
--enable-stub-interactive so that it doesn't affect tests)

[michaelpg, 2014/02/07 02:04:27]

I'll check with Stefan and Dmitry. I'm only including this here so the string
looks nicer; I'm wary of breaking other functionality that would expect a real
owner to exist if the owner email is set.

+    if (!base::SysInfo::IsRunningOnChromeOS() && owner_email.empty()) {
+      owner_email = "test@test.test";
+    }
+
+    localized_strings->SetString("controlledSettingOwner",
+        l10n_util::GetStringFUTF16(
+            IDS_OPTIONS_CONTROLLED_SETTING_OWNER_MULTIPROFILES,
+            base::ASCIIToUTF16(owner_email)));
+  } else {
+    localized_strings->SetString("controlledSettingOwner",
+        l10n_util::GetStringUTF16(
+            IDS_OPTIONS_CONTROLLED_SETTING_OWNER));
+  }
 }
 
 void CoreChromeOSOptionsHandler::SelectNetworkCallback(