Introduce BloomInteractionObserver

This simplifies the code in multiple ways:
   1) BloomControllerImpl no longer relies on any of the other Assistant
      classes, like |AssistantInteractionObserver| and
      |AssistantActionObserver|.
   2) BloomControllerImpl no longer needs to expose methods like
      HasInteraction() and GetLastInteractionResolution() just for the
      purpose of unittests.

Bug: b/165356952
Tests: chromeos_components_unittests --gtest_filter="Bloom*.*"
Change-Id: I150c2be11ccad611bf468ad33c5835e9c7146d7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2365753
Commit-Queue: Jeroen Dhollander <jeroendh@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#802777}

chromeos/components/bloom/BUILD.gn

@@ -8,6 +8,9 @@ source_set("bloom") {
     "bloom_controller_impl.h",
     "bloom_interaction.cc",
     "bloom_interaction.h",
+    "bloom_interaction_observer.h",
+    "bloom_interaction_observer_impl.cc",
+    "bloom_interaction_observer_impl.h",
     "screenshot_grabber.h",
   ]
 

chromeos/components/bloom/bloom_controller_impl.cc

@@ -4,7 +4,7 @@
 
 #include "chromeos/components/bloom/bloom_controller_impl.h"
 
-#include "ash/public/cpp/assistant/controller/assistant_interaction_controller.h"
+#include "base/logging.h"
 #include "chromeos/components/bloom/bloom_interaction.h"
 #include "chromeos/components/bloom/screenshot_grabber.h"
 
