[CodeHealth] Deprecate SimpleURLLoader unique_ptr string

This CL provides methods that communicate the body response of a request
using optional<string>, while marking as deprecated the existing methods
that rely on unique_ptr<string>.

In general, unique_ptr<string> is less efficient than merely using an
optional<string>, and it is also less ergonomic to use. The occurrence
of unique_ptr<string> in the codebase is usually a historical quirk of
unique_ptr being used as a replacement for optional.

Bug: 1420528
Change-Id: I4ca52f7fa0debc15ac6fe276a4f3deac9e2035ce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5027601
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Claudio DeSouza <cdesouza@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1225550}

ash/ambient/backdrop/ambient_backend_controller_impl.cc

@@ -326,10 +326,11 @@ class BackdropURLLoader {
 
   // Starts downloading the proto. |request_body| is a serialized proto and
   // will be used as the upload body if it is a POST request.
-  void Start(std::unique_ptr<network::ResourceRequest> resource_request,
+  void Start(
+      std::unique_ptr<network::ResourceRequest> resource_request,
       const absl::optional<std::string>& request_body,
       const net::NetworkTrafficAnnotationTag& traffic_annotation,
-             network::SimpleURLLoader::BodyAsStringCallback callback) {
+      network::SimpleURLLoader::BodyAsStringCallbackDeprecated callback) {
     // No ongoing downloading task.
     DCHECK(!simple_loader_);
 
@@ -350,7 +351,8 @@ class BackdropURLLoader {
 
  private:
   // Called when the download completes.
-  void OnUrlDownloaded(network::SimpleURLLoader::BodyAsStringCallback callback,
+  void OnUrlDownloaded(
+      network::SimpleURLLoader::BodyAsStringCallbackDeprecated callback,
       std::unique_ptr<std::string> response_body) {
     loader_factory_.reset();
 

chrome/browser/ash/enhanced_network_tts/enhanced_network_tts_impl.cc

@@ -197,7 +197,8 @@ void EnhancedNetworkTtsImpl::ProcessNextServerRequest() {
   }
 
   const ServerRequestList::iterator first_request_it = server_requests_.begin();
-  network::SimpleURLLoader::BodyAsStringCallback body_as_string_callback =
+  network::SimpleURLLoader::BodyAsStringCallbackDeprecated
+      body_as_string_callback =
           base::BindOnce(&EnhancedNetworkTtsImpl::OnServerResponseReceived,
                          weak_factory_.GetWeakPtr(), first_request_it);
   server_requests_.front().url_loader->DownloadToString(

chrome/browser/bitmap_fetcher/bitmap_fetcher.cc

@@ -50,8 +50,9 @@ void BitmapFetcher::Init(net::ReferrerPolicy referrer_policy,
 }
 
 void BitmapFetcher::Start(network::mojom::URLLoaderFactory* loader_factory) {
-  network::SimpleURLLoader::BodyAsStringCallback callback = base::BindOnce(
+  network::SimpleURLLoader::BodyAsStringCallbackDeprecated callback =
-      &BitmapFetcher::OnSimpleLoaderComplete, weak_factory_.GetWeakPtr());
+      base::BindOnce(&BitmapFetcher::OnSimpleLoaderComplete,
+                     weak_factory_.GetWeakPtr());
 
   // Early exit to handle data URLs.
   if (url_.SchemeIs(url::kDataScheme)) {

components/captive_portal/core/captive_portal_detector.cc

@@ -65,8 +65,9 @@ void CaptivePortalDetector::StartProbe(
   simple_loader_ = network::SimpleURLLoader::Create(std::move(resource_request),
                                                     traffic_annotation);
   simple_loader_->SetAllowHttpErrorResults(true);
-  network::SimpleURLLoader::BodyAsStringCallback callback = base::BindOnce(
+  network::SimpleURLLoader::BodyAsStringCallbackDeprecated callback =
-      &CaptivePortalDetector::OnSimpleLoaderComplete, base::Unretained(this));
+      base::BindOnce(&CaptivePortalDetector::OnSimpleLoaderComplete,
+                     base::Unretained(this));
   state_ = State::kProbe;
   simple_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
       loader_factory_, std::move(callback));

components/endpoint_fetcher/endpoint_fetcher.cc

@@ -251,9 +251,9 @@ void EndpointFetcher::PerformRequest(
                                       network::SimpleURLLoader::RETRY_ON_5XX);
   simple_url_loader_->SetTimeoutDuration(base::Milliseconds(timeout_ms_));
   simple_url_loader_->SetAllowHttpErrorResults(true);
-  network::SimpleURLLoader::BodyAsStringCallback body_as_string_callback =
+  network::SimpleURLLoader::BodyAsStringCallbackDeprecated
-      base::BindOnce(&EndpointFetcher::OnResponseFetched,
+      body_as_string_callback = base::BindOnce(
-                     weak_ptr_factory_.GetWeakPtr(),
+          &EndpointFetcher::OnResponseFetched, weak_ptr_factory_.GetWeakPtr(),
           std::move(endpoint_fetcher_callback));
   simple_url_loader_->DownloadToString(
       url_loader_factory_.get(), std::move(body_as_string_callback),

content/public/test/simple_url_loader_test_helper.cc

@@ -14,7 +14,7 @@ SimpleURLLoaderTestHelper::SimpleURLLoaderTestHelper() {}
 
 SimpleURLLoaderTestHelper::~SimpleURLLoaderTestHelper() {}
 
-network::SimpleURLLoader::BodyAsStringCallback
+network::SimpleURLLoader::BodyAsStringCallbackDeprecated
 SimpleURLLoaderTestHelper::GetCallback() {
   DCHECK(!callback_created_);
   callback_created_ = true;

content/public/test/simple_url_loader_test_helper.h

@@ -27,9 +27,9 @@ class SimpleURLLoaderTestHelper {
 
   ~SimpleURLLoaderTestHelper();
 
-  // Returns a BodyAsStringCallback for use with a SimpleURLLoader. May be
+  // Returns a BodyAsStringCallbackDeprecated for use with a SimpleURLLoader.
-  // called only once.
+  // May be called only once.
-  network::SimpleURLLoader::BodyAsStringCallback GetCallback();
+  network::SimpleURLLoader::BodyAsStringCallbackDeprecated GetCallback();
 
   // Waits until the callback returned by GetCallback() is invoked.
   void WaitForCallback();

content/test/io_thread_shared_url_loader_factory_owner.cc

@@ -40,10 +40,10 @@ void InitializeSharedFactoryOnIOThread(
   run_loop.Run();
 }
 
-network::SimpleURLLoader::BodyAsStringCallback RunOnUIThread(
+network::SimpleURLLoader::BodyAsStringCallbackDeprecated RunOnUIThread(
-    network::SimpleURLLoader::BodyAsStringCallback ui_callback) {
+    network::SimpleURLLoader::BodyAsStringCallbackDeprecated ui_callback) {
   return base::BindOnce(
-      [](network::SimpleURLLoader::BodyAsStringCallback callback,
+      [](network::SimpleURLLoader::BodyAsStringCallbackDeprecated callback,
          std::unique_ptr<std::string> response_body) {
         DCHECK_CURRENTLY_ON(BrowserThread::IO);
         GetUIThreadTaskRunner({})->PostTask(
@@ -110,7 +110,7 @@ int IOThreadSharedURLLoaderFactoryOwner::LoadBasicRequestOnIOThread(
       FROM_HERE, base::BindOnce(
                      [](network::SimpleURLLoader* loader,
                         network::mojom::URLLoaderFactory* factory,
-                        network::SimpleURLLoader::BodyAsStringCallback
+                        network::SimpleURLLoader::BodyAsStringCallbackDeprecated
                             body_as_string_callback) {
                        loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
                            factory, std::move(body_as_string_callback));

services/network/public/cpp/simple_url_loader.cc

@@ -58,6 +58,18 @@ namespace {
 // Used by tests to override the tick clock for the timeout timer.
 const base::TickClock* timeout_tick_clock_ = nullptr;
 
+// A temporary util adapter to wrap the download callback with the response
+// body, and to hop the string content from a unique_ptr<string> into a
+// optional<string>.
+void GetFromUniquePtrToOptional(
+    SimpleURLLoader::BodyAsStringCallback body_as_string_callback,
+    std::unique_ptr<std::string> response_body) {
+  std::move(body_as_string_callback)
+      .Run(response_body
+               ? std::make_optional<std::string>(std::move(*response_body))
+               : std::nullopt);
+}
+
 // This file contains SimpleURLLoaderImpl, several BodyHandler implementations,
 // BodyReader, and StringUploadDataPipeGetter.
 //
@@ -210,9 +222,15 @@ class SimpleURLLoaderImpl : public SimpleURLLoader,
   ~SimpleURLLoaderImpl() override;
 
   // SimpleURLLoader implementation.
+  void DownloadToString(mojom::URLLoaderFactory* url_loader_factory,
+                        BodyAsStringCallbackDeprecated body_as_string_callback,
+                        size_t max_body_size) override;
   void DownloadToString(mojom::URLLoaderFactory* url_loader_factory,
                         BodyAsStringCallback body_as_string_callback,
                         size_t max_body_size) override;
+  void DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      mojom::URLLoaderFactory* url_loader_factory,
+      BodyAsStringCallbackDeprecated body_as_string_callback) override;
   void DownloadToStringOfUnboundedSizeUntilCrashAndDie(
       mojom::URLLoaderFactory* url_loader_factory,
       BodyAsStringCallback body_as_string_callback) override;
@@ -681,7 +699,7 @@ class SaveToStringBodyHandler : public BodyHandler,
   SaveToStringBodyHandler(
       SimpleURLLoaderImpl* simple_url_loader,
       bool want_download_progress,
-      SimpleURLLoader::BodyAsStringCallback body_as_string_callback,
+      SimpleURLLoader::BodyAsStringCallbackDeprecated body_as_string_callback,
       int64_t max_body_size)
       : BodyHandler(simple_url_loader, want_download_progress),
         max_body_size_(max_body_size),
@@ -742,7 +760,7 @@ class SaveToStringBodyHandler : public BodyHandler,
   const int64_t max_body_size_;
 
   std::unique_ptr<std::string> body_;
-  SimpleURLLoader::BodyAsStringCallback body_as_string_callback_;
+  SimpleURLLoader::BodyAsStringCallbackDeprecated body_as_string_callback_;
 
   const base::Location url_loader_created_from_;
 
@@ -1260,7 +1278,7 @@ SimpleURLLoaderImpl::~SimpleURLLoaderImpl() {}
 
 void SimpleURLLoaderImpl::DownloadToString(
     mojom::URLLoaderFactory* url_loader_factory,
-    BodyAsStringCallback body_as_string_callback,
+    BodyAsStringCallbackDeprecated body_as_string_callback,
     size_t max_body_size) {
   DCHECK_LE(max_body_size, kMaxBoundedStringDownloadSize);
   body_handler_ = std::make_unique<SaveToStringBodyHandler>(
@@ -1269,9 +1287,19 @@ void SimpleURLLoaderImpl::DownloadToString(
   Start(url_loader_factory);
 }
 
+void SimpleURLLoaderImpl::DownloadToString(
+    mojom::URLLoaderFactory* url_loader_factory,
+    BodyAsStringCallback body_as_string_callback,
+    size_t max_body_size) {
+  DownloadToString(url_loader_factory,
+                   base::BindOnce(GetFromUniquePtrToOptional,
+                                  std::move(body_as_string_callback)),
+                   max_body_size);
+}
+
 void SimpleURLLoaderImpl::DownloadToStringOfUnboundedSizeUntilCrashAndDie(
     mojom::URLLoaderFactory* url_loader_factory,
-    BodyAsStringCallback body_as_string_callback) {
+    BodyAsStringCallbackDeprecated body_as_string_callback) {
   body_handler_ = std::make_unique<SaveToStringBodyHandler>(
       this, !on_download_progress_callback_.is_null(),
       std::move(body_as_string_callback),
@@ -1281,6 +1309,14 @@ void SimpleURLLoaderImpl::DownloadToStringOfUnboundedSizeUntilCrashAndDie(
   Start(url_loader_factory);
 }
 
+void SimpleURLLoaderImpl::DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+    mojom::URLLoaderFactory* url_loader_factory,
+    BodyAsStringCallback body_as_string_callback) {
+  DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      url_loader_factory, base::BindOnce(GetFromUniquePtrToOptional,
+                                         std::move(body_as_string_callback)));
+}
+
 void SimpleURLLoaderImpl::DownloadHeadersOnly(
     mojom::URLLoaderFactory* url_loader_factory,
     HeadersOnlyCallback headers_only_callback) {

services/network/public/cpp/simple_url_loader.h

@@ -97,8 +97,10 @@ class COMPONENT_EXPORT(NETWORK_CPP) SimpleURLLoader {
   // like HTTP_OK, which could happen if there's an interruption before the
   // full response body is received. It is safe to delete the SimpleURLLoader
   // during the callback.
-  using BodyAsStringCallback =
+  using BodyAsStringCallbackDeprecated =
       base::OnceCallback<void(std::unique_ptr<std::string> response_body)>;
+  using BodyAsStringCallback =
+      base::OnceCallback<void(std::optional<std::string> response_body)>;
 
   // Callback used when ignoring the response body. |headers| are the received
   // HTTP headers, or nullptr if none were received. It is safe to delete the
@@ -172,6 +174,10 @@ class COMPONENT_EXPORT(NETWORK_CPP) SimpleURLLoader {
   // invoked on completion. Deleting the SimpleURLLoader before the callback is
   // invoked will result in cancelling the request, and the callback will not be
   // called.
+  virtual void DownloadToString(
+      mojom::URLLoaderFactory* url_loader_factory,
+      BodyAsStringCallbackDeprecated body_as_string_callback,
+      size_t max_body_size) = 0;
   virtual void DownloadToString(mojom::URLLoaderFactory* url_loader_factory,
                                 BodyAsStringCallback body_as_string_callback,
                                 size_t max_body_size) = 0;
@@ -181,6 +187,9 @@ class COMPONENT_EXPORT(NETWORK_CPP) SimpleURLLoader {
   // exceeded. It's recommended consumers use one of the other download methods
   // instead (DownloadToString if the body is expected to be of reasonable
   // length, or DownloadToFile otherwise).
+  virtual void DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      mojom::URLLoaderFactory* url_loader_factory,
+      BodyAsStringCallbackDeprecated body_as_string_callback) = 0;
   virtual void DownloadToStringOfUnboundedSizeUntilCrashAndDie(
       mojom::URLLoaderFactory* url_loader_factory,
       BodyAsStringCallback body_as_string_callback) = 0;