cros login: Use default hour type in case use does not have the pref.

Fixes the issue when the user does not have the stored hour clock type
preference - it uses the previous user preference.

Bug: b/168759134
Change-Id: I415b85456dd578ac0565b42c819ef9d0b3590d94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2443009Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Auto-Submit: Roman Sorokin [CET] <rsorokin@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816633}

chrome/browser/chromeos/login/screens/user_selection_screen.cc

@@ -828,18 +828,27 @@ void UserSelectionScreen::HandleFocusPod(const AccountId& account_id) {
   lock_screen_utils::SetKeyboardSettings(account_id);
 
   bool use_24hour_clock = false;
-  if (user_manager::known_user::GetBooleanPref(
+  if (!user_manager::known_user::GetBooleanPref(
           account_id, ::prefs::kUse24HourClock, &use_24hour_clock)) {
-    g_browser_process->platform_part()
-        ->GetSystemClock()
-        ->SetLastFocusedPodHourClockType(use_24hour_clock ? base::k24HourClock
-                                                          : base::k12HourClock);
+    focused_user_clock_type_.reset();
+  } else {
+    base::HourClockType clock_type =
+        use_24hour_clock ? base::k24HourClock : base::k12HourClock;
+    if (focused_user_clock_type_.has_value()) {
+      focused_user_clock_type_->UpdateClockType(clock_type);
+    } else {
+      focused_user_clock_type_ = g_browser_process->platform_part()
+                                     ->GetSystemClock()
+                                     ->CreateScopedHourClockType(clock_type);
+    }
   }
+
   focused_pod_account_id_ = account_id;
 }
 
 void UserSelectionScreen::HandleNoPodFocused() {
   focused_pod_account_id_ = EmptyAccountId();
+  focused_user_clock_type_.reset();
   if (display_type_ == OobeUI::kLoginDisplay)
     lock_screen_utils::EnforceDevicePolicyInputMethods(std::string());
 }

chrome/browser/chromeos/login/screens/user_selection_screen.h

@@ -21,6 +21,7 @@
 #include "chrome/browser/chromeos/login/signin/token_handle_util.h"
 #include "chrome/browser/chromeos/login/ui/login_display.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/system/system_clock.h"
 #include "chromeos/components/proximity_auth/screenlock_bridge.h"
 #include "chromeos/dbus/cryptohome/rpc.pb.h"
 #include "components/account_id/account_id.h"
@@ -185,6 +186,8 @@ class UserSelectionScreen
   user_manager::UserList users_to_send_;
 
   AccountId focused_pod_account_id_;
+  base::Optional<system::SystemClock::ScopedHourClockType>
+      focused_user_clock_type_;
 
   // Sometimes we might get focused pod while user session is still active. e.g.
   // while creating lock screen. So postpone any work until after the session

chrome/browser/chromeos/login/test/local_state_mixin.cc

@@ -9,6 +9,8 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_browser_main.h"
 #include "chrome/browser/chrome_browser_main_extra_parts.h"
+#include "components/user_manager/fake_user_manager.h"
+#include "components/user_manager/scoped_user_manager.h"
 
 namespace chromeos {
 
@@ -21,7 +23,16 @@ class TestMainExtraPart : public ChromeBrowserMainExtraParts {
   ~TestMainExtraPart() override = default;
 
   // ChromeBrowserMainExtraParts:
-  void PostEarlyInitialization() override { delegate_->SetUpLocalState(); }
+  void PostEarlyInitialization() override {
+    // SaveKnownUser depends on UserManager to get the local state that has to
+    // be updated, and do ephemeral user checks.
+    // Given that user manager does not exist yet (by design), create a
+    // temporary fake user manager instance.
+    auto user_manager = std::make_unique<user_manager::FakeUserManager>();
+    user_manager->set_local_state(g_browser_process->local_state());
+    user_manager::ScopedUserManager scoper(std::move(user_manager));
+    delegate_->SetUpLocalState();
+  }
 
  private:
   LocalStateMixin::Delegate* const delegate_;

chrome/browser/chromeos/login/test/login_manager_mixin.cc

@@ -23,9 +23,7 @@
 #include "chromeos/login/auth/user_context.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
-#include "components/user_manager/fake_user_manager.h"
 #include "components/user_manager/known_user.h"
-#include "components/user_manager/scoped_user_manager.h"
 
 namespace chromeos {
 
@@ -108,38 +106,28 @@ bool LoginManagerMixin::SetUpUserDataDirectory() {
 }
 
 void LoginManagerMixin::SetUpLocalState() {
-  // SaveKnownUser depends on UserManager to get the local state that has to
-  // be updated, and do ephemeral user checks.
-  // Given that user manager does not exist yet (by design), create a
-  // temporary fake user manager instance.
-  {
-    auto user_manager = std::make_unique<user_manager::FakeUserManager>();
-    user_manager->set_local_state(g_browser_process->local_state());
-    user_manager::ScopedUserManager scoper(std::move(user_manager));
-    for (const auto& user : initial_users_) {
-      ListPrefUpdate users_pref(g_browser_process->local_state(),
-                                "LoggedInUsers");
-      users_pref->AppendIfNotPresent(
-          std::make_unique<base::Value>(user.account_id.GetUserEmail()));
-
-      DictionaryPrefUpdate user_type_update(g_browser_process->local_state(),
-                                            "UserType");
-      user_type_update->SetKey(user.account_id.GetAccountIdKey(),
-                               base::Value(static_cast<int>(user.user_type)));
-
-      DictionaryPrefUpdate user_token_update(g_browser_process->local_state(),
-                                             "OAuthTokenStatus");
-      user_token_update->SetKey(
-          user.account_id.GetUserEmail(),
-          base::Value(static_cast<int>(user.token_status)));
-
-      user_manager::known_user::UpdateId(user.account_id);
-
-      if (user.user_type == user_manager::USER_TYPE_CHILD) {
-        user_manager::known_user::SetProfileRequiresPolicy(
-            user.account_id,
-            user_manager::known_user::ProfileRequiresPolicy::kPolicyRequired);
-      }
+  for (const auto& user : initial_users_) {
+    ListPrefUpdate users_pref(g_browser_process->local_state(),
+                              "LoggedInUsers");
+    users_pref->AppendIfNotPresent(
+        std::make_unique<base::Value>(user.account_id.GetUserEmail()));
+
+    DictionaryPrefUpdate user_type_update(g_browser_process->local_state(),
+                                          "UserType");
+    user_type_update->SetKey(user.account_id.GetAccountIdKey(),
+                             base::Value(static_cast<int>(user.user_type)));
+
+    DictionaryPrefUpdate user_token_update(g_browser_process->local_state(),
+                                           "OAuthTokenStatus");
+    user_token_update->SetKey(user.account_id.GetUserEmail(),
+                              base::Value(static_cast<int>(user.token_status)));
+
+    user_manager::known_user::UpdateId(user.account_id);
+
+    if (user.user_type == user_manager::USER_TYPE_CHILD) {
+      user_manager::known_user::SetProfileRequiresPolicy(
+          user.account_id,
+          user_manager::known_user::ProfileRequiresPolicy::kPolicyRequired);
     }
   }
 

chrome/browser/chromeos/system/system_clock.cc

@@ -19,7 +19,6 @@
 #include "chromeos/settings/cros_settings_names.h"
 #include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_service.h"
-#include "components/session_manager/core/session_manager.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/browser/notification_service.h"
@@ -113,21 +112,42 @@ void SystemClock::SetProfile(Profile* profile) {
   UpdateClockType();
 }
 
-void SystemClock::SetLastFocusedPodHourClockType(
+SystemClock::ScopedHourClockType::ScopedHourClockType(
+    base::WeakPtr<SystemClock> system_clock)
+    : system_clock_(std::move(system_clock)) {}
+
+SystemClock::ScopedHourClockType::~ScopedHourClockType() {
+  if (!system_clock_)
+    return;
+  system_clock_->scoped_hour_clock_type_.reset();
+  system_clock_->UpdateClockType();
+}
+
+SystemClock::ScopedHourClockType::ScopedHourClockType(
+    ScopedHourClockType&& other) = default;
+
+SystemClock::ScopedHourClockType& SystemClock::ScopedHourClockType::operator=(
+    ScopedHourClockType&& other) = default;
+
+void SystemClock::ScopedHourClockType::UpdateClockType(
+    base::HourClockType clock_type) {
+  if (!system_clock_)
+    return;
+  system_clock_->scoped_hour_clock_type_ = clock_type;
+  system_clock_->UpdateClockType();
+}
+
+SystemClock::ScopedHourClockType SystemClock::CreateScopedHourClockType(
     base::HourClockType hour_clock_type) {
-  user_pod_was_focused_ = true;
-  last_focused_pod_hour_clock_type_ = hour_clock_type;
+  DCHECK(!scoped_hour_clock_type_.has_value());
+  scoped_hour_clock_type_ = hour_clock_type;
   UpdateClockType();
+  return ScopedHourClockType(weak_ptr_factory_.GetWeakPtr());
 }
 
 bool SystemClock::ShouldUse24HourClock() const {
-  const session_manager::SessionState session_state =
-      session_manager::SessionManager::Get()->session_state();
-  if (session_state == session_manager::SessionState::LOGIN_PRIMARY ||
-      session_state == session_manager::SessionState::LOCKED) {
-    if (user_pod_was_focused_)
-      return last_focused_pod_hour_clock_type_ == base::k24HourClock;
-  }
+  if (scoped_hour_clock_type_.has_value())
+    return scoped_hour_clock_type_ == base::k24HourClock;
   // default is used for kUse24HourClock preference on login screen and whenever
   // set so in user's preference
   const chromeos::LoginState::LoggedInUserType status =

chrome/browser/chromeos/system/system_clock.h

@@ -39,7 +39,27 @@ class SystemClock : public chromeos::LoginState::Observer,
   SystemClock();
   ~SystemClock() override;
 
-  void SetLastFocusedPodHourClockType(base::HourClockType hour_clock_type);
+  // Could be used to temporary set the required clock type. At most one should
+  // exist at the time.
+  class ScopedHourClockType {
+   public:
+    explicit ScopedHourClockType(base::WeakPtr<SystemClock> system_clock);
+    ~ScopedHourClockType();
+
+    ScopedHourClockType(const ScopedHourClockType&) = delete;
+    ScopedHourClockType& operator=(const ScopedHourClockType&) = delete;
+
+    ScopedHourClockType(ScopedHourClockType&&);
+    ScopedHourClockType& operator=(ScopedHourClockType&&);
+
+    void UpdateClockType(base::HourClockType clock_type);
+
+   private:
+    base::WeakPtr<SystemClock> system_clock_;
+  };
+
+  ScopedHourClockType CreateScopedHourClockType(
+      base::HourClockType hour_clock_type);
 
   void AddObserver(SystemClockObserver* observer);
   void RemoveObserver(SystemClockObserver* observer);
@@ -67,8 +87,7 @@ class SystemClock : public chromeos::LoginState::Observer,
 
   void UpdateClockType();
 
-  bool user_pod_was_focused_ = false;
-  base::HourClockType last_focused_pod_hour_clock_type_ = base::k12HourClock;
+  base::Optional<base::HourClockType> scoped_hour_clock_type_;
 
   Profile* user_profile_ = nullptr;
   ScopedObserver<Profile, ProfileObserver> profile_observer_{this};

chrome/browser/ui/ash/system_tray_client_browsertest.cc

@@ -8,8 +8,10 @@
 #include "ash/public/cpp/ash_view_ids.h"
 #include "ash/public/cpp/login_screen_test_api.h"
 #include "ash/public/cpp/system_tray_test_api.h"
+#include "base/i18n/time_formatting.h"
 #include "chrome/browser/chromeos/login/lock/screen_locker_tester.h"
 #include "chrome/browser/chromeos/login/login_manager_test.h"
+#include "chrome/browser/chromeos/login/test/local_state_mixin.h"
 #include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
 #include "chrome/browser/chromeos/login/ui/user_adding_screen.h"
 #include "chrome/browser/chromeos/policy/device_policy_cros_browser_test.h"
@@ -22,6 +24,7 @@
 #include "chromeos/strings/grit/chromeos_strings.h"
 #include "components/account_id/account_id.h"
 #include "components/prefs/pref_service.h"
+#include "components/user_manager/known_user.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test.h"
@@ -147,3 +150,36 @@ IN_PROC_BROWSER_TEST_F(SystemTrayClientClockTest, FocusedPod24HourClock) {
   EXPECT_TRUE(ash::LoginScreenTestApi::FocusUser(account_id2_));
   EXPECT_FALSE(tray_test_api->Is24HourClock());
 }
+
+class SystemTrayClientClockUnknownPrefTest
+    : public SystemTrayClientClockTest,
+      public chromeos::LocalStateMixin::Delegate {
+ public:
+  // chromeos::localStateMixin::Delegate:
+  void SetUpLocalState() override {
+    // Set preference for the first user only.
+    user_manager::known_user::SetBooleanPref(account_id1_,
+                                             ::prefs::kUse24HourClock, true);
+
+    ASSERT_FALSE(user_manager::known_user::GetBooleanPref(
+        account_id2_, ::prefs::kUse24HourClock, nullptr));
+  }
+
+ protected:
+  chromeos::LocalStateMixin local_state_{&mixin_host_, this};
+};
+
+IN_PROC_BROWSER_TEST_F(SystemTrayClientClockUnknownPrefTest, SwitchToDefault) {
+  // Check default value.
+  ASSERT_EQ(base::GetHourClockType(), base::k12HourClock);
+
+  auto tray_test_api = ash::SystemTrayTestApi::Create();
+  // Check user with the set preference.
+  EXPECT_TRUE(ash::LoginScreenTestApi::FocusUser(account_id1_));
+  EXPECT_TRUE(tray_test_api->Is24HourClock());
+
+  // Should get back to the default 12 hours because there is not preference for
+  // the second user.
+  EXPECT_TRUE(ash::LoginScreenTestApi::FocusUser(account_id2_));
+  EXPECT_FALSE(tray_test_api->Is24HourClock());
+}

chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc

@@ -1218,17 +1218,27 @@ void SigninScreenHandler::HandleFocusPod(const AccountId& account_id,
   lock_screen_utils::SetKeyboardSettings(account_id);
 
   bool use_24hour_clock = false;
-  if (user_manager::known_user::GetBooleanPref(
+  if (!user_manager::known_user::GetBooleanPref(
           account_id, prefs::kUse24HourClock, &use_24hour_clock)) {
-    g_browser_process->platform_part()
-        ->GetSystemClock()
-        ->SetLastFocusedPodHourClockType(use_24hour_clock ? base::k24HourClock
-                                                          : base::k12HourClock);
+    focused_user_clock_type_.reset();
+    return;
+  }
+
+  base::HourClockType clock_type =
+      use_24hour_clock ? base::k24HourClock : base::k12HourClock;
+
+  if (focused_user_clock_type_.has_value()) {
+    focused_user_clock_type_->UpdateClockType(clock_type);
+  } else {
+    focused_user_clock_type_ = g_browser_process->platform_part()
+                                   ->GetSystemClock()
+                                   ->CreateScopedHourClockType(clock_type);
   }
 }
 
 void SigninScreenHandler::HandleNoPodFocused() {
   focused_pod_account_id_.reset();
+  focused_user_clock_type_.reset();
 }
 
 void SigninScreenHandler::HandleGetPublicSessionKeyboardLayouts(

chrome/browser/ui/webui/chromeos/login/signin_screen_handler.h

@@ -22,6 +22,7 @@
 #include "chrome/browser/chromeos/login/signin_specifics.h"
 #include "chrome/browser/chromeos/login/ui/login_display.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/system/system_clock.h"
 #include "chrome/browser/ui/webui/chromeos/login/base_webui_handler.h"
 #include "chrome/browser/ui/webui/chromeos/login/network_state_informer.h"
 #include "chrome/browser/ui/webui/chromeos/login/oobe_ui.h"
@@ -420,6 +421,8 @@ class SigninScreenHandler
   std::unique_ptr<LoginFeedback> login_feedback_;
 
   std::unique_ptr<AccountId> focused_pod_account_id_;
+  base::Optional<system::SystemClock::ScopedHourClockType>
+      focused_user_clock_type_;
 
   base::WeakPtrFactory<SigninScreenHandler> weak_factory_{this};