@@ -13,13 +13,10 @@ namespace bloom {
 
 BloomControllerImpl::BloomControllerImpl(
     signin::IdentityManager* identity_manager,
-    ash::AssistantInteractionController* assistant_interaction_controller,
     std::unique_ptr<ScreenshotGrabber> screenshot_grabber)
     : identity_manager_(identity_manager),
-      assistant_interaction_controller_(assistant_interaction_controller),
       screenshot_grabber_(std::move(screenshot_grabber)) {
   DCHECK(identity_manager_);
-  DCHECK(assistant_interaction_controller_);
   DCHECK(screenshot_grabber_);
 }
 
@@ -32,10 +29,14 @@ void BloomControllerImpl::StartInteraction() {
 
   current_interaction_ = std::make_unique<BloomInteraction>(this);
   current_interaction_->Start();
+
+  for (auto& observer : interaction_observers_)
+    observer.OnInteractionStarted();
 }
 
-bool BloomControllerImpl::HasInteraction() const {
-  return current_interaction_ != nullptr;
+void BloomControllerImpl::ShowUI() {
+  for (auto& observer : interaction_observers_)
+    observer.OnShowUI();
 }
 
 void BloomControllerImpl::StopInteraction(
@@ -46,12 +47,20 @@ void BloomControllerImpl::StopInteraction(
            << ToString(resolution);
 
   current_interaction_ = nullptr;
-  last_interaction_resolution_ = resolution;
+
+  for (auto& observer : interaction_observers_)
+    observer.OnInteractionFinished(resolution);
+}
+
+void BloomControllerImpl::AddObserver(BloomInteractionObserver* observer) {
+  DCHECK(observer);
+  interaction_observers_.AddObserver(observer);
 }
 
-BloomInteractionResolution BloomControllerImpl::GetLastInteractionResolution()
-    const {
-  return last_interaction_resolution_;
+void BloomControllerImpl::AddObserver(
+    std::unique_ptr<BloomInteractionObserver> observer) {
+  AddObserver(observer.get());
+  owned_interaction_observers_.push_back(std::move(observer));
 }
 
 void BloomControllerImpl::SetScreenshotGrabberForTesting(

chromeos/components/bloom/bloom_controller_impl.h

@@ -5,11 +5,12 @@
 #ifndef CHROMEOS_COMPONENTS_BLOOM_BLOOM_CONTROLLER_IMPL_H_
 #define CHROMEOS_COMPONENTS_BLOOM_BLOOM_CONTROLLER_IMPL_H_
 
-#include "chromeos/components/bloom/public/cpp/bloom_controller.h"
+#include <memory>
+#include <vector>
 
-namespace ash {
-class AssistantInteractionController;
-}  // namespace ash
+#include "base/observer_list.h"
+#include "chromeos/components/bloom/bloom_interaction_observer.h"
+#include "chromeos/components/bloom/public/cpp/bloom_controller.h"
 
 namespace signin {
 class IdentityManager;
@@ -23,37 +24,35 @@ class ScreenshotGrabber;
 
 class BloomControllerImpl : public BloomController {
  public:
-  BloomControllerImpl(
-      signin::IdentityManager* identity_manager,
-      ash::AssistantInteractionController* assistant_interaction_controller,
-      std::unique_ptr<ScreenshotGrabber> screenshot_grabber);
+  BloomControllerImpl(signin::IdentityManager* identity_manager,
+                      std::unique_ptr<ScreenshotGrabber> screenshot_grabber);
   BloomControllerImpl(const BloomControllerImpl&) = delete;
   BloomControllerImpl& operator=(const BloomControllerImpl&) = delete;
   ~BloomControllerImpl() override;
 
   // BloomController implementation:
   void StartInteraction() override;
-  BloomInteractionResolution GetLastInteractionResolution() const override;
+  void StopInteraction(BloomInteractionResolution resolution) override;
 
-  bool HasInteraction() const;
-  void StopInteraction(BloomInteractionResolution resolution);
+  void AddObserver(BloomInteractionObserver* observer) override;
+  void AddObserver(std::unique_ptr<BloomInteractionObserver> observer) override;
+
+  void ShowUI();
 
   ScreenshotGrabber* screenshot_grabber() { return screenshot_grabber_.get(); }
   signin::IdentityManager* identity_manager() { return identity_manager_; }
-  ash::AssistantInteractionController* assistant_interaction_controller() {
-    return assistant_interaction_controller_;
-  }
 
   void SetScreenshotGrabberForTesting(std::unique_ptr<ScreenshotGrabber>);
 
  private:
   signin::IdentityManager* const identity_manager_;
-  ash::AssistantInteractionController* const assistant_interaction_controller_;
   std::unique_ptr<ScreenshotGrabber> screenshot_grabber_;
 
+  base::ObserverList<BloomInteractionObserver> interaction_observers_;
+  std::vector<std::unique_ptr<BloomInteractionObserver>>
+      owned_interaction_observers_;
+
   std::unique_ptr<BloomInteraction> current_interaction_;
-  BloomInteractionResolution last_interaction_resolution_ =
-      BloomInteractionResolution::kNormal;
 };
 
 }  // namespace bloom

chromeos/components/bloom/bloom_controller_impl_unittest.cc

@@ -3,8 +3,8 @@
 // found in the LICENSE file.
 
 #include "chromeos/components/bloom/bloom_controller_impl.h"
-#include "ash/public/cpp/assistant/controller/assistant_interaction_controller.h"
 #include "base/test/task_environment.h"
+#include "chromeos/components/bloom/bloom_interaction_observer.h"
 #include "chromeos/components/bloom/public/cpp/bloom_interaction_resolution.h"
 #include "chromeos/components/bloom/screenshot_grabber.h"
 #include "chromeos/services/assistant/public/shared/constants.h"
@@ -53,24 +53,67 @@ class ScreenshotGrabberMock : public ScreenshotGrabber {
   Callback callback_;
 };
 
-class AssistantInteractionControllerMock
-    : public ash::AssistantInteractionController {
+class BloomInteractionObserverMock : public BloomInteractionObserver {
  public:
-  MOCK_METHOD(const ash::AssistantInteractionModel*, GetModel, (), (const));
-  MOCK_METHOD(base::TimeDelta, GetTimeDeltaSinceLastInteraction, (), (const));
-  MOCK_METHOD(bool, HasHadInteraction, (), (const));
+  MOCK_METHOD(void, OnInteractionStarted, ());
+  MOCK_METHOD(void, OnShowUI, ());
+  MOCK_METHOD(void, OnShowResult, (const std::string& html));
   MOCK_METHOD(void,
-              StartTextInteraction,
-              (const std::string& query,
-               bool allow_tts,
-               chromeos::assistant::AssistantQuerySource source));
-  MOCK_METHOD(void, StartBloomInteraction, ());
+              OnInteractionFinished,
+              (BloomInteractionResolution resolution));
+};
+
+// Helper class that will track the state of the Bloom interaction,
+// So we can easily test if interactions are started/stopped correctly.
+class BloomInteractionTracker : public BloomInteractionObserver {
+ public:
+  void OnInteractionStarted() override {
+    EXPECT_FALSE(has_interaction_);
+    has_interaction_ = true;
+  }
+
+  void OnShowUI() override { EXPECT_TRUE(has_interaction_); }
+
+  void OnShowResult(const std::string& html) override {
+    EXPECT_TRUE(has_interaction_);
+  }
+
+  void OnInteractionFinished(BloomInteractionResolution resolution) override {
+    EXPECT_TRUE(has_interaction_);
+    has_interaction_ = false;
+    last_resolution_ = resolution;
+  }
+
+  bool HasInteraction() const { return has_interaction_; }
+  BloomInteractionResolution GetLastInteractionResolution() const {
+    return last_resolution_;
+  }
+
+ private:
+  bool has_interaction_ = false;
+  BloomInteractionResolution last_resolution_ =
+      BloomInteractionResolution::kNormal;
 };
 
 #define EXPECT_NO_CALLS(args...) EXPECT_CALL(args).Times(0)
 
 }  // namespace
 
+void PrintTo(const BloomInteractionResolution& value, std::ostream* output) {
+#define CASE(name)                         \
+  ({                                       \
+    case BloomInteractionResolution::name: \
+      *output << #name;                    \
+      break;                               \
+  })
+
+  switch (value) {
+    CASE(kNormal);
+    CASE(kNoScreenshot);
+    CASE(kNoAccessToken);
+  }
+}
+
 class BloomControllerImplTest : public testing::Test {
  public:
   void SetUp() override {
@@ -99,8 +142,18 @@ class BloomControllerImplTest : public testing::Test {
         controller_.screenshot_grabber());
   }
 
-  AssistantInteractionControllerMock& assistant_interaction_controller_mock() {
-    return assistant_interaction_controller_mock_;
+  BloomInteractionObserverMock* AddInteractionObserverMock() {
+    auto observer = std::make_unique<NiceMock<BloomInteractionObserverMock>>();
+    auto* raw_ptr = observer.get();
+    controller().AddObserver(std::move(observer));
+    return raw_ptr;
+  }
+
+  BloomInteractionTracker* AddInteractionTracker() {
+    auto observer = std::make_unique<BloomInteractionTracker>();
+    auto* raw_ptr = observer.get();
+    controller().AddObserver(std::move(observer));
+    return raw_ptr;
   }
 
   void IssueAccessToken(std::string token = std::string("<access-token>")) {
@@ -117,11 +170,8 @@ class BloomControllerImplTest : public testing::Test {
   base::test::TaskEnvironment task_env_;
   signin::IdentityTestEnvironment identity_test_env_;
 
-  AssistantInteractionControllerMock assistant_interaction_controller_mock_;
-
   BloomControllerImpl controller_{
       identity_test_env_.identity_manager(),
-      &assistant_interaction_controller_mock_,
       std::make_unique<NiceMock<ScreenshotGrabberMock>>()};
 };
 
@@ -145,59 +195,80 @@ TEST_F(BloomControllerImplTest, ShouldTakeScreenshot) {
   controller().StartInteraction();
 }
 
-TEST_F(BloomControllerImplTest,
-       ShouldStartAssistantInteractionWhenAccessTokenAndScreenshotAreReady) {
-  EXPECT_CALL(assistant_interaction_controller_mock(), StartBloomInteraction);
+TEST_F(BloomControllerImplTest, ShouldAbortWhenFetchingAccessTokenFails) {
+  auto* interaction_tracker = AddInteractionTracker();
 
   controller().StartInteraction();
-  IssueAccessToken();
   screenshot_grabber_mock().SendScreenshot();
+
+  FailAccessToken();
+
+  EXPECT_FALSE(interaction_tracker->HasInteraction());
+  EXPECT_EQ(BloomInteractionResolution::kNoAccessToken,
+            interaction_tracker->GetLastInteractionResolution());
 }
 
-TEST_F(BloomControllerImplTest,
-       ShouldNotStartAssistantInteractionBeforeAccessTokenArrives) {
-  EXPECT_NO_CALLS(assistant_interaction_controller_mock(),
-                  StartBloomInteraction);
+TEST_F(BloomControllerImplTest, ShouldAbortWhenFetchingScreenshotFails) {
+  auto* interaction_tracker = AddInteractionTracker();
 
   controller().StartInteraction();
-  screenshot_grabber_mock().SendScreenshot();
+  IssueAccessToken();
+
+  screenshot_grabber_mock().SendScreenshotFailed();
+
+  EXPECT_FALSE(interaction_tracker->HasInteraction());
+  EXPECT_EQ(BloomInteractionResolution::kNoScreenshot,
+            interaction_tracker->GetLastInteractionResolution());
 }
 
-TEST_F(BloomControllerImplTest,
-       ShouldNotStartAssistantInteractionBeforeScreenshotArrives) {
-  EXPECT_NO_CALLS(assistant_interaction_controller_mock(),
-                  StartBloomInteraction);
+////////////////////////////////////////////////////////////////////////////////
+///
+/// Below are the tests to ensure the BloomInteractionObserver is invoked.
+///
+////////////////////////////////////////////////////////////////////////////////
+
+using BloomInteractionObserverTest = BloomControllerImplTest;
+
+TEST_F(BloomInteractionObserverTest,
+       ShouldCallOnInteractionStartedWhenBloomInteractionStarts) {
+  auto* observer = AddInteractionObserverMock();
+  EXPECT_CALL(*observer, OnInteractionStarted);
 
   controller().StartInteraction();
-  IssueAccessToken();
 }
 
-TEST_F(BloomControllerImplTest, ShouldAbortWhenFetchingAccessTokenFails) {
-  EXPECT_NO_CALLS(assistant_interaction_controller_mock(),
-                  StartBloomInteraction);
+TEST_F(BloomInteractionObserverTest,
+       ShouldCallOnShowUIWhenAccessTokenAndScreenshotAreReady) {
+  auto* observer = AddInteractionObserverMock();
+  EXPECT_CALL(*observer, OnInteractionStarted);
+
+  EXPECT_CALL(*observer, OnShowUI);
 
   controller().StartInteraction();
+  IssueAccessToken();
   screenshot_grabber_mock().SendScreenshot();
+}
 
-  FailAccessToken();
+TEST_F(BloomInteractionObserverTest,
+       ShouldNotCallOnShowUIBeforeAccessTokenArrives) {
+  auto* observer = AddInteractionObserverMock();
+  EXPECT_CALL(*observer, OnInteractionStarted);
 
-  EXPECT_FALSE(controller().HasInteraction());
-  EXPECT_EQ(BloomInteractionResolution::kNoAccessToken,
-            controller().GetLastInteractionResolution());
+  EXPECT_NO_CALLS(*observer, OnShowUI);
+
+  controller().StartInteraction();
+  screenshot_grabber_mock().SendScreenshot();
 }
 
-TEST_F(BloomControllerImplTest, ShouldAbortWhenFetchingScreenshotFails) {
-  EXPECT_NO_CALLS(assistant_interaction_controller_mock(),
-                  StartBloomInteraction);
+TEST_F(BloomInteractionObserverTest,
+       ShouldNotCallOnShowUIBeforeScreenshotArrives) {
+  auto* observer = AddInteractionObserverMock();
+  EXPECT_CALL(*observer, OnInteractionStarted);
+
+  EXPECT_NO_CALLS(*observer, OnShowUI);
 
   controller().StartInteraction();
   IssueAccessToken();
-
-  screenshot_grabber_mock().SendScreenshotFailed();
-
-  EXPECT_FALSE(controller().HasInteraction());
-  EXPECT_EQ(BloomInteractionResolution::kNoScreenshot,
-            controller().GetLastInteractionResolution());
 }
 
 }  // namespace bloom

chromeos/components/bloom/bloom_interaction.cc

@@ -4,7 +4,6 @@
 
 #include "chromeos/components/bloom/bloom_interaction.h"
 
-#include "ash/public/cpp/assistant/controller/assistant_interaction_controller.h"
 #include "base/bind.h"
 #include "base/memory/weak_ptr.h"
 #include "chromeos/components/bloom/bloom_controller_impl.h"
@@ -34,7 +33,7 @@ void BloomInteraction::Start() {
 
 void BloomInteraction::StartAssistantInteraction(std::string&& access_token,
                                                  Screenshot&& screenshot) {
-  controller_->assistant_interaction_controller()->StartBloomInteraction();
+  controller_->ShowUI();
   // TODO(jeroendh): continue here by contacting the Bloom service.
 }
 

chromeos/components/bloom/bloom_interaction_observer.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 CHROMEOS_COMPONENTS_BLOOM_BLOOM_INTERACTION_OBSERVER_H_
+#define CHROMEOS_COMPONENTS_BLOOM_BLOOM_INTERACTION_OBSERVER_H_
+
+#include "base/observer_list_types.h"
+#include "chromeos/components/bloom/public/cpp/bloom_interaction_resolution.h"
+
+namespace chromeos {
+namespace bloom {
+
+// Observer of the Bloom interaction events.
+class BloomInteractionObserver : public base::CheckedObserver {
+ public:
+  ~BloomInteractionObserver() override = default;
+
+  virtual void OnInteractionStarted() = 0;
+
+  // Called when the Assistant UI should be shown.
+  virtual void OnShowUI() = 0;
+
+  // Called when the result is ready.
+  virtual void OnShowResult(const std::string& html) = 0;
+
+  virtual void OnInteractionFinished(BloomInteractionResolution resolution) = 0;
+};
+
+}  // namespace bloom
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_BLOOM_BLOOM_INTERACTION_OBSERVER_H_

chromeos/components/bloom/bloom_interaction_observer_impl.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 "chromeos/components/bloom/bloom_interaction_observer_impl.h"
+#include "ash/public/cpp/assistant/controller/assistant_interaction_controller.h"
+#include "base/check.h"
+
+namespace chromeos {
+namespace bloom {
+
+BloomInteractionObserverImpl::BloomInteractionObserverImpl(
+    ash::AssistantInteractionController* assistant_interaction_controller)
+    : assistant_interaction_controller_(assistant_interaction_controller) {
+  DCHECK(assistant_interaction_controller_);
+}
+
+BloomInteractionObserverImpl::~BloomInteractionObserverImpl() = default;
+
+void BloomInteractionObserverImpl::OnInteractionStarted() {
+  assistant_interaction_controller_->StartBloomInteraction();
+}
+
+void BloomInteractionObserverImpl::OnShowUI() {
+  // TODO(jeroendh): implement
+}
+
+void BloomInteractionObserverImpl::OnShowResult(const std::string& html) {
+  // TODO(jeroendh): implement
+}
+
+void BloomInteractionObserverImpl::OnInteractionFinished(
+    BloomInteractionResolution resolution) {
+  // TODO(jeroendh): implement
+}
+
+}  // namespace bloom
+}  // namespace chromeos

chromeos/components/bloom/bloom_interaction_observer_impl.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 CHROMEOS_COMPONENTS_BLOOM_BLOOM_INTERACTION_OBSERVER_IMPL_H_
+#define CHROMEOS_COMPONENTS_BLOOM_BLOOM_INTERACTION_OBSERVER_IMPL_H_
+
+#include "chromeos/components/bloom/bloom_interaction_observer.h"
+
+namespace ash {
+class AssistantInteractionController;
+}  // namespace ash
+
+namespace chromeos {
+namespace bloom {
+
+// Main observer of the Bloom interaction events.
+// Will trigger the Assistant service so it can open/close the UI and display
+// all bloom responses.
+class BloomInteractionObserverImpl : public BloomInteractionObserver {
+ public:
+  explicit BloomInteractionObserverImpl(
+      ash::AssistantInteractionController* assistant_interaction_controller);
+  ~BloomInteractionObserverImpl() override;
+
+  // BloomInteractionObserver implementation:
+  void OnInteractionStarted() override;
+  void OnShowUI() override;
+  void OnShowResult(const std::string& html) override;
+  void OnInteractionFinished(BloomInteractionResolution resolution) override;
+
+ private:
+  ash::AssistantInteractionController* assistant_interaction_controller_;
+};
+
+}  // namespace bloom
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_BLOOM_BLOOM_INTERACTION_OBSERVER_IMPL_H_

chromeos/components/bloom/public/cpp/bloom_controller.h

@@ -13,6 +13,8 @@
 namespace chromeos {
 namespace bloom {
 
+class BloomInteractionObserver;
+
 // Main controller for the Bloom integration.
 class COMPONENT_EXPORT(BLOOM) BloomController {
  public:
@@ -27,9 +29,11 @@ class COMPONENT_EXPORT(BLOOM) BloomController {
   // Starts an interaction. This will ask the user for a screenshot, analyze the
   // content and display the result.
   virtual void StartInteraction() = 0;
+  virtual void StopInteraction(BloomInteractionResolution resolution) = 0;
 
-  // Returns the result of the last interaction.
-  virtual BloomInteractionResolution GetLastInteractionResolution() const = 0;
+  virtual void AddObserver(BloomInteractionObserver* observer) = 0;
+  virtual void AddObserver(
+      std::unique_ptr<BloomInteractionObserver> observer) = 0;
 };
 
 }  // namespace bloom

chromeos/components/bloom/public/cpp/bloom_controller_factory.cc

@@ -6,6 +6,7 @@
 
 #include "base/callback.h"
 #include "chromeos/components/bloom/bloom_controller_impl.h"
+#include "chromeos/components/bloom/bloom_interaction_observer_impl.h"
 #include "chromeos/components/bloom/screenshot_grabber.h"
 
 namespace chromeos {
@@ -26,9 +27,11 @@ std::unique_ptr<BloomController> BloomControllerFactory::Create(
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     signin::IdentityManager* identity_manager,
     ash::AssistantInteractionController* assistant_interaction_controller) {
-  return std::make_unique<BloomControllerImpl>(
-      identity_manager, assistant_interaction_controller,
-      std::make_unique<FakeScreenshotGrabber>());
+  auto result = std::make_unique<BloomControllerImpl>(
+      identity_manager, std::make_unique<FakeScreenshotGrabber>());
+  result->AddObserver(std::make_unique<BloomInteractionObserverImpl>(
+      assistant_interaction_controller));
+  return std::move(result);
 }
 
 }  // namespace bloom