assistant: close assistant ui for feedback

then click thumbs down emoji.

Bug: b/157520832
Test: open assistant with hotword, run any query,
Change-Id: I439339082e84d5234a291787e7be9485fb817827
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2286954
Commit-Queue: Jeffrey Young <cowmoo@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Jeroen Dhollander <jeroendh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#787758}

ash/assistant/assistant_controller_impl.cc

@@ -204,6 +204,10 @@ void AssistantControllerImpl::OnDeepLinkReceived(
     case DeepLinkType::kFeedback:
       NewWindowDelegate::GetInstance()->OpenFeedbackPage(
           /*from_assistant=*/true);
+
+      // Close the assistant UI so that the feedback page is visible.
+      assistant_ui_controller_.CloseUi(
+          chromeos::assistant::AssistantExitPoint::kUnspecified);
       break;
     case DeepLinkType::kScreenshot:
       // We close the UI before taking the screenshot as it's probably not the

ash/assistant/assistant_controller_impl_unittest.cc

@@ -50,6 +50,20 @@ class MockAssistantControllerObserver : public AssistantControllerObserver {
               (override));
 };
 
+class MockAssistantUiModelObserver : public AssistantUiModelObserver {
+ public:
+  MockAssistantUiModelObserver() = default;
+  ~MockAssistantUiModelObserver() override = default;
+
+  MOCK_METHOD(void,
+              OnUiVisibilityChanged,
+              (AssistantVisibility new_visibility,
+               AssistantVisibility old_visibility,
+               base::Optional<AssistantEntryPoint> entry_point,
+               base::Optional<AssistantExitPoint> exit_point),
+              (override));
+};
+
 // MockNewWindowDelegate -------------------------------------------------------
 
 class MockNewWindowDelegate : public testing::NiceMock<TestNewWindowDelegate> {
@@ -59,6 +73,8 @@ class MockNewWindowDelegate : public testing::NiceMock<TestNewWindowDelegate> {
               NewTabWithUrl,
               (const GURL& url, bool from_user_interaction),
               (override));
+
+  MOCK_METHOD(void, OpenFeedbackPage, (bool from_assistant), (override));
 };
 
 // AssistantControllerImplTest -------------------------------------------------
