Switched ChromiumHttpConnection to use network service.

Switches chromeos::assistant::ChromiumHttpConnection from using
//net directly to using the network service (SimpleURLLoader).

Bug: 913757
Change-Id: If89b2eb49f66a0279d9b23c1533c15166c322110
Reviewed-on: https://chromium-review.googlesource.com/c/1380541Reviewed-by: Leo Zhang <googleo@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: Chris Mumford <cmumford@google.com>
Cr-Commit-Position: refs/heads/master@{#630006}

chrome/browser/profiles/profile_impl.cc

@@ -1322,8 +1322,7 @@ std::unique_ptr<service_manager::Service> ProfileImpl::HandleServiceRequest(
   if (service_name == chromeos::assistant::mojom::kServiceName) {
     return std::make_unique<chromeos::assistant::Service>(
         std::move(request), content::GetNetworkConnectionTracker(),
-        base::CreateSingleThreadTaskRunnerWithTraits(
-            {content::BrowserThread::IO}));
+        GetURLLoaderFactory()->Clone());
   }
 #endif  // BUILDFLAG(ENABLE_CROS_ASSISTANT)
 

chromeos/services/assistant/BUILD.gn

@@ -41,6 +41,7 @@ component("lib") {
     "//components/account_id",
     "//services/device/public/mojom",
     "//services/identity/public/mojom",
+    "//services/network/public/cpp:cpp",
     "//ui/accessibility:ax_assistant",
   ]
 
@@ -129,6 +130,8 @@ source_set("tests") {
     "//mojo/public/cpp/bindings:bindings",
     "//services/device/public/mojom",
     "//services/identity/public/mojom",
+    "//services/network:test_support",
+    "//services/network/public/cpp:cpp",
     "//services/service_manager/public/cpp",
     "//services/service_manager/public/cpp/test:test_support",
     "//testing/gmock",

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -34,6 +34,7 @@
 #include "libassistant/shared/internal_api/assistant_manager_delegate.h"
 #include "libassistant/shared/internal_api/assistant_manager_internal.h"
 #include "libassistant/shared/public/media_manager.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/service_manager/public/cpp/connector.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "url/gurl.h"
@@ -99,13 +100,15 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
     service_manager::Connector* connector,
     device::mojom::BatteryMonitorPtr battery_monitor,
     Service* service,
-    network::NetworkConnectionTracker* network_connection_tracker)
+    network::NetworkConnectionTracker* network_connection_tracker,
+    std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+        url_loader_factory_info)
     : media_session_(std::make_unique<AssistantMediaSession>(connector)),
       action_module_(std::make_unique<action::CrosActionModule>(
           this,
           assistant::features::IsAppSupportEnabled(),
           assistant::features::IsRoutinesEnabled())),
-      chromium_api_delegate_(service->io_task_runner()),
+      chromium_api_delegate_(std::move(url_loader_factory_info)),
       display_connection_(std::make_unique<CrosDisplayConnection>(this)),
       assistant_settings_manager_(
           std::make_unique<AssistantSettingsManagerImpl>(service, this)),

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -34,6 +34,10 @@ class AssistantManager;
 class AssistantManagerInternal;
 }  // namespace assistant_client
 
+namespace network {
+class SharedURLLoaderFactoryInfo;
+}  // namespace network
+
 namespace service_manager {
 class Connector;
 }  // namespace service_manager
@@ -85,7 +89,9 @@ class AssistantManagerServiceImpl
       service_manager::Connector* connector,
       device::mojom::BatteryMonitorPtr battery_monitor,
       Service* service,
-      network::NetworkConnectionTracker* network_connection_tracker);
+      network::NetworkConnectionTracker* network_connection_tracker,
+      std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+          url_loader_factory_info);
 
   ~AssistantManagerServiceImpl() override;
 

chromeos/services/assistant/chromium_api_delegate.cc

@@ -4,17 +4,18 @@
 
 #include "chromeos/services/assistant/chromium_api_delegate.h"
 
+#include <utility>
+
 #include "base/single_thread_task_runner.h"
