RGB Keyboards: Initialize keyboard to white on OOBE

- Observes session states to initialize to white on OOBE
- Solves race condition within RgbKeyboardManager when color gets set
  before the manager is ready

Bug: b/247882525
Test: ash_unittests
Change-Id: I1f915e39b4d78a60663f287421bb3a45bc9ec980
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3916687
Reviewed-by: Jimmy Gong <jimmyxgong@chromium.org>
Quick-Run: David Padlipsky <dpad@google.com>
Commit-Queue: David Padlipsky <dpad@google.com>
Cr-Commit-Position: refs/heads/main@{#1052147}

ash/rgb_keyboard/rgb_keyboard_manager.cc

@@ -63,6 +63,8 @@ void RgbKeyboardManager::SetStaticBackgroundColor(uint8_t r,
                                                   uint8_t g,
                                                   uint8_t b) {
   DCHECK(RgbkbdClient::Get());
+  background_type_ = BackgroundType::kStaticSingleColor;
+  background_color_ = SkColorSetRGB(r, g, b);
   if (!IsRgbKeyboardSupported()) {
     LOG(ERROR) << "Attempted to set RGB keyboard color, but flag is disabled.";
     return;
@@ -79,6 +81,7 @@ void RgbKeyboardManager::SetStaticBackgroundColor(uint8_t r,
 
 void RgbKeyboardManager::SetRainbowMode() {
   DCHECK(RgbkbdClient::Get());
+  background_type_ = BackgroundType::kStaticRainbow;
   if (!IsRgbKeyboardSupported()) {
     LOG(ERROR) << "Attempted to set RGB rainbow mode, but flag is disabled.";
     return;
@@ -140,6 +143,23 @@ void RgbKeyboardManager::OnGetRgbKeyboardCapabilities(
 void RgbKeyboardManager::InitializeRgbKeyboard() {
   DCHECK(RgbkbdClient::Get());
 
+  // Initialize the keyboard based on the correct current state.
+  // `background_type_` will usually be set to kNone. In some cases, a color
+  // may have been set by `KeyboardBacklightColorController` before we are fully
+  // initialized, so here we will correctly set the color once ready.
+  switch (background_type_) {
+    case BackgroundType::kStaticSingleColor:
+      SetStaticBackgroundColor(SkColorGetR(background_color_),
+                               SkColorGetG(background_color_),
+                               SkColorGetB(background_color_));
+      break;
+    case BackgroundType::kStaticRainbow:
+      SetRainbowMode();
+      break;
+    case BackgroundType::kNone:
+      break;
+  }
+
   // Initialize caps lock color changing if supported
   if (IsPerKeyKeyboard()) {
     VLOG(1) << "Setting initial RGB keyboard caps lock state to "

ash/rgb_keyboard/rgb_keyboard_manager.h

@@ -12,6 +12,7 @@
 #include "base/memory/raw_ptr.h"
 #include "chromeos/ash/components/dbus/rgbkbd/rgbkbd_client.h"
 #include "third_party/cros_system_api/dbus/rgbkbd/dbus-constants.h"
+#include "third_party/skia/include/core/SkColor.h"
 
 namespace ash {
 
@@ -41,6 +42,13 @@ class ASH_EXPORT RgbKeyboardManager : public ImeControllerImpl::Observer,
   }
 
  private:
+  // Enum to track the background mode sent to rgbkbd
+  enum class BackgroundType {
+    kNone,
+    kStaticSingleColor,
+    kStaticRainbow,
+  };
+
   // ImeControllerImpl::Observer:
   void OnCapsLockChanged(bool enabled) override;
   void OnKeyboardLayoutNameChanged(const std::string&) override {}
@@ -63,6 +71,11 @@ class ASH_EXPORT RgbKeyboardManager : public ImeControllerImpl::Observer,
 
   raw_ptr<ImeControllerImpl> ime_controller_ptr_;
 
+  // Tracks the currently set background color when `background_type_` is set to
+  // `BackgroundType::kStaticSingleColor`.
+  SkColor background_color_;
+  BackgroundType background_type_ = BackgroundType::kNone;
+
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.
   base::WeakPtrFactory<RgbKeyboardManager> weak_ptr_factory_{this};

ash/rgb_keyboard/rgb_keyboard_manager_unittest.cc

@@ -11,12 +11,10 @@
 #include "ash/ime/ime_controller_impl.h"
 #include "ash/rgb_keyboard/histogram_util.h"
 #include "base/memory/raw_ptr.h"
-#include "base/strings/strcat.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
 #include "chromeos/ash/components/dbus/rgbkbd/fake_rgbkbd_client.h"
 #include "chromeos/ash/components/dbus/rgbkbd/rgbkbd_client.h"
-#include "rgb_keyboard_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace ash {
@@ -262,4 +260,52 @@ TEST_F(RgbKeyboardManagerTest, SetCapsLockStateAllowedForPerKeyKeboards) {
   ime_controller_->UpdateCapsLockState(/*caps_enabled=*/true);
   EXPECT_TRUE(client_->get_caps_lock_state());
 }
+
+// Following `RaceCondition*` tests verify that if a keyboard backlight color is
+// set before `RgbKeyboardManager` is ready, that the color is set once
+// `RgbKeyboardManager` is finally ready.
+TEST_F(RgbKeyboardManagerTest, RaceConditionStaticSingleColor) {
+  const uint8_t expected_r = 1;
+  const uint8_t expected_g = 2;
+  const uint8_t expected_b = 3;
+
+  client_->set_should_run_rgb_keyboard_capabilities_callback(
+      /*should_run_callback=*/false);
+  InitializeManagerWithCapability(
+      rgbkbd::RgbKeyboardCapabilities::kIndividualKey);
+
+  manager_->SetStaticBackgroundColor(expected_r, expected_g, expected_b);
+  {
+    const RgbColor& rgb_values = client_->recently_sent_rgb();
+    EXPECT_NE(expected_r, std::get<0>(rgb_values));
+    EXPECT_NE(expected_g, std::get<1>(rgb_values));
+    EXPECT_NE(expected_b, std::get<2>(rgb_values));
+  }
+
+  client_->set_should_run_rgb_keyboard_capabilities_callback(
+      /*should_run_callback=*/true);
+  client_->attempt_run_rgb_keyboard_capabilities_callback();
+  {
+    const RgbColor& rgb_values = client_->recently_sent_rgb();
+    EXPECT_EQ(expected_r, std::get<0>(rgb_values));
+    EXPECT_EQ(expected_g, std::get<1>(rgb_values));
+    EXPECT_EQ(expected_b, std::get<2>(rgb_values));
+  }
+}
+
+TEST_F(RgbKeyboardManagerTest, RaceConditionStaticRainbow) {
+  client_->set_should_run_rgb_keyboard_capabilities_callback(
+      /*should_run_callback=*/false);
+  InitializeManagerWithCapability(
+      rgbkbd::RgbKeyboardCapabilities::kIndividualKey);
+
+  manager_->SetRainbowMode();
+  EXPECT_FALSE(client_->is_rainbow_mode_set());
+
+  client_->set_should_run_rgb_keyboard_capabilities_callback(
+      /*should_run_callback=*/true);
+  client_->attempt_run_rgb_keyboard_capabilities_callback();
+  EXPECT_TRUE(client_->is_rainbow_mode_set());
+}
+
 }  // namespace ash

ash/system/keyboard_brightness/keyboard_backlight_color_controller.cc

@@ -13,11 +13,11 @@
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "ash/system/keyboard_brightness/keyboard_backlight_color_nudge_controller.h"
-#include "ash/wallpaper/wallpaper_controller_impl.h"
-#include "ash/webui/personalization_app/mojom/personalization_app.mojom.h"
+#include "ash/webui/personalization_app/mojom/personalization_app.mojom-shared.h"
 #include "base/metrics/histogram_functions.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
+#include "components/session_manager/session_manager_types.h"
 #include "third_party/skia/include/core/SkColor.h"
 
 namespace ash {
@@ -83,6 +83,14 @@ KeyboardBacklightColorController::GetBacklightColor(
       pref_service->GetInteger(prefs::kPersonalizationKeyboardBacklightColor));
 }
 
+void KeyboardBacklightColorController::OnSessionStateChanged(
+    session_manager::SessionState state) {
+  // If we are in OOBE, we should set the backlight to a default of white.
+  if (state != session_manager::SessionState::OOBE)
+    return;
+  DisplayBacklightColor(personalization_app::mojom::BacklightColor::kWhite);
+}
+
 void KeyboardBacklightColorController::OnActiveUserPrefServiceChanged(
     PrefService* pref_service) {
   const auto backlight_color = GetBacklightColor(GetActiveAccountId());

ash/system/keyboard_brightness/keyboard_backlight_color_controller.h

@@ -11,6 +11,7 @@
 #include "ash/public/cpp/wallpaper/wallpaper_controller_observer.h"
 #include "ash/webui/personalization_app/mojom/personalization_app.mojom-shared.h"
 #include "base/scoped_observation.h"
+#include "components/session_manager/session_manager_types.h"
 #include "third_party/skia/include/core/SkColor.h"
 
 class PrefRegistrySimple;
@@ -49,6 +50,7 @@ class ASH_EXPORT KeyboardBacklightColorController
   // b/239967737: |OnActiveUserPrefServiceChanged| doesn't get triggered when
   // chrome restarts.
   void OnUserSessionUpdated(const AccountId& account_id) override;
+  void OnSessionStateChanged(session_manager::SessionState state) override;
 
   // WallpaperControllerObserver:
   void OnWallpaperColorsChanged() override;

ash/system/keyboard_brightness/keyboard_backlight_color_controller_unittest.cc

@@ -11,6 +11,7 @@
 #include "ash/wallpaper/wallpaper_controller_impl.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
+#include "components/session_manager/session_manager_types.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkColor.h"
 
@@ -191,4 +192,21 @@ TEST_F(KeyboardBacklightColorControllerTest,
   EXPECT_EQ(kDefaultColor, displayed_color());
 }
 
+TEST_F(KeyboardBacklightColorControllerTest, DisplayWhiteBacklightOnOobe) {
+  controller_->OnSessionStateChanged(session_manager::SessionState::ACTIVE);
+  EXPECT_NE(ConvertBacklightColorToSkColor(
+                personalization_app::mojom::BacklightColor::kWhite),
+            displayed_color());
+
+  controller_->OnSessionStateChanged(session_manager::SessionState::LOCKED);
+  EXPECT_NE(ConvertBacklightColorToSkColor(
+                personalization_app::mojom::BacklightColor::kWhite),
+            displayed_color());
+
+  controller_->OnSessionStateChanged(session_manager::SessionState::OOBE);
+  EXPECT_EQ(ConvertBacklightColorToSkColor(
+                personalization_app::mojom::BacklightColor::kWhite),
+            displayed_color());
+}
+
 }  // namespace ash

chromeos/ash/components/dbus/rgbkbd/fake_rgbkbd_client.cc

@@ -13,7 +13,8 @@ FakeRgbkbdClient::~FakeRgbkbdClient() = default;
 
 void FakeRgbkbdClient::GetRgbKeyboardCapabilities(
     GetRgbKeyboardCapabilitiesCallback callback) {
-  std::move(callback).Run(capabilities_);
+  callback_ = std::move(callback);
+  attempt_run_rgb_keyboard_capabilities_callback();
 }
 
 void FakeRgbkbdClient::SetCapsLockState(bool enabled) {

chromeos/ash/components/dbus/rgbkbd/fake_rgbkbd_client.h

@@ -49,6 +49,17 @@ class COMPONENT_EXPORT(RGBKBD) FakeRgbkbdClient : public RgbkbdClient {
 
   const RgbColor& recently_sent_rgb() const { return rgb_color_; }
 
+  void attempt_run_rgb_keyboard_capabilities_callback() {
+    if (callback_.is_null() || !should_run_callback_)
+      return;
+    std::move(callback_).Run(capabilities_);
+  }
+
+  void set_should_run_rgb_keyboard_capabilities_callback(
+      bool should_run_callback) {
+    should_run_callback_ = should_run_callback;
+  }
+
   int animation_mode_call_count() const { return animation_mode_call_count_; }
 
   void ResetStoredRgbColors();
@@ -59,6 +70,12 @@ class COMPONENT_EXPORT(RGBKBD) FakeRgbkbdClient : public RgbkbdClient {
   bool is_rainbow_mode_set_ = false;
   RgbColor rgb_color_;
   int animation_mode_call_count_ = 0;
+  GetRgbKeyboardCapabilitiesCallback callback_;
+
+  // Set if the the `GetRgbKeyboardCapabilitiesCallback` should be ran right
+  // when `GetRgbKeyboardCapabilities` is called or if it should be delayed to
+  // be manual executed for testing.
+  bool should_run_callback_ = true;
 };
 
 }  // namespace ash