@@ -67,6 +83,9 @@ class AssistantControllerImplTest : public AssistantAshTestBase {
  public:
   AssistantController* controller() { return AssistantController::Get(); }
   MockNewWindowDelegate& new_window_delegate() { return new_window_delegate_; }
+  const AssistantUiModel* ui_model() {
+    return AssistantUiController::Get()->GetModel();
+  }
 
  private:
   MockNewWindowDelegate new_window_delegate_;
@@ -78,11 +97,12 @@ class AssistantControllerImplTest : public AssistantAshTestBase {
 
 // Tests that AssistantController observers are notified of deep link received.
 TEST_F(AssistantControllerImplTest, NotifiesDeepLinkReceived) {
-  testing::NiceMock<MockAssistantControllerObserver> mock;
-  ScopedObserver<AssistantController, AssistantControllerObserver> obs{&mock};
-  obs.Add(controller());
+  testing::NiceMock<MockAssistantControllerObserver> controller_observer_mock;
+  ScopedObserver<AssistantController, AssistantControllerObserver>
+      scoped_controller_obs{&controller_observer_mock};
+  scoped_controller_obs.Add(controller());
 
-  EXPECT_CALL(mock, OnDeepLinkReceived)
+  EXPECT_CALL(controller_observer_mock, OnDeepLinkReceived)
       .WillOnce(
           testing::Invoke([](assistant::util::DeepLinkType type,
                              const std::map<std::string, std::string>& params) {
@@ -101,14 +121,15 @@ TEST_F(AssistantControllerImplTest, NotifiesDeepLinkReceived) {
 // having been opened. Note that it is important that these events be notified
 // before and after the URL is actually opened respectively.
 TEST_F(AssistantControllerImplTest, NotifiesOpeningUrlAndUrlOpened) {
-  testing::NiceMock<MockAssistantControllerObserver> mock;
-  ScopedObserver<AssistantController, AssistantControllerObserver> obs{&mock};
-  obs.Add(controller());
+  testing::NiceMock<MockAssistantControllerObserver> controller_observer_mock;
+  ScopedObserver<AssistantController, AssistantControllerObserver>
+      scoped_controller_obs{&controller_observer_mock};
+  scoped_controller_obs.Add(controller());
 
   // Enforce ordering of events.
   testing::InSequence sequence;
 
-  EXPECT_CALL(mock, OnOpeningUrl)
+  EXPECT_CALL(controller_observer_mock, OnOpeningUrl)
       .WillOnce(testing::Invoke(
           [](const GURL& url, bool in_background, bool from_server) {
             EXPECT_EQ(GURL("https://g.co/"), url);
@@ -122,7 +143,7 @@ TEST_F(AssistantControllerImplTest, NotifiesOpeningUrlAndUrlOpened) {
         EXPECT_TRUE(from_user_interaction);
       });
 
-  EXPECT_CALL(mock, OnUrlOpened)
+  EXPECT_CALL(controller_observer_mock, OnUrlOpened)
       .WillOnce(testing::Invoke([](const GURL& url, bool from_server) {
         EXPECT_EQ(GURL("https://g.co/"), url);
         EXPECT_TRUE(from_server);
@@ -132,4 +153,49 @@ TEST_F(AssistantControllerImplTest, NotifiesOpeningUrlAndUrlOpened) {
                         /*from_server=*/true);
 }
 
+TEST_F(AssistantControllerImplTest, OpensFeedbackPageForFeedbackDeeplink) {
+  testing::NiceMock<MockAssistantControllerObserver> controller_observer_mock;
+  ScopedObserver<AssistantController, AssistantControllerObserver>
+      scoped_controller_obs{&controller_observer_mock};
+  scoped_controller_obs.Add(controller());
+
+  EXPECT_CALL(controller_observer_mock, OnDeepLinkReceived)
+      .WillOnce(
+          testing::Invoke([](assistant::util::DeepLinkType type,
+                             const std::map<std::string, std::string>& params) {
+            EXPECT_EQ(assistant::util::DeepLinkType::kFeedback, type);
+            std::map<std::string, std::string> expected_params;
+            EXPECT_EQ(params, expected_params);
+          }));
+
+  EXPECT_CALL(new_window_delegate(), OpenFeedbackPage)
+      .WillOnce([](bool from_assistant) { EXPECT_TRUE(from_assistant); });
+
+  controller()->OpenUrl(GURL("googleassistant://send-feedback"),
+                        /*in_background=*/false, /*from_server=*/true);
+}
+
+TEST_F(AssistantControllerImplTest, ClosesAssistantUiForFeedbackDeeplink) {
+  ShowAssistantUi();
+
+  testing::NiceMock<MockAssistantUiModelObserver> ui_model_observer_mock;
+  ui_model()->AddObserver(&ui_model_observer_mock);
+
+  EXPECT_CALL(ui_model_observer_mock, OnUiVisibilityChanged)
+      .WillOnce([](AssistantVisibility new_visibility,
+                   AssistantVisibility old_visibility,
+                   base::Optional<AssistantEntryPoint> entry_point,
+                   base::Optional<AssistantExitPoint> exit_point) {
+        EXPECT_EQ(old_visibility, AssistantVisibility::kVisible);
+        EXPECT_EQ(new_visibility, AssistantVisibility::kClosed);
+        EXPECT_FALSE(entry_point.has_value());
+        EXPECT_EQ(exit_point.value(), AssistantExitPoint::kUnspecified);
+      });
+
+  controller()->OpenUrl(GURL("googleassistant://send-feedback"),
+                        /*in_background=*/false, /*from_server=*/true);
+
+  ui_model()->RemoveObserver(&ui_model_observer_mock);
+}
+
 }  // namespace ash