[CodeHealth] Passing response value by value

For historical reasons, ResponseValueObject was an abstract class, and
providing a response therefore would always require an allocation. This
CL simplifies ResponsoValueObject into a single type with no
inheritance, that can be passed around by value.

With this change, it has also been revealed places where ResponseValue
is supposed to be an optional. This has provided opportunity for
removing unnecessarily if a certain value is engaged or not when being
passed around, as a unique_ptr can always be nullptr.

Work in a subsequent CL is going to handle a similar solution for
ResponseActionObject.

Bug: 1417490
Change-Id: Ied1709e693ce2305a6f03a02b9ad9e4cc2d9a8fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4270110
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Claudio DeSouza <cdesouza@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1108724}

chrome/browser/ash/extensions/autotest_private/autotest_private_api.cc

@@ -4395,7 +4395,7 @@ AutotestPrivateWaitForDisplayRotationFunction::Run() {
 
   auto result = CheckScreenRotationAnimation();
   if (result)
-    return RespondNow(std::move(result));
+    return RespondNow(std::move(*result));
   return RespondLater();
 }
 
@@ -4427,10 +4427,10 @@ void AutotestPrivateWaitForDisplayRotationFunction::
   auto result = CheckScreenRotationAnimation();
   // Wait for the rotation if unlocking causes rotation.
   if (result)
-    Respond(std::move(result));
+    Respond(std::move(*result));
 }
 
