Read configs after Assistant becomes ready

- IsAssistantReady checks that Assistant is ready. Previously we were
  reading some configs in SetAssistant. We have to read them after
  Assistant becomes ready.
- Add AssistantControllerImplTestForStartUp to test start up phase of
  Assistant. SetUpActiveUser in its base class was called in base
  class's SetUp function. This made us difficult to modify configs
  before SetUpActiveUser. If we can call it from a test body, we can
  change configs in a test body and call SetUpActiveUser manually after
  that. This change was necessary for this CL as we read configs in
  AssistantStatus change now, which happens in SetUpActiveUser.

(cherry picked from commit e4cba6e)

Bug: b:266628593,b:266753062
Test: Manually tested as described in the bug.
Change-Id: I227d271e19c1e5abcd5b47fd427facee3266c05b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4194063
Reviewed-by: Tao Wu <wutao@chromium.org>
Auto-Submit: Yuki Awano <yawano@google.com>
Commit-Queue: Yuki Awano <yawano@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1097204}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4209917
Commit-Queue: Tao Wu <wutao@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5481@{#852}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}

ash/assistant/assistant_controller_impl.cc

@@ -20,6 +20,7 @@
 #include "base/bind.h"
 #include "base/memory/scoped_refptr.h"
 #include "chromeos/ash/services/assistant/public/cpp/assistant_browser_delegate.h"
+#include "chromeos/ash/services/assistant/public/cpp/assistant_enums.h"
 #include "chromeos/ash/services/assistant/public/cpp/assistant_prefs.h"
 #include "chromeos/ash/services/assistant/public/cpp/assistant_service.h"
 #include "chromeos/ash/services/assistant/public/cpp/features.h"
@@ -78,9 +79,6 @@ void AssistantControllerImpl::SetAssistant(assistant::Assistant* assistant) {
   assistant_notification_controller_.SetAssistant(assistant);
   assistant_ui_controller_.SetAssistant(assistant);
 
-  OnAccessibilityStatusChanged();
-  OnColorModeChanged(DarkLightModeControllerImpl::Get()->IsDarkModeEnabled());
-
   if (assistant) {
     for (AssistantControllerObserver& observer : observers_)
       observer.OnAssistantReady();
@@ -342,10 +340,17 @@ void AssistantControllerImpl::NotifyUrlOpened(const GURL& url,
 
 void AssistantControllerImpl::OnAssistantStatusChanged(
     assistant::AssistantStatus status) {
-  if (status == assistant::AssistantStatus::NOT_READY) {
-    assistant_volume_control_receiver_.reset();
-    assistant_ui_controller_.CloseUi(
-        assistant::AssistantExitPoint::kUnspecified);
+  switch (status) {
+    case assistant::AssistantStatus::NOT_READY:
+      assistant_volume_control_receiver_.reset();
+      assistant_ui_controller_.CloseUi(
+          assistant::AssistantExitPoint::kUnspecified);
+      break;
+    case assistant::AssistantStatus::READY:
+      OnAccessibilityStatusChanged();
+      OnColorModeChanged(
+          DarkLightModeControllerImpl::Get()->IsDarkModeEnabled());
+      break;
   }
 }
 

ash/assistant/assistant_controller_impl_unittest.cc

@@ -106,17 +106,20 @@ class AssistantControllerImplTest : public AssistantAshTestBase {
   const AssistantUiModel* ui_model() {
     return AssistantUiController::Get()->GetModel();
   }
-  TestAssistantService* test_assistant_service() {
-    return &test_assistant_service_;
-  }
 
  private:
   MockNewWindowDelegate* new_window_delegate_;
   std::unique_ptr<TestNewWindowDelegateProvider> delegate_provider_;
+};
 
-  // AssistantService must outlive AssistantController as destructor can
-  // reference AssistantService.
-  TestAssistantService test_assistant_service_;
+// Same with `AssistantControllerImplTest` except that this class does not set
+// up an active user in `SetUp`.
+class AssistantControllerImplTestForStartUp
+    : public AssistantControllerImplTest {
+ public:
+  AssistantControllerImplTestForStartUp() {
+    set_up_active_user_in_test_set_up_ = false;
+  }
 };
 
 }  // namespace
@@ -256,19 +259,19 @@ TEST_F(AssistantControllerImplTest, ClosesAssistantUiForFeedbackDeeplink) {
 
 // Dark mode is set to true if the DarkLightMode flag is off. This is determined
 // in DarkLightModeControllerImpl::IsDarkModeEnabled().
-TEST_F(AssistantControllerImplTest, ColorModeIsSetWhenAssistantIsReadyFlagOff) {
+TEST_F(AssistantControllerImplTestForStartUp,
+       ColorModeIsSetWhenAssistantIsReadyFlagOff) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitWithFeatures(
       /*enabled_features=*/{}, /*disabled_features=*/{
           chromeos::features::kDarkLightMode, features::kNotificationsRefresh});
+  SetUpActiveUser();
 
-  controller()->SetAssistant(test_assistant_service());
-
-  ASSERT_TRUE(test_assistant_service()->dark_mode_enabled().has_value());
-  EXPECT_TRUE(test_assistant_service()->dark_mode_enabled().value());
+  ASSERT_TRUE(assistant_service()->dark_mode_enabled().has_value());
+  EXPECT_TRUE(assistant_service()->dark_mode_enabled().value());
 }
 
-TEST_F(AssistantControllerImplTest, ColorModeIsUpdated) {
+TEST_F(AssistantControllerImplTestForStartUp, ColorModeIsUpdated) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(chromeos::features::kDarkLightMode);
 
@@ -281,18 +284,18 @@ TEST_F(AssistantControllerImplTest, ColorModeIsUpdated) {
   auto* dark_light_mode_controller = DarkLightModeControllerImpl::Get();
   dark_light_mode_controller->OnActiveUserPrefServiceChanged(
       active_user_pref_service);
-  controller()->SetAssistant(test_assistant_service());
+  SetUpActiveUser();
   const bool initial_dark_mode_status =
       dark_light_mode_controller->IsDarkModeEnabled();
-  ASSERT_TRUE(test_assistant_service()->dark_mode_enabled().has_value());
+  ASSERT_TRUE(assistant_service()->dark_mode_enabled().has_value());
   EXPECT_EQ(initial_dark_mode_status,
-            test_assistant_service()->dark_mode_enabled().value());
+            assistant_service()->dark_mode_enabled().value());
 
   // Switch the color mode.
   dark_light_mode_controller->ToggleColorMode();
-  ASSERT_TRUE(test_assistant_service()->dark_mode_enabled().has_value());
+  ASSERT_TRUE(assistant_service()->dark_mode_enabled().has_value());
   EXPECT_NE(initial_dark_mode_status,
-            test_assistant_service()->dark_mode_enabled().value());
+            assistant_service()->dark_mode_enabled().value());
 }
 
 }  // namespace ash

ash/assistant/test/assistant_ash_test_base.cc

@@ -118,7 +118,9 @@ void AssistantAshTestBase::SetUp() {
   test_api_->DisableAnimations();
   EnableKeyboard();
 
-  SetUpActiveUser();
+  if (set_up_active_user_in_test_set_up_) {
+    SetUpActiveUser();
+  }
 }
 
 void AssistantAshTestBase::TearDown() {

ash/assistant/test/assistant_ash_test_base.h

@@ -204,9 +204,16 @@ class AssistantAshTestBase : public AshTestBase {
 
   TestAssistantService* assistant_service();
 
- private:
+ protected:
+  // Sets up an active user for a test. Note that this function is called in
+  // `SetUp` by default. You can change this behavior by setting
+  // `set_up_active_user_in_test_set_up_`.
   void SetUpActiveUser();
 
+  // This variable must be set before `SetUp` function call.
+  bool set_up_active_user_in_test_set_up_ = true;
+
+ private:
   std::unique_ptr<AssistantTestApi> test_api_;
   std::unique_ptr<TestAssistantSetup> test_setup_;
   std::unique_ptr<TestAshWebViewFactory> test_web_view_factory_;