Finish BloomController by adding BloomServerProxy

|BloomServerProxy| will handle all communication with the Bloom servers.

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

chromeos/components/bloom/BUILD.gn

@@ -11,6 +11,9 @@ source_set("bloom") {
     "bloom_interaction_observer.h",
     "bloom_interaction_observer_impl.cc",
     "bloom_interaction_observer_impl.h",
+    "bloom_server_proxy.h",
+    "bloom_server_proxy_impl.cc",
+    "bloom_server_proxy_impl.h",
     "screenshot_grabber.h",
   ]
 

chromeos/components/bloom/bloom_controller_impl.cc

@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "chromeos/components/bloom/bloom_interaction.h"
+#include "chromeos/components/bloom/bloom_server_proxy.h"
 #include "chromeos/components/bloom/screenshot_grabber.h"
 
 namespace chromeos {
@@ -13,11 +14,14 @@ namespace bloom {
 
 BloomControllerImpl::BloomControllerImpl(
     signin::IdentityManager* identity_manager,
-    std::unique_ptr<ScreenshotGrabber> screenshot_grabber)
+    std::unique_ptr<ScreenshotGrabber> screenshot_grabber,
+    std::unique_ptr<BloomServerProxy> server_proxy)
     : identity_manager_(identity_manager),
-      screenshot_grabber_(std::move(screenshot_grabber)) {
+      screenshot_grabber_(std::move(screenshot_grabber)),
+      server_proxy_(std::move(server_proxy)) {
   DCHECK(identity_manager_);
   DCHECK(screenshot_grabber_);
+  DCHECK(server_proxy_);
 }
 
 BloomControllerImpl::~BloomControllerImpl() = default;
@@ -39,6 +43,11 @@ void BloomControllerImpl::ShowUI() {
     observer.OnShowUI();
 }
 
+void BloomControllerImpl::ShowResult(const std::string& result) {
+  for (auto& observer : interaction_observers_)
+    observer.OnShowResult(result);
+}
+
 void BloomControllerImpl::StopInteraction(
     BloomInteractionResolution resolution) {
   DCHECK(current_interaction_);

chromeos/components/bloom/bloom_controller_impl.h

@@ -20,12 +20,14 @@ namespace chromeos {
 namespace bloom {
 
 class BloomInteraction;
+class BloomServerProxy;
 class ScreenshotGrabber;
 
 class BloomControllerImpl : public BloomController {
  public:
   BloomControllerImpl(signin::IdentityManager* identity_manager,
-                      std::unique_ptr<ScreenshotGrabber> screenshot_grabber);
+                      std::unique_ptr<ScreenshotGrabber> screenshot_grabber,
+                      std::unique_ptr<BloomServerProxy> server_proxy);
   BloomControllerImpl(const BloomControllerImpl&) = delete;
   BloomControllerImpl& operator=(const BloomControllerImpl&) = delete;
   ~BloomControllerImpl() override;
@@ -38,8 +40,10 @@ class BloomControllerImpl : public BloomController {
   void AddObserver(std::unique_ptr<BloomInteractionObserver> observer) override;
 
   void ShowUI();
+  void ShowResult(const std::string& result);
 
   ScreenshotGrabber* screenshot_grabber() { return screenshot_grabber_.get(); }
+  BloomServerProxy* server_proxy() { return server_proxy_.get(); }
   signin::IdentityManager* identity_manager() { return identity_manager_; }
 
   void SetScreenshotGrabberForTesting(std::unique_ptr<ScreenshotGrabber>);
@@ -47,6 +51,7 @@ class BloomControllerImpl : public BloomController {
  private:
   signin::IdentityManager* const identity_manager_;
   std::unique_ptr<ScreenshotGrabber> screenshot_grabber_;
+  std::unique_ptr<BloomServerProxy> server_proxy_;
 
   base::ObserverList<BloomInteractionObserver> interaction_observers_;
   std::vector<std::unique_ptr<BloomInteractionObserver>>

chromeos/components/bloom/bloom_controller_impl_unittest.cc

@@ -5,6 +5,7 @@
 #include "chromeos/components/bloom/bloom_controller_impl.h"
 #include "base/test/task_environment.h"
 #include "chromeos/components/bloom/bloom_interaction_observer.h"
+#include "chromeos/components/bloom/bloom_server_proxy.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"
@@ -19,11 +20,14 @@ namespace bloom {
 
 namespace {
 
+using ::testing::_;
 using ::testing::AnyNumber;
 using ::testing::NiceMock;
 
 const char kEmail[] = "test@gmail.com";
 
+#define EXPECT_NO_CALLS(args...) EXPECT_CALL(args).Times(0)
+
 class ScreenshotGrabberMock : public ScreenshotGrabber {
  public:
   ScreenshotGrabberMock() {
@@ -95,7 +99,35 @@ class BloomInteractionTracker : public BloomInteractionObserver {
       BloomInteractionResolution::kNormal;
 };
 
-#define EXPECT_NO_CALLS(args...) EXPECT_CALL(args).Times(0)
+class BloomServerProxyMock : public BloomServerProxy {
+ public:
+  BloomServerProxyMock() {
+    ON_CALL(*this, AnalyzeProblem)
+        .WillByDefault([this](const std::string& access_token,
+                              const Screenshot screenshot, Callback callback) {
+          // Store the callback passed to AnalyzeProblem() so we can invoke it
+          // later from our tests.
+          this->callback_ = std::move(callback);
+        });
+  }
+  MOCK_METHOD(void,
+              AnalyzeProblem,
+              (const std::string& access_token,
+               const Screenshot screenshot,
+               Callback callback));
+
+  void SendResponse(base::Optional<std::string> html = std::string("<html/>")) {
+    EXPECT_TRUE(callback_) << "AnalyzeProblem() was never called.";
+
+    if (callback_)
+      std::move(callback_).Run(html);
+  }
+
+  void SendResponseFailure() { SendResponse(base::nullopt); }
+
+ private:
+  Callback callback_;
+};
 
 }  // namespace
 
@@ -111,6 +143,7 @@ void PrintTo(const BloomInteractionResolution& value, std::ostream* output) {
     CASE(kNormal);
     CASE(kNoScreenshot);
     CASE(kNoAccessToken);
+    CASE(kServerError);
   }
 }
 
@@ -121,6 +154,14 @@ class BloomControllerImplTest : public testing::Test {
   }
 
  protected:
+  void StartInteractionAndSendAccessTokenAndScreenshot(
+      std::string access_token = "<access-token>",
+      Screenshot screenshot = Screenshot()) {
+    controller().StartInteraction();
+    IssueAccessToken(access_token);
+    screenshot_grabber_mock().SendScreenshot(screenshot);
+  }
+
   // Returns the |ScopeSet| that was passed to the access token request.
   // Fails the test and returns an empty |ScopeSet| if there were no access
   // token requests.
@@ -156,6 +197,10 @@ class BloomControllerImplTest : public testing::Test {
     return raw_ptr;
   }
 
+  BloomServerProxyMock& bloom_server() {
+    return *static_cast<BloomServerProxyMock*>(controller_.server_proxy());
+  }
+
   void IssueAccessToken(std::string token = std::string("<access-token>")) {
     identity_test_env_.WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
         token, /*expiration=*/base::Time::Max());
@@ -172,7 +217,8 @@ class BloomControllerImplTest : public testing::Test {
 
   BloomControllerImpl controller_{
       identity_test_env_.identity_manager(),
-      std::make_unique<NiceMock<ScreenshotGrabberMock>>()};
+      std::make_unique<NiceMock<ScreenshotGrabberMock>>(),
+      std::make_unique<NiceMock<BloomServerProxyMock>>()};
 };
 
 TEST_F(BloomControllerImplTest, ShouldBeReturnedWhenCallingGet) {
@@ -221,6 +267,27 @@ TEST_F(BloomControllerImplTest, ShouldAbortWhenFetchingScreenshotFails) {
             interaction_tracker->GetLastInteractionResolution());
 }
 
+TEST_F(BloomControllerImplTest,
+       ShouldPassAccessTokenAndScreenshotToBloomServer) {
+  const std::string& access_token = "<the-access-token>";
+  const Screenshot screenshot{1, 2, 3, 4, 5};
+
+  EXPECT_CALL(bloom_server(), AnalyzeProblem(access_token, screenshot, _));
+
+  StartInteractionAndSendAccessTokenAndScreenshot(access_token, screenshot);
+}
+
+TEST_F(BloomControllerImplTest, ShouldAbortWhenBloomServerFails) {
+  auto* interaction_tracker = AddInteractionTracker();
+  StartInteractionAndSendAccessTokenAndScreenshot();
+
+  bloom_server().SendResponseFailure();
+
+  EXPECT_FALSE(interaction_tracker->HasInteraction());
+  EXPECT_EQ(BloomInteractionResolution::kServerError,
+            interaction_tracker->GetLastInteractionResolution());
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 ///
 /// Below are the tests to ensure the BloomInteractionObserver is invoked.
@@ -240,7 +307,6 @@ TEST_F(BloomInteractionObserverTest,
 TEST_F(BloomInteractionObserverTest,
        ShouldCallOnShowUIWhenAccessTokenAndScreenshotAreReady) {
   auto* observer = AddInteractionObserverMock();
-  EXPECT_CALL(*observer, OnInteractionStarted);
 
   EXPECT_CALL(*observer, OnShowUI);
 
@@ -252,7 +318,6 @@ TEST_F(BloomInteractionObserverTest,
 TEST_F(BloomInteractionObserverTest,
        ShouldNotCallOnShowUIBeforeAccessTokenArrives) {
   auto* observer = AddInteractionObserverMock();
-  EXPECT_CALL(*observer, OnInteractionStarted);
 
   EXPECT_NO_CALLS(*observer, OnShowUI);
 
@@ -263,7 +328,6 @@ TEST_F(BloomInteractionObserverTest,
 TEST_F(BloomInteractionObserverTest,
        ShouldNotCallOnShowUIBeforeScreenshotArrives) {
   auto* observer = AddInteractionObserverMock();
-  EXPECT_CALL(*observer, OnInteractionStarted);
 
   EXPECT_NO_CALLS(*observer, OnShowUI);
 
@@ -271,5 +335,14 @@ TEST_F(BloomInteractionObserverTest,
   IssueAccessToken();
 }
 
+TEST_F(BloomInteractionObserverTest, ShouldForwardServerResponse) {
+  auto* observer = AddInteractionObserverMock();
+  StartInteractionAndSendAccessTokenAndScreenshot();
+
+  EXPECT_CALL(*observer, OnShowResult("<html>response</html>"));
+
+  bloom_server().SendResponse("<html>response</html>");
+}
+
 }  // namespace bloom
 }  // namespace chromeos

chromeos/components/bloom/bloom_interaction.cc

@@ -8,6 +8,7 @@
 #include "base/memory/weak_ptr.h"
 #include "chromeos/components/bloom/bloom_controller_impl.h"
 #include "chromeos/components/bloom/bloom_interaction.h"
+#include "chromeos/components/bloom/bloom_server_proxy.h"
 #include "chromeos/components/bloom/public/cpp/future_value.h"
 #include "chromeos/components/bloom/screenshot_grabber.h"
 #include "chromeos/services/assistant/public/shared/constants.h"
@@ -34,7 +35,17 @@ void BloomInteraction::Start() {
 void BloomInteraction::StartAssistantInteraction(std::string&& access_token,
                                                  Screenshot&& screenshot) {
   controller_->ShowUI();
-  // TODO(jeroendh): continue here by contacting the Bloom service.
+  controller_->server_proxy()->AnalyzeProblem(
+      access_token, screenshot, Bind(&BloomInteraction::OnServerResponse));
+}
+
+void BloomInteraction::OnServerResponse(base::Optional<std::string> html) {
+  if (!html) {
+    controller_->StopInteraction(BloomInteractionResolution ::kServerError);
+    return;
+  }
+
+  controller_->ShowResult(html.value());
 }
 
 void BloomInteraction::FetchAccessTokenAsync() {

chromeos/components/bloom/bloom_interaction.h

@@ -8,6 +8,7 @@
 #include <string>
 #include "base/bind.h"
 #include "base/callback_forward.h"
+#include "base/optional.h"
 #include "chromeos/components/bloom/screenshot_grabber.h"
 
 class GoogleServiceAuthError;
@@ -48,6 +49,8 @@ class BloomInteraction {
   void StartAssistantInteraction(std::string&& access_token,
                                  Screenshot&& screenshot);
 
+  void OnServerResponse(base::Optional<std::string> html);
+
   void FetchAccessTokenAsync();
   void FetchScreenshotAsync();
 

chromeos/components/bloom/bloom_server_proxy.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_SERVER_PROXY_H_
+#define CHROMEOS_COMPONENTS_BLOOM_BLOOM_SERVER_PROXY_H_
+
+#include <string>
+#include "base/callback.h"
+#include "chromeos/components/bloom/screenshot_grabber.h"
+
+namespace chromeos {
+namespace bloom {
+
+// Local object that handles all communication with the Bloom servers.
+class BloomServerProxy {
+ public:
+  using Callback =
+      base::OnceCallback<void(base::Optional<std::string> response)>;
+
+  BloomServerProxy() = default;
+  BloomServerProxy(BloomServerProxy&) = delete;
+  BloomServerProxy& operator=(BloomServerProxy&) = delete;
+  virtual ~BloomServerProxy() = default;
+
+  // Send the screenshot to the Bloom server for analysis, and send the response
+  // to |callback|.
+  virtual void AnalyzeProblem(const std::string& access_token,
+                              const Screenshot screenshot,
+                              Callback callback) = 0;
+};
+
+}  // namespace bloom
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_BLOOM_BLOOM_SERVER_PROXY_H_

chromeos/components/bloom/bloom_server_proxy_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_server_proxy_impl.h"
+
+namespace chromeos {
+namespace bloom {
+
+BloomServerProxyImpl::BloomServerProxyImpl() = default;
+BloomServerProxyImpl::~BloomServerProxyImpl() = default;
+
+void BloomServerProxyImpl::AnalyzeProblem(const std::string& access_token,
+                                          const Screenshot screenshot,
+                                          Callback callback) {
+  // TODO(jeroendh): implement
+}
+
+}  // namespace bloom
+}  // namespace chromeos

chromeos/components/bloom/bloom_server_proxy_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_SERVER_PROXY_IMPL_H_
+#define CHROMEOS_COMPONENTS_BLOOM_BLOOM_SERVER_PROXY_IMPL_H_
+
+#include "chromeos/components/bloom/bloom_server_proxy.h"
+
+namespace chromeos {
+namespace bloom {
+
+class BloomServerProxyImpl : public BloomServerProxy {
+ public:
+  BloomServerProxyImpl();
+  ~BloomServerProxyImpl() override;
+
+  void AnalyzeProblem(const std::string& access_token,
+                      const Screenshot screenshot,
+                      Callback callback) override;
+};
+
+}  // namespace bloom
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_BLOOM_BLOOM_SERVER_PROXY_IMPL_H_

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

@@ -4,9 +4,12 @@
 
 #include "chromeos/components/bloom/public/cpp/bloom_controller_factory.h"
 
+#include <memory>
+
 #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/bloom_server_proxy_impl.h"
 #include "chromeos/components/bloom/screenshot_grabber.h"
 
 namespace chromeos {
@@ -28,9 +31,12 @@ std::unique_ptr<BloomController> BloomControllerFactory::Create(
     signin::IdentityManager* identity_manager,
     ash::AssistantInteractionController* assistant_interaction_controller) {
   auto result = std::make_unique<BloomControllerImpl>(
-      identity_manager, std::make_unique<FakeScreenshotGrabber>());
+      identity_manager, std::make_unique<FakeScreenshotGrabber>(),
+      std::make_unique<BloomServerProxyImpl>());
+
   result->AddObserver(std::make_unique<BloomInteractionObserverImpl>(
       assistant_interaction_controller));
+
   return std::move(result);
 }
 

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

@@ -16,6 +16,7 @@ std::string ToString(BloomInteractionResolution resolution) {
     CASE(kNormal);
     CASE(kNoAccessToken);
     CASE(kNoScreenshot);
+    CASE(kServerError);
   }
 }
 

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

@@ -16,10 +16,12 @@ enum class BloomInteractionResolution {
   // Bloom interaction completed normally.
   kNormal = 0,
   // Bloom interaction failed to fetch an access token.
-  kNoAccessToken = 1,
+  kNoAccessToken,
   // Bloom interaction failed to take a screenshot
   // (or the user aborted while taking a screenshot).
-  kNoScreenshot = 2,
+  kNoScreenshot,
+  // Bloom server returned an error.
+  kServerError,
 };
 
 std::string COMPONENT_EXPORT(BLOOM)