-#include "chromeos/services/assistant/default_url_request_context_getter.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace chromeos {
 namespace assistant {
 
 ChromiumApiDelegate::ChromiumApiDelegate(
-    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
-    : http_connection_factory_(
-          base::MakeRefCounted<DefaultURLRequestContextGetter>(
-              io_task_runner)) {}
+    std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+        url_loader_factory_info)
+    : http_connection_factory_(std::move(url_loader_factory_info)) {}
 
 ChromiumApiDelegate::~ChromiumApiDelegate() = default;
 

chromeos/services/assistant/chromium_api_delegate.h

@@ -12,6 +12,10 @@
 #include "base/macros.h"
 #include "libassistant/shared/internal_api/fuchsia_api_helper.h"
 
+namespace network {
+class SharedURLLoaderFactoryInfo;
+}  // namespace network
+
 namespace chromeos {
 namespace assistant {
 
@@ -19,8 +23,9 @@ class ChromiumHttpConnectionFactory;
 
 class ChromiumApiDelegate : public assistant_client::FuchsiaApiDelegate {
  public:
-  ChromiumApiDelegate(
-      scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
+  explicit ChromiumApiDelegate(
+      std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+          url_loader_factory_info);
   ~ChromiumApiDelegate() override;
   // assistant_client::FuchsiaApiDelegate overrides:
   assistant_client::HttpConnectionFactory* GetHttpConnectionFactory() override;

chromeos/services/assistant/chromium_http_connection.h

@@ -2,8 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// The file comes from Google Home(cast) implementation.
-
 #ifndef CHROMEOS_SERVICES_ASSISTANT_CHROMIUM_HTTP_CONNECTION_H_
 #define CHROMEOS_SERVICES_ASSISTANT_CHROMIUM_HTTP_CONNECTION_H_
 
@@ -14,26 +12,33 @@
 
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/single_thread_task_runner.h"
+#include "base/sequenced_task_runner.h"
 #include "libassistant/shared/internal_api/http_connection.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
 #include "net/http/http_request_headers.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_fetcher_delegate.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/simple_url_loader_stream_consumer.h"
+#include "services/network/public/mojom/chunked_data_pipe_getter.mojom.h"
 #include "url/gurl.h"
 
+namespace network {
+class SimpleURLLoader;
+class SharedURLLoaderFactory;
+class SharedURLLoaderFactoryInfo;
+}  // namespace network
+
 namespace chromeos {
 namespace assistant {
 
-// Implements libassistant's HttpConnection using Chromium //net library.
+// Implements libassistant's HttpConnection.
 class ChromiumHttpConnection
     : public assistant_client::HttpConnection,
-      public ::net::URLFetcherDelegate,
+      public network::mojom::ChunkedDataPipeGetter,
+      public network::SimpleURLLoaderStreamConsumer,
       public base::RefCountedThreadSafe<ChromiumHttpConnection> {
  public:
-  ChromiumHttpConnection(
-      scoped_refptr<::net::URLRequestContextGetter> url_request_context_getter,
-      Delegate* delegate);
+  ChromiumHttpConnection(std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+                             url_loader_factory_info,
+                         Delegate* delegate);
 
   // assistant_client::HttpConnection implementation:
   void SetRequest(const std::string& url, Method method) override;
@@ -49,6 +54,16 @@ class ChromiumHttpConnection
   void Close() override;
   void UploadData(const std::string& data, bool is_last_chunk) override;
 
+  // network::SimpleURLLoaderStreamConsumer implementation:
+  void OnDataReceived(base::StringPiece string_piece,
+                      base::OnceClosure resume) override;
+  void OnComplete(bool success) override;
+  void OnRetry(base::OnceClosure start_retry) override;
+
+  // network::mojom::ChunkedDataPipeGetter implementation:
+  void GetSize(GetSizeCallback get_size_callback) override;
+  void StartReading(mojo::ScopedDataPipeProducerHandle pipe) override;
+
  protected:
   ~ChromiumHttpConnection() override;
 
@@ -62,25 +77,42 @@ class ChromiumHttpConnection
     DESTROYED,
   };
 
-  // HttpConnection methods, re-scheduled on network thread:
-  void SetRequestOnThread(const std::string& url, Method method);
-  void AddHeaderOnThread(const std::string& name, const std::string& value);
-  void SetUploadContentOnThread(const std::string& content,
-                                const std::string& content_type);
-  void SetChunkedUploadContentTypeOnThread(const std::string& content_type);
-  void EnablePartialResultsOnThread();
-  void StartOnThread();
-  void CloseOnThread();
-  void UploadDataOnThread(const std::string& data, bool is_last_chunk);
-
-  // URLFetcherDelegate implementation:
-  void OnURLFetchComplete(const ::net::URLFetcher* source) override;
-
-  scoped_refptr<::net::URLRequestContextGetter> url_request_context_getter_;
-  Delegate* const delegate_;
-  scoped_refptr<base::SingleThreadTaskRunner> network_task_runner_;
+  // HttpConnection methods, re-scheduled on |task_runner|:
+  void SetRequestOnTaskRunner(const std::string& url, Method method);
+  void AddHeaderOnTaskRunner(const std::string& name, const std::string& value);
+  void SetUploadContentOnTaskRunner(const std::string& content,
+                                    const std::string& content_type);
+  void SetChunkedUploadContentTypeOnTaskRunner(const std::string& content_type);
+  void EnablePartialResultsOnTaskRunner();
+  void StartOnTaskRunner();
+  void CloseOnTaskRunner();
+  void UploadDataOnTaskRunner(const std::string& data, bool is_last_chunk);
 
+  // URL loader completion callback.
+  void OnURLLoadComplete(std::unique_ptr<std::string> response_body);
+
+  // Send more chunked upload data.
+  void SendData();
+
+  // |upload_pipe_| can now receive more data.
+  void OnUploadPipeWriteable(MojoResult unused);
+
+  Delegate* const delegate_;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
   State state_ = State::NEW;
+  bool has_last_chunk_ = false;
+  uint64_t upload_body_size_ = 0;
+  std::unique_ptr<network::SharedURLLoaderFactoryInfo> url_loader_factory_info_;
+  std::unique_ptr<network::SimpleURLLoader> url_loader_;
+  // The portion of the body not yet uploaded when doing chunked uploads.
+  std::string upload_body_;
+  // Current pipe being used to send the |upload_body_| to |url_loader_|.
+  mojo::ScopedDataPipeProducerHandle upload_pipe_;
+  // Watches |upload_pipe_| for writeability.
+  std::unique_ptr<mojo::SimpleWatcher> upload_pipe_watcher_;
+  // If non-null, invoked once the size of the upload is known.
+  network::mojom::ChunkedDataPipeGetter::GetSizeCallback get_size_callback_;
+  mojo::BindingSet<network::mojom::ChunkedDataPipeGetter> binding_set_;
 
   // Parameters to be set before Start() call.
   GURL url_;
@@ -91,19 +123,15 @@ class ChromiumHttpConnection
   std::string chunked_upload_content_type_;
   bool handle_partial_response_ = false;
 
-  // |url_fetcher_| has to be accessed by network thread only. Some methods
-  // of URLFetcher (especially GetResponseAsString()) are not safe to access
-  // from other threads even under lock, since chromium accesses it as well.
-  std::unique_ptr<::net::URLFetcher> url_fetcher_;
-
   DISALLOW_COPY_AND_ASSIGN(ChromiumHttpConnection);
 };
 
 class ChromiumHttpConnectionFactory
     : public assistant_client::HttpConnectionFactory {
  public:
-  ChromiumHttpConnectionFactory(
-      scoped_refptr<::net::URLRequestContextGetter> url_request_context_getter);
+  explicit ChromiumHttpConnectionFactory(
+      std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+          url_loader_factory_info);
   ~ChromiumHttpConnectionFactory() override;
 
   // assistant_client::HttpConnectionFactory implementation:
@@ -111,7 +139,7 @@ class ChromiumHttpConnectionFactory
       assistant_client::HttpConnection::Delegate* delegate) override;
 
  private:
-  scoped_refptr<::net::URLRequestContextGetter> url_request_context_getter_;
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromiumHttpConnectionFactory);
 };

chromeos/services/assistant/service.cc

@@ -24,6 +24,7 @@
 #include "google_apis/gaia/google_service_auth_error.h"
 #include "services/identity/public/cpp/scope_set.h"
 #include "services/identity/public/mojom/constants.mojom.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/service_manager/public/cpp/connector.h"
 
 #if BUILDFLAG(ENABLE_CROS_LIBASSISTANT)
@@ -56,7 +57,8 @@ constexpr base::TimeDelta kMaxTokenRefreshDelay =
 
 Service::Service(service_manager::mojom::ServiceRequest request,
                  network::NetworkConnectionTracker* network_connection_tracker,
-                 scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
+                 std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+                     url_loader_factory_info)
     : service_binding_(this, std::move(request)),
       platform_binding_(this),
       session_observer_binding_(this),
@@ -64,7 +66,7 @@ Service::Service(service_manager::mojom::ServiceRequest request,
       main_task_runner_(base::SequencedTaskRunnerHandle::Get()),
       power_manager_observer_(this),
       network_connection_tracker_(network_connection_tracker),
-      io_task_runner_(std::move(io_task_runner)),
+      url_loader_factory_info_(std::move(url_loader_factory_info)),
       weak_ptr_factory_(this) {
   registry_.AddInterface<mojom::AssistantPlatform>(base::BindRepeating(
       &Service::BindAssistantPlatformConnection, base::Unretained(this)));
@@ -296,9 +298,12 @@ void Service::CreateAssistantManagerService() {
   device::mojom::BatteryMonitorPtr battery_monitor;
   service_binding_.GetConnector()->BindInterface(
       device::mojom::kServiceName, mojo::MakeRequest(&battery_monitor));
+
+  // |assistant_manager_service_| is only created once.
+  DCHECK(url_loader_factory_info_);
   assistant_manager_service_ = std::make_unique<AssistantManagerServiceImpl>(
       service_binding_.GetConnector(), std::move(battery_monitor), this,
-      network_connection_tracker_);
+      network_connection_tracker_, std::move(url_loader_factory_info_));
 #else
   assistant_manager_service_ =
       std::make_unique<FakeAssistantManagerServiceImpl>();

chromeos/services/assistant/service.h

@@ -41,6 +41,7 @@ class OneShotTimer;
 
 namespace network {
 class NetworkConnectionTracker;
+class SharedURLLoaderFactoryInfo;
 }  // namespace network
 
 namespace power_manager {
@@ -61,7 +62,8 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) Service
  public:
   Service(service_manager::mojom::ServiceRequest request,
           network::NetworkConnectionTracker* network_connection_tracker,
-          scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
+          std::unique_ptr<network::SharedURLLoaderFactoryInfo>
+              url_loader_factory_info);
   ~Service() override;
 
   mojom::Client* client() { return client_.get(); }
@@ -88,10 +90,6 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) Service
   }
 
   ash::AssistantStateBase* assistant_state() { return &assistant_state_; }
-  // net::URLRequestContextGetter requires a base::SingleThreadTaskRunner.
-  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner() {
-    return io_task_runner_;
-  }
 
   scoped_refptr<base::SequencedTaskRunner> main_task_runner() {
     return main_task_runner_;
@@ -202,7 +200,8 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) Service
   ash::AssistantStateProxy assistant_state_;
 
   network::NetworkConnectionTracker* network_connection_tracker_;
-  scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
+  // non-null until |assistant_manager_service_| is created.
+  std::unique_ptr<network::SharedURLLoaderFactoryInfo> url_loader_factory_info_;
 
   base::WeakPtrFactory<Service> weak_ptr_factory_;
 

chromeos/services/assistant/service_unittest.cc

@@ -5,6 +5,7 @@
 #include "chromeos/services/assistant/service.h"
 
 #include <utility>
+#include <vector>
 
 #include "base/logging.h"
 #include "base/memory/scoped_refptr.h"
@@ -19,6 +20,9 @@
 #include "chromeos/services/assistant/fake_assistant_manager_service_impl.h"
 #include "chromeos/services/assistant/public/mojom/constants.mojom.h"
 #include "services/identity/public/mojom/identity_manager.mojom.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
 #include "services/service_manager/public/cpp/service_binding.h"
 #include "services/service_manager/public/cpp/test/test_connector_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -194,6 +198,9 @@ class AssistantServiceTest : public testing::Test {
   void SetUp() override {
     GetPlatform()->Init(fake_assistant_client_.CreateInterfacePtrAndBind(),
                         fake_device_actions_.CreateInterfacePtrAndBind());
+    shared_url_loader_factory_ =
+        base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+            &url_loader_factory_);
     platform_.FlushForTesting();
     base::RunLoop().RunUntilIdle();
   }
@@ -233,6 +240,9 @@ class AssistantServiceTest : public testing::Test {
   FakeAssistantManagerServiceImpl* fake_assistant_manager_;
   chromeos::FakePowerManagerClient* power_manager_client_;
 
+  network::TestURLLoaderFactory url_loader_factory_;
+  scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_;
+
   scoped_refptr<base::TestMockTimeTaskRunner> mock_task_runner_;
   std::unique_ptr<base::OneShotTimer> mock_timer_;
 

chromeos/services/ime/BUILD.gn

@@ -65,6 +65,7 @@ source_set("services_unittests") {
     "//base/test:test_support",
     "//chromeos/services/ime/public/mojom",
     "//mojo/public/cpp/bindings",
+    "//services/network:test_support",
     "//services/service_manager/public/cpp",
     "//services/service_manager/public/cpp/test:test_support",
     "//testing/gmock",