-ExtensionFunction::ResponseValue
+absl::optional<ExtensionFunction::ResponseValue>
 AutotestPrivateWaitForDisplayRotationFunction::CheckScreenRotationAnimation() {
   auto* root_window = ash::Shell::GetRootWindowForDisplayId(display_id_);
   if (!root_window) {
@@ -4451,7 +4451,7 @@ AutotestPrivateWaitForDisplayRotationFunction::CheckScreenRotationAnimation() {
   self_ = this;
 
   animator->AddObserver(this);
-  return nullptr;
+  return absl::nullopt;
 }
 
 ///////////////////////////////////////////////////////////////////////////////

chrome/browser/ash/extensions/autotest_private/autotest_private_api.h

@@ -1173,7 +1173,7 @@ class AutotestPrivateWaitForDisplayRotationFunction
   ~AutotestPrivateWaitForDisplayRotationFunction() override;
   ResponseAction Run() override;
 
-  ResponseValue CheckScreenRotationAnimation();
+  absl::optional<ResponseValue> CheckScreenRotationAnimation();
 
   int64_t display_id_ = display::kInvalidDisplayId;
   absl::optional<display::Display::Rotation> target_rotation_;

chrome/browser/extensions/api/bookmarks/bookmarks_api.cc

@@ -67,7 +67,7 @@ ExtensionFunction::ResponseAction BookmarksFunction::Run() {
   }
 
   ResponseValue response = RunOnReady();
-  return response ? RespondNow(std::move(response)) : RespondLater();
+  return RespondNow(std::move(response));
 }
 
 BookmarkModel* BookmarksFunction::GetBookmarkModel() {
@@ -194,9 +194,7 @@ void BookmarksFunction::BookmarkModelLoaded(BookmarkModel* model,
   model->RemoveObserver(this);
 
   ResponseValue response = RunOnReady();
-  if (response)
-    Respond(std::move(response));
-  // else, the function will Respond() on its own later.
+  Respond(std::move(response));
 
   Release();  // Balanced in Run().
 }

chrome/browser/extensions/api/browsing_data/browsing_data_api.cc

@@ -307,15 +307,21 @@ ExtensionFunction::ResponseAction BrowsingDataRemoverFunction::Run() {
   }
 
   if (origins) {
-    ResponseValue error_response;
-    if (!ParseOrigins(*origins, &origins_, &error_response))
-      return RespondNow(std::move(error_response));
+    OriginParsingResult result = ParseOrigins(*origins);
+    if (result.has_value()) {
+      origins_ = std::move(*result);
+    } else {
+      return RespondNow(std::move(result.error()));
+    }
     mode_ = content::BrowsingDataFilterBuilder::Mode::kDelete;
   } else {
     if (exclude_origins) {
-      ResponseValue error_response;
-      if (!ParseOrigins(*exclude_origins, &origins_, &error_response))
-        return RespondNow(std::move(error_response));
+      OriginParsingResult result = ParseOrigins(*exclude_origins);
+      if (result.has_value()) {
+        origins_ = std::move(*result);
+      } else {
+        return RespondNow(std::move(result.error()));
+      }
     }
     mode_ = content::BrowsingDataFilterBuilder::Mode::kPreserve;
   }
@@ -459,26 +465,23 @@ bool BrowsingDataRemoverFunction::ParseOriginTypeMask(
   return true;
 }
 
-bool BrowsingDataRemoverFunction::ParseOrigins(
-    const base::Value::List& list_value,
-    std::vector<url::Origin>* result,
-    ResponseValue* error_response) {
-  result->reserve(list_value.size());
+BrowsingDataRemoverFunction::OriginParsingResult
+BrowsingDataRemoverFunction::ParseOrigins(const base::Value::List& list_value) {
+  std::vector<url::Origin> result;
+  result.reserve(list_value.size());
   for (const auto& value : list_value) {
     if (!value.is_string()) {
-      *error_response = BadMessage();
-      return false;
+      return base::unexpected(BadMessage());
     }
     url::Origin origin = url::Origin::Create(GURL(value.GetString()));
     if (origin.opaque()) {
-      *error_response = Error(base::StringPrintf(
+      return base::unexpected(Error(base::StringPrintf(
           extension_browsing_data_api_constants::kInvalidOriginError,
-          value.GetString().c_str()));
-      return false;
+          value.GetString().c_str())));
     }
-    result->push_back(std::move(origin));
+    result.push_back(std::move(origin));
   }
-  return true;
+  return result;
 }
 
 // Parses the |dataToRemove| argument to generate the removal mask.

chrome/browser/extensions/api/browsing_data/browsing_data_api.h

@@ -15,6 +15,7 @@
 #include "base/memory/raw_ptr.h"
 #include "base/scoped_observation.h"
 #include "base/time/time.h"
+#include "base/types/expected.h"
 #include "components/browsing_data/core/browsing_data_utils.h"
 #include "components/signin/core/browser/account_reconcilor.h"
 #include "content/public/browser/browsing_data_filter_builder.h"
@@ -133,9 +134,9 @@ class BrowsingDataRemoverFunction
   // Parses the developer-provided list of origins into |result|.
   // Returns whether or not parsing was successful. In case of parse failure,
   // |error_response| will contain the error response.
-  bool ParseOrigins(const base::Value::List& list_value,
-                    std::vector<url::Origin>* result,
-                    ResponseValue* error_response);
+  using OriginParsingResult =
+      base::expected<std::vector<url::Origin>, ResponseValue>;
+  OriginParsingResult ParseOrigins(const base::Value::List& list_value);
 
   // Called when we're ready to start removing data.
   void StartRemoving();

chrome/browser/extensions/api/cookies/cookies_api.cc

@@ -348,38 +348,32 @@ ExtensionFunction::ResponseAction CookiesGetAllFunction::Run() {
 void CookiesGetAllFunction::GetAllCookiesCallback(
     const net::CookieList& cookie_list) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  ResponseValue response;
   if (extension()) {
     std::vector<api::cookies::Cookie> match_vector;
     cookies_helpers::AppendMatchingCookiesFromCookieListToVector(
         cookie_list, &parsed_args_->details, extension(), &match_vector);
 
-    response =
-        ArgumentList(api::cookies::GetAll::Results::Create(match_vector));
+    Respond(ArgumentList(api::cookies::GetAll::Results::Create(match_vector)));
   } else {
     // TODO(devlin): When can |extension()| be null for this function?
-    response = WithArguments();
+    Respond(NoArguments());
   }
-  Respond(std::move(response));
 }
 
 void CookiesGetAllFunction::GetCookieListCallback(
     const net::CookieAccessResultList& cookie_list,
     const net::CookieAccessResultList& excluded_cookies) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  ResponseValue response;
   if (extension()) {
     std::vector<api::cookies::Cookie> match_vector;
     cookies_helpers::AppendMatchingCookiesFromCookieAccessResultListToVector(
         cookie_list, &parsed_args_->details, extension(), &match_vector);
 
-    response =
-        ArgumentList(api::cookies::GetAll::Results::Create(match_vector));
+    Respond(ArgumentList(api::cookies::GetAll::Results::Create(match_vector)));
   } else {
     // TODO(devlin): When can |extension()| be null for this function?
-    response = WithArguments();
+    Respond(NoArguments());
   }
-  Respond(std::move(response));
 }
 
 void CookiesGetAllFunction::NotifyExtensionTelemetry() {
@@ -526,7 +520,7 @@ void CookiesSetFunction::GetCookieListCallback(
     return;
   }
 
-  ResponseValue value;
+  absl::optional<ResponseValue> value;
   for (const net::CookieWithAccessResult& cookie_with_access_result :
        cookie_list) {
     // Return the first matching cookie. Relies on the fact that the
@@ -536,12 +530,13 @@ void CookiesSetFunction::GetCookieListCallback(
     if (cookie_with_access_result.cookie.Name() == name) {
       api::cookies::Cookie api_cookie = cookies_helpers::CreateCookie(
           cookie_with_access_result.cookie, *parsed_args_->details.store_id);
-      value = ArgumentList(api::cookies::Set::Results::Create(api_cookie));
+      value.emplace(
+          ArgumentList(api::cookies::Set::Results::Create(api_cookie)));
       break;
     }
   }
 
-  Respond(value ? std::move(value) : WithArguments());
+  Respond(value ? std::move(*value) : WithArguments());
 }
 
 CookiesRemoveFunction::CookiesRemoveFunction() {

chrome/browser/extensions/api/developer_private/developer_private_api.cc

@@ -1778,11 +1778,8 @@ void DeveloperPrivateLoadDirectoryFunction::ReadDirectoryByFileSystemAPICb(
     pending_copy_operations_count_--;
 
     if (!pending_copy_operations_count_) {
-      ExtensionFunction::ResponseValue response;
-      if (success_)
-        response = NoArguments();
-      else
-        response = Error(error_);
+      ExtensionFunction::ResponseValue response =
+          success_ ? NoArguments() : Error(error_);
       content::GetUIThreadTaskRunner({})->PostTask(
           FROM_HERE,
           base::BindOnce(&DeveloperPrivateLoadDirectoryFunction::Respond, this,

chrome/browser/extensions/api/extension_action/extension_action_api.cc

@@ -696,22 +696,19 @@ ExtensionFunction::ResponseAction ActionOpenPopupFunction::Run() {
 void ActionOpenPopupFunction::OnShowPopupComplete(ExtensionHost* popup_host) {
   DCHECK(!did_respond());
 
-  ResponseValue response_value;
   if (popup_host) {
     // TODO(https://crbug.com/1245093): Return the tab for which the extension
     // popup was shown?
     DCHECK(popup_host->document_element_available());
-    response_value = NoArguments();
+    Respond(NoArguments());
   } else {
     // NOTE(devlin): We could have the callback pass more information here about
     // why the popup didn't open (e.g., another active popup vs popup closing
     // before display, as may happen if the window closes), but it's not clear
     // whether that would be significantly helpful to developers and it may
     // leak other information about the user's browser.
-    response_value = Error(kFailedToOpenPopupGenericError);
+    Respond(Error(kFailedToOpenPopupGenericError));
   }
-
-  Respond(std::move(response_value));
 }
 
 BrowserActionOpenPopupFunction::BrowserActionOpenPopupFunction() = default;

extensions/browser/api/async_api_function.cc

@@ -94,20 +94,19 @@ void AsyncApiFunction::RespondOnUIThread() {
 }
 
 void AsyncApiFunction::SendResponse(bool success) {
-  ResponseValue response;
   base::Value::List arguments;
   if (results_) {
     arguments = std::move(*results_);
     results_.reset();
   }
   if (success) {
-    response = ArgumentList(std::move(arguments));
+    ExtensionFunction::Respond(ArgumentList(std::move(arguments)));
   } else if (results_) {
-    response = ErrorWithArguments(std::move(arguments), error_);
+    ExtensionFunction::Respond(
+        ErrorWithArguments(std::move(arguments), error_));
   } else {
-    response = Error(error_);
+    ExtensionFunction::Respond(Error(error_));
   }
-  ExtensionFunction::Respond(std::move(response));
 }
 
 }  // namespace extensions

extensions/browser/api/guest_view/app_view/app_view_guest_internal_api.cc

@@ -25,15 +25,13 @@ AppViewGuestInternalAttachFrameFunction::Run() {
   GURL url = extension()->GetResourceURL(params->url);
   EXTENSION_FUNCTION_VALIDATE(url.is_valid());
 
-  ResponseValue response;
   if (AppViewGuest::CompletePendingRequest(
           browser_context(), url, params->guest_instance_id, extension_id(),
           render_frame_host()->GetProcess())) {
-    response = NoArguments();
+    return RespondNow(NoArguments());
   } else {
-    response = Error("could not complete");
+    return RespondNow(Error("could not complete"));
   }
-  return RespondNow(std::move(response));
 }
 
 AppViewGuestInternalDenyRequestFunction::
@@ -48,15 +46,13 @@ AppViewGuestInternalDenyRequestFunction::Run() {
 
   // Since the URL passed into AppViewGuest:::CompletePendingRequest is invalid,
   // a new <appview> WebContents will not be created.
-  ResponseValue response;
   if (AppViewGuest::CompletePendingRequest(
           browser_context(), GURL(), params->guest_instance_id, extension_id(),
           render_frame_host()->GetProcess())) {
-    response = NoArguments();
+    return RespondNow(NoArguments());
   } else {
-    response = Error("could not complete");
+    return RespondNow(Error("could not complete"));
   }
-  return RespondNow(std::move(response));
 }
 
 }  // namespace extensions

extensions/browser/api/management/management_api.cc

@@ -843,16 +843,13 @@ ManagementGenerateAppForLinkFunction::~ManagementGenerateAppForLinkFunction() =
 void ManagementGenerateAppForLinkFunction::FinishCreateWebApp(
     const std::string& web_app_id,
     bool install_success) {
-  ResponseValue response;
   if (install_success) {
-    response = ArgumentList(management::GenerateAppForLink::Results::Create(
+    Respond(ArgumentList(management::GenerateAppForLink::Results::Create(
         app_for_link_delegate_->CreateExtensionInfoFromWebApp(
-            web_app_id, browser_context())));
+            web_app_id, browser_context()))));
   } else {
-    response = Error(keys::kGenerateAppForLinkInstallError);
+    Respond(Error(keys::kGenerateAppForLinkInstallError));
   }
-
-  Respond(std::move(response));
   Release();  // Balanced in Run().
 }
 
@@ -1038,18 +1035,17 @@ ManagementInstallReplacementWebAppFunction::Run() {
 
 void ManagementInstallReplacementWebAppFunction::FinishResponse(
     ManagementAPIDelegate::InstallOrLaunchWebAppResult result) {
-  ResponseValue response;
   switch (result) {
     case ManagementAPIDelegate::InstallOrLaunchWebAppResult::kSuccess:
-      response = NoArguments();
+      Respond(NoArguments());
       break;
     case ManagementAPIDelegate::InstallOrLaunchWebAppResult::kInvalidWebApp:
-      response = Error(keys::kInstallReplacementWebAppInvalidWebAppError);
+      Respond(Error(keys::kInstallReplacementWebAppInvalidWebAppError));
       break;
     case ManagementAPIDelegate::InstallOrLaunchWebAppResult::kUnknownError:
-      response = Error(keys::kGenerateAppForLinkInstallError);
+      Respond(Error(keys::kGenerateAppForLinkInstallError));
+      break;
   }
-  Respond(std::move(response));
 }
 
 ManagementEventRouter::ManagementEventRouter(content::BrowserContext* context)

extensions/browser/api/test/test_api.cc

@@ -85,11 +85,11 @@ ExtensionFunction::ResponseAction TestSendMessageFunction::Run() {
   // If none of the listeners intend to respond, or one has already responded,
   // finish the function. We always reply to the message, even if it's just an
   // empty string.
-  if (!listener_will_respond || response_.get()) {
+  if (!listener_will_respond || response_) {
     if (!response_) {
-      response_ = OneArgument(base::Value(std::string()));
+      response_.emplace(WithArguments(std::string()));
     }
-    return RespondNow(std::move(response_));
+    return RespondNow(std::move(*response_));
   }
   // Otherwise, wait for a reply.
   waiting_ = true;
@@ -100,16 +100,16 @@ TestSendMessageFunction::~TestSendMessageFunction() = default;
 
 void TestSendMessageFunction::Reply(const std::string& message) {
   DCHECK(!response_);
-  response_ = OneArgument(base::Value(message));
+  response_.emplace(WithArguments(message));
   if (waiting_)
-    Respond(std::move(response_));
+    Respond(std::move(*response_));
 }
 
 void TestSendMessageFunction::ReplyWithError(const std::string& error) {
   DCHECK(!response_);
-  response_ = Error(error);
+  response_.emplace(Error(error));
   if (waiting_)
-    Respond(std::move(response_));
+    Respond(std::move(*response_));
 }
 
 TestSendScriptResultFunction::TestSendScriptResultFunction() = default;