Close Assistant Ui when launching onboarding flow.

Previously, as reported in the bug, Assistant Ui was *not* closing
in tablet mode.

Bug: b:150252578
Change-Id: I3410fa8fcebc13d564f12f84dc7fe731e1e32ab6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2076097
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746932}

ash/BUILD.gn

@@ -1758,6 +1758,7 @@ test("ash_unittests") {
     "assistant/assistant_interaction_controller_unittest.cc",
     "assistant/assistant_notification_controller_unittest.cc",
     "assistant/assistant_screen_context_controller_unittest.cc",
+    "assistant/assistant_setup_controller_unittest.cc",
     "assistant/assistant_state_controller_unittest.cc",
     "assistant/model/assistant_query_history_unittest.cc",
     "assistant/ui/assistant_web_container_view_unittest.cc",
@@ -2327,6 +2328,8 @@ static_library("test_support") {
     "assistant/test/assistant_ash_test_base.h",
     "assistant/test/test_assistant_service.cc",
     "assistant/test/test_assistant_service.h",
+    "assistant/test/test_assistant_setup.cc",
+    "assistant/test/test_assistant_setup.h",
     "assistant/test/test_assistant_web_view.cc",
     "assistant/test/test_assistant_web_view.h",
     "assistant/test/test_assistant_web_view_factory.cc",
@@ -2460,6 +2463,7 @@ static_library("test_support") {
     "//base:i18n",
     "//base/test:test_support",
     "//cc:test_support",
+    "//chromeos/services/assistant/public/cpp:prefs",
     "//ui/platform_window/common",
 
     # TODO(https://crbug.com/644336): Make CrasAudioHandler Chrome or Ash only.

ash/app_list/views/assistant/assistant_test_api_impl.cc

@@ -91,6 +91,10 @@ views::View* AssistantTestApiImpl::keyboard_input_toggle() {
   return page_view()->GetViewByID(AssistantViewID::kKeyboardInputToggle);
 }
 
+views::View* AssistantTestApiImpl::opt_in_view() {
+  return page_view()->GetViewByID(AssistantViewID::kOptInView);
+}
+
 aura::Window* AssistantTestApiImpl::window() {
   return main_view()->GetWidget()->GetNativeWindow();
 }
@@ -118,6 +122,18 @@ void AssistantTestApiImpl::SetTabletMode(bool enable) {
   TabletMode::Get()->SetEnabledForTest(enable);
 }
 
+void AssistantTestApiImpl::SetConsentStatus(
+    chromeos::assistant::prefs::ConsentStatus consent_status) {
+  Shell::Get()->session_controller()->GetPrimaryUserPrefService()->SetInteger(
+      chromeos::assistant::prefs::kAssistantConsentStatus, consent_status);
+
+  // Ensure the value has taken effect.
+  ASSERT_EQ(GetAssistantState()->consent_status(), consent_status)
+      << "Changing this preference did not take effect immediately, which will "
+         "cause timing issues in this test. If this trace is seen we must add "
+         "a waiter here to wait for the new state to take effect.";
+}
+
 void AssistantTestApiImpl::SetPreferVoice(bool value) {
   Shell::Get()->session_controller()->GetPrimaryUserPrefService()->SetBoolean(
       chromeos::assistant::prefs::kAssistantLaunchWithMicOpen, value);

ash/app_list/views/assistant/assistant_test_api_impl.h

@@ -31,6 +31,7 @@ class AssistantTestApiImpl : public AssistantTestApi {
   void SendTextQuery(const std::string& query) override;
   void SetAssistantEnabled(bool enable) override;
   void SetTabletMode(bool enable) override;
+  void SetConsentStatus(chromeos::assistant::prefs::ConsentStatus) override;
   void SetPreferVoice(bool value) override;
   AssistantState* GetAssistantState() override;
   void WaitUntilIdle() override;
@@ -42,6 +43,7 @@ class AssistantTestApiImpl : public AssistantTestApi {
   views::View* greeting_label() override;
   views::View* voice_input_toggle() override;
   views::View* keyboard_input_toggle() override;
+  views::View* opt_in_view() override;
   aura::Window* window() override;
   views::View* app_list_view() override;
   aura::Window* root_window() override;

ash/assistant/assistant_setup_controller.cc

@@ -54,9 +54,9 @@ void AssistantSetupController::OnDeepLinkReceived(
 }
 
 void AssistantSetupController::OnOptInButtonPressed() {
+  using chromeos::assistant::prefs::ConsentStatus;
   if (AssistantState::Get()->consent_status().value_or(
-          chromeos::assistant::prefs::ConsentStatus::kUnknown) ==
-      chromeos::assistant::prefs::ConsentStatus::kUnauthorized) {
+          ConsentStatus::kUnknown) == ConsentStatus::kUnauthorized) {
     assistant_controller_->OpenUrl(assistant::util::CreateLocalizedGURL(
         kGSuiteAdministratorInstructionsUrl));
   } else {
@@ -69,20 +69,20 @@ void AssistantSetupController::StartOnboarding(bool relaunch, FlowType type) {
   if (!assistant_setup)
     return;
 
-  if (relaunch) {
-    assistant_setup->StartAssistantOptInFlow(
-        type, base::BindOnce(&AssistantSetupController::OnOptInFlowFinished,
-                             weak_ptr_factory_.GetWeakPtr()));
-    return;
-  }
+  assistant_controller_->ui_controller()->CloseUi(
+      chromeos::assistant::mojom::AssistantExitPoint::kSetup);
 
-  assistant_setup->StartAssistantOptInFlow(type, base::DoNothing());
+  assistant_setup->StartAssistantOptInFlow(
+      type, base::BindOnce(&AssistantSetupController::OnOptInFlowFinished,
+                           weak_ptr_factory_.GetWeakPtr(), relaunch));
 }
 
-void AssistantSetupController::OnOptInFlowFinished(bool completed) {
-  if (completed)
+void AssistantSetupController::OnOptInFlowFinished(bool relaunch,
+                                                   bool completed) {
+  if (relaunch && completed) {
     assistant_controller_->ui_controller()->ShowUi(
         chromeos::assistant::mojom::AssistantEntryPoint::kSetup);
+  }
 }
 
 }  // namespace ash

ash/assistant/assistant_setup_controller.h

@@ -37,7 +37,7 @@ class AssistantSetupController : public AssistantControllerObserver,
   void StartOnboarding(bool relaunch, FlowType type = FlowType::kConsentFlow);
 
  private:
-  void OnOptInFlowFinished(bool completed);
+  void OnOptInFlowFinished(bool relaunch, bool completed);
 
   AssistantController* const assistant_controller_;  // Owned by Shell.
 

ash/assistant/assistant_setup_controller_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/assistant/test/assistant_ash_test_base.h"
+#include "ash/assistant/test/test_assistant_setup.h"
+#include "ash/public/cpp/assistant/assistant_setup.h"
+#include "ui/views/view.h"
+
+namespace ash {
+
+namespace {
+
+class AssistantSetupControllerTest : public AssistantAshTestBase {
+ protected:
+  // Invoke to finish opt-in flow with the desired state of completion. Note
+  // that this API may only be called while opt-in flow is in progress.
+  void FinishAssistantOptInFlow(bool completed) {
+    DCHECK(AssistantSetup::GetInstance());
+    static_cast<TestAssistantSetup*>(AssistantSetup::GetInstance())
+        ->FinishAssistantOptInFlow(completed);
+  }
+};
+
+}  // namespace
+
+TEST_F(AssistantSetupControllerTest, ShouldCloseAssistantUiWhenOnboarding) {
+  ShowAssistantUi(AssistantEntryPoint::kUnspecified);
+  EXPECT_TRUE(IsVisible());
+
+  SetConsentStatus(chromeos::assistant::prefs::ConsentStatus::kUnknown);
+  EXPECT_TRUE(opt_in_view()->GetVisible());
+
+  ClickOnAndWait(opt_in_view());
+
+  EXPECT_FALSE(IsVisible());
+}
+
+TEST_F(AssistantSetupControllerTest,
+       ShouldCloseAssistantUiWhenOnboardingInTabletMode) {
+  SetTabletMode(true);
+
+  ShowAssistantUi(AssistantEntryPoint::kUnspecified);
+  EXPECT_TRUE(IsVisible());
+
+  SetConsentStatus(chromeos::assistant::prefs::ConsentStatus::kUnknown);
+  EXPECT_TRUE(opt_in_view()->GetVisible());
+
+  ClickOnAndWait(opt_in_view());
+
+  EXPECT_FALSE(IsVisible());
+}
+
+TEST_F(AssistantSetupControllerTest,
+       ShouldNotRelaunchAssistantIfOptInFlowAborted) {
+  ShowAssistantUi(AssistantEntryPoint::kUnspecified);
+  EXPECT_TRUE(IsVisible());
+
+  SetConsentStatus(chromeos::assistant::prefs::ConsentStatus::kUnknown);
+  EXPECT_TRUE(opt_in_view()->GetVisible());
+
+  ClickOnAndWait(opt_in_view());
+
+  EXPECT_FALSE(IsVisible());
+
+  FinishAssistantOptInFlow(/*completed=*/false);
+
+  EXPECT_FALSE(IsVisible());
+}
+
+TEST_F(AssistantSetupControllerTest,
+       ShouldRelaunchAssistantIfOptInFlowCompleted) {
+  ShowAssistantUi(AssistantEntryPoint::kUnspecified);
+  EXPECT_TRUE(IsVisible());
+
+  SetConsentStatus(chromeos::assistant::prefs::ConsentStatus::kUnknown);
+  EXPECT_TRUE(opt_in_view()->GetVisible());
+
+  ClickOnAndWait(opt_in_view());
+
+  EXPECT_FALSE(IsVisible());
+
+  FinishAssistantOptInFlow(/*completed=*/true);
+
+  EXPECT_TRUE(IsVisible());
+}
+
+}  // namespace ash

ash/assistant/test/assistant_ash_test_base.cc

@@ -9,6 +9,7 @@
 
 #include "ash/app_list/app_list_controller_impl.h"
 #include "ash/assistant/assistant_controller.h"
+#include "ash/assistant/test/test_assistant_setup.h"
 #include "ash/assistant/test/test_assistant_web_view_factory.h"
 #include "ash/keyboard/ui/keyboard_ui_controller.h"
 #include "ash/keyboard/ui/test/keyboard_test_util.h"
@@ -57,6 +58,7 @@ void PressHomeButton() {
 
 AssistantAshTestBase::AssistantAshTestBase()
     : test_api_(AssistantTestApi::Create()),
+      test_setup_(std::make_unique<TestAssistantSetup>()),
       test_web_view_factory_(std::make_unique<TestAssistantWebViewFactory>()) {}
 
 AssistantAshTestBase::~AssistantAshTestBase() = default;
@@ -120,6 +122,11 @@ void AssistantAshTestBase::SetTabletMode(bool enable) {
   test_api_->SetTabletMode(enable);
 }
 
+void AssistantAshTestBase::SetConsentStatus(
+    chromeos::assistant::prefs::ConsentStatus consent_status) {
+  test_api_->SetConsentStatus(consent_status);
+}
+
 void AssistantAshTestBase::SetPreferVoice(bool prefer_voice) {
   test_api_->SetPreferVoice(prefer_voice);
 }
@@ -235,6 +242,10 @@ views::View* AssistantAshTestBase::keyboard_input_toggle() {
   return test_api_->keyboard_input_toggle();
 }
 
+views::View* AssistantAshTestBase::opt_in_view() {
+  return test_api_->opt_in_view();
+}
+
 void AssistantAshTestBase::ShowKeyboard() {
   auto* keyboard_controller = keyboard::KeyboardUIController::Get();
   keyboard_controller->ShowKeyboard(/*lock=*/false);

ash/assistant/test/assistant_ash_test_base.h

@@ -12,6 +12,7 @@
 #include "ash/assistant/model/assistant_ui_model.h"
 #include "ash/test/ash_test_base.h"
 #include "base/macros.h"
+#include "chromeos/services/assistant/public/cpp/assistant_prefs.h"
 
 namespace aura {
 class Window;
@@ -30,6 +31,7 @@ class AssistantInteractionController;
 class AssistantInteractionModel;
 class AssistantTestApi;
 class TestAssistantService;
+class TestAssistantSetup;
 class TestAssistantWebViewFactory;
 
 // Helper class to make testing the Assistant Ash UI easier.
@@ -61,6 +63,9 @@ class AssistantAshTestBase : public AshTestBase {
 
   void SetTabletMode(bool enable);
 
+  // Changes the user preference controlling the status of user consent.
+  void SetConsentStatus(chromeos::assistant::prefs::ConsentStatus);
+
   // Change the user setting controlling whether the user prefers voice or
   // keyboard.
   void SetPreferVoice(bool value);
@@ -141,6 +146,9 @@ class AssistantAshTestBase : public AshTestBase {
   // Return the button to enable text mode.
   views::View* keyboard_input_toggle();
 
+  // Returns the button to launch Assistant onboarding.
+  views::View* opt_in_view();
+
   // Show/Dismiss the on-screen keyboard.
   void ShowKeyboard();
   void DismissKeyboard();
@@ -159,6 +167,7 @@ class AssistantAshTestBase : public AshTestBase {
   TestAssistantService* assistant_service();
 
   std::unique_ptr<AssistantTestApi> test_api_;
+  std::unique_ptr<TestAssistantSetup> test_setup_;
   std::unique_ptr<TestAssistantWebViewFactory> test_web_view_factory_;
   AssistantController* controller_ = nullptr;
 

ash/assistant/test/test_assistant_setup.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/assistant/test/test_assistant_setup.h"
+
+namespace ash {
+
+TestAssistantSetup::TestAssistantSetup() = default;
+
+TestAssistantSetup::~TestAssistantSetup() = default;
+
+void TestAssistantSetup::StartAssistantOptInFlow(
+    FlowType type,
+    StartAssistantOptInFlowCallback callback) {
+  if (callback_) {
+    // If opt-in flow is already in progress, immediately return |false|. This
+    // behavior is consistent w/ the actual browser-side implementation.
+    std::move(callback).Run(false);
+    return;
+  }
+  // Otherwise we cache the callback until FinishAssistantOptInFlow() is called.
+  callback_ = std::move(callback);
+}
+
+bool TestAssistantSetup::BounceOptInWindowIfActive() {
+  // If |callback_| exists, opt-in flow is in progress and the browser-side
+  // implementation would return |true| after bouncing the dialog window.
+  return !!callback_;
+}
+
+void TestAssistantSetup::FinishAssistantOptInFlow(bool completed) {
+  DCHECK(callback_);
+  std::move(callback_).Run(completed);
+}
+
+}  // namespace ash

ash/assistant/test/test_assistant_setup.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef ASH_ASSISTANT_TEST_TEST_ASSISTANT_SETUP_H_
+#define ASH_ASSISTANT_TEST_TEST_ASSISTANT_SETUP_H_
+
+#include "ash/public/cpp/assistant/assistant_setup.h"
+#include "base/callback.h"
+
+namespace ash {
+
+// An implementation of AssistantSetup for use in unittests.
+class TestAssistantSetup : public AssistantSetup {
+ public:
+  TestAssistantSetup();
+  TestAssistantSetup(const TestAssistantSetup&) = delete;
+  TestAssistantSetup& operator=(const TestAssistantSetup&) = delete;
+  ~TestAssistantSetup() override;
+
+  // AssistantSetup:
+  void StartAssistantOptInFlow(
+      FlowType type,
+      StartAssistantOptInFlowCallback callback) override;
+  bool BounceOptInWindowIfActive() override;
+
+  // Invoke in unittests to finish opt-in flow with the desired state of
+  // completion. Note that this API may only be called while opt-in flow is in
+  // progress (as indicated by existence of |callback_|).
+  void FinishAssistantOptInFlow(bool completed);
+
+ private:
+  // If |callback_| exists, that means that opt-in flow is in progress as far
+  // as unittests are concerned.
+  StartAssistantOptInFlowCallback callback_;
+};
+
+}  // namespace ash
+
+#endif  // ASH_ASSISTANT_TEST_TEST_ASSISTANT_SETUP_H_

ash/public/cpp/test/assistant_test_api.h

@@ -9,6 +9,7 @@
 #include <string>
 
 #include "ash/ash_export.h"
+#include "chromeos/services/assistant/public/cpp/assistant_prefs.h"
 
 namespace aura {
 class Window;
@@ -47,6 +48,9 @@ class ASH_EXPORT AssistantTestApi {
 
   virtual void SetTabletMode(bool enable) = 0;
 
+  // Changes the user preference controlling the status of user consent.
+  virtual void SetConsentStatus(chromeos::assistant::prefs::ConsentStatus) = 0;
+
   // Changes the user setting controlling whether the user prefers voice or
   // keyboard (internally called |kAssistantLaunchWithMicOpen|).
   // This will ensure the new value is propagated to the |AssistantState|.
@@ -91,6 +95,10 @@ class ASH_EXPORT AssistantTestApi {
   // Can only be used after the Assistant UI has been shown at least once.
   virtual views::View* keyboard_input_toggle() = 0;
 
+  // Returns the button to launch Assistant onboarding.
+  // Can only be used after the Assistant UI has been shown at least once.
+  virtual views::View* opt_in_view() = 0;
+
   // Returns the window containing the Assistant UI.
   // Note that this window is shared for all components of the |AppList|.
   // Can only be used after the Assistant UI has been shown at least once.