[CodeHealth] APIRequestResult::TakeBody to return an rvalue

This improvement to this function allows us to make the moving of the value explicit. It also allows us to remove the flag indicating the value has been moved, and actually move this validation to profiling tools, also known as poison-after-use. This change also removes the unnecessary uses of `TakeBody` for cases where a const reference were enough.
brave · Feb 27, 2025 · 37715ff · 37715ff
1 parent 62c5910
commit 37715ff
Show file tree

Hide file tree

Showing 11 changed files with 37 additions and 30 deletions.
diff --git a/components/api_request_helper/api_request_helper.cc b/components/api_request_helper/api_request_helper.cc
@@ -147,9 +147,7 @@ bool APIRequestResult::IsResponseCodeValid() const {
   return response_code_ >= 100 && response_code_ <= 599;
 }
 
-base::Value APIRequestResult::TakeBody() {
-  CHECK(!body_consumed_);
-  body_consumed_ = true;
+base::Value APIRequestResult::TakeBody() && {
   return std::move(value_body_);
 }
 

diff --git a/components/api_request_helper/api_request_helper.h b/components/api_request_helper/api_request_helper.h
@@ -59,7 +59,7 @@ class APIRequestResult {
   int response_code() const { return response_code_; }
 
   // Extract the sanitized response as base::Value.
-  base::Value TakeBody();
+  base::Value TakeBody() &&;
 
   // Returns the sanitized response as base::Value.
   // Note: don't clone large responses, use TakeBody() instead.
@@ -87,7 +87,6 @@ class APIRequestResult {
   base::flat_map<std::string, std::string> headers_;
   int error_code_ = -1;
   GURL final_url_;
-  bool body_consumed_ = false;
 };
 
 struct APIRequestOptions {

diff --git a/components/brave_news/browser/feed_fetcher.cc b/components/brave_news/browser/feed_fetcher.cc
@@ -204,7 +204,7 @@ void FeedFetcher::OnFetchFeedFetchedFeed(
   }
 
   base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, base::BindOnce(&ParseFeedItems, result.TakeBody()),
+      FROM_HERE, base::BindOnce(&ParseFeedItems, std::move(result).TakeBody()),
       base::BindOnce(
           [](base::WeakPtr<FeedFetcher> fetcher, std::string locale,
              std::string etag, FetchFeedSourceCallback callback,

diff --git a/components/brave_news/browser/publishers_controller.cc b/components/brave_news/browser/publishers_controller.cc
@@ -265,7 +265,7 @@ void PublishersController::EnsurePublishersIsUpdating(
                 << ", final_url: " << api_request_result.final_url();
         // TODO(petemill): handle bad status or response
         std::optional<Publishers> publisher_list =
-            ParseCombinedPublisherList(api_request_result.TakeBody());
+            ParseCombinedPublisherList(api_request_result.value_body());
 
         // Update failed, we'll just reuse whatever publishers we had before.
         if (!publisher_list) {

diff --git a/components/brave_news/browser/suggestions_controller.cc b/components/brave_news/browser/suggestions_controller.cc
@@ -97,7 +97,7 @@ double GetVisitWeighting(
 }
 
 SuggestionsController::PublisherSimilarities ParseSimilarityResponse(
-    base::Value records_v,
+    const base::Value& records_v,
     const std::string& locale) {
   if (!records_v.is_dict()) {
     return {};
@@ -315,7 +315,8 @@ void SuggestionsController::EnsureSimilarityMatrixIsUpdating(
                                 controller->locale_ = locale;
                                 controller->similarities_ =
                                     ParseSimilarityResponse(
-                                        api_request_result.TakeBody(), locale);
+                                        api_request_result.value_body(),
+                                        locale);
                                 controller->on_current_update_complete_
                                     ->Signal();
                                 controller->is_update_in_progress_ = false;

diff --git a/components/brave_news/browser/topics_fetcher.cc b/components/brave_news/browser/topics_fetcher.cc
@@ -145,8 +145,8 @@ void TopicsFetcher::OnFetchedTopicArticles(
 
   state.topic_articles_result = std::move(result);
 
-  auto topics = ParseTopics(state.topics_result.TakeBody(),
-                            state.topic_articles_result.TakeBody());
+  auto topics = ParseTopics(state.topics_result.value_body(),
+                            state.topic_articles_result.value_body());
   std::move(state.callback).Run(std::move(topics));
 }
 

diff --git a/components/brave_vpn/browser/api/brave_vpn_api_request.cc b/components/brave_vpn/browser/api/brave_vpn_api_request.cc
@@ -269,8 +269,8 @@ void BraveVpnAPIRequest::OnGetSubscriberCredential(
     APIRequestResult api_request_result) {
   bool success = api_request_result.response_code() == 200;
   std::string error;
-  std::string subscriber_credential =
-      ParseSubscriberCredentialFromJson(api_request_result.TakeBody(), &error);
+  std::string subscriber_credential = ParseSubscriberCredentialFromJson(
+      api_request_result.value_body(), &error);
   if (!success) {
     subscriber_credential = error;
     VLOG(1) << __func__ << " Response from API was not HTTP 200 (Received "

diff --git a/components/brave_vpn/browser/api/vpn_response_parser.cc b/components/brave_vpn/browser/api/vpn_response_parser.cc
@@ -12,7 +12,7 @@
 
 namespace brave_vpn {
 
-std::string ParseSubscriberCredentialFromJson(base::Value records_v,
+std::string ParseSubscriberCredentialFromJson(const base::Value& records_v,
                                               std::string* error) {
   if (!records_v.is_dict()) {
     VLOG(1) << __func__ << "Invalid response, could not parse JSON.";

diff --git a/components/brave_vpn/browser/api/vpn_response_parser.h b/components/brave_vpn/browser/api/vpn_response_parser.h
@@ -12,7 +12,7 @@
 
 namespace brave_vpn {
 
-std::string ParseSubscriberCredentialFromJson(base::Value records_v,
+std::string ParseSubscriberCredentialFromJson(const base::Value& records_v,
                                               std::string* error);
 
 }  // namespace brave_vpn

diff --git a/components/brave_wallet/browser/meld_integration_service.cc b/components/brave_wallet/browser/meld_integration_service.cc
@@ -382,7 +382,8 @@ void MeldIntegrationService::OnGetServiceProviders(
 
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE,
-      base::BindOnce(&ParseServiceProviders, api_request_result.TakeBody()),
+      base::BindOnce(&ParseServiceProviders,
+                     std::move(api_request_result).TakeBody()),
       base::BindOnce(&MeldIntegrationService::OnParseServiceProviders,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
@@ -454,7 +455,8 @@ void MeldIntegrationService::OnGetCryptoQuotes(
 
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE,
-      base::BindOnce(&ParseCryptoQuotes, api_request_result.TakeBody()),
+      base::BindOnce(&ParseCryptoQuotes,
+                     std::move(api_request_result).TakeBody()),
       base::BindOnce(&MeldIntegrationService::OnParseCryptoQuotes,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
@@ -524,7 +526,8 @@ void MeldIntegrationService::OnGetPaymentMethods(
 
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE,
-      base::BindOnce(&ParsePaymentMethods, api_request_result.TakeBody()),
+      base::BindOnce(&ParsePaymentMethods,
+                     std::move(api_request_result).TakeBody()),
       base::BindOnce(&MeldIntegrationService::OnParsePaymentMethods,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
@@ -587,7 +590,8 @@ void MeldIntegrationService::OnGetFiatCurrencies(
 
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE,
-      base::BindOnce(&ParseFiatCurrencies, api_request_result.TakeBody()),
+      base::BindOnce(&ParseFiatCurrencies,
+                     std::move(api_request_result).TakeBody()),
       base::BindOnce(&MeldIntegrationService::OnParseFiatCurrencies,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
@@ -650,7 +654,8 @@ void MeldIntegrationService::OnGetCryptoCurrencies(
 
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE,
-      base::BindOnce(&ParseCryptoCurrencies, api_request_result.TakeBody()),
+      base::BindOnce(&ParseCryptoCurrencies,
+                     std::move(api_request_result).TakeBody()),
       base::BindOnce(&MeldIntegrationService::OnParseCryptoCurrencies,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
@@ -710,7 +715,8 @@ void MeldIntegrationService::OnGetCountries(
   }
 
   base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE, base::BindOnce(&ParseCountries, api_request_result.TakeBody()),
+      FROM_HERE,
+      base::BindOnce(&ParseCountries, std::move(api_request_result).TakeBody()),
       base::BindOnce(&MeldIntegrationService::OnParseCountries,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
@@ -784,7 +790,8 @@ void MeldIntegrationService::OnCryptoBuyWidgetCreate(
 
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE,
-      base::BindOnce(&ParseCryptoWidgetCreate, api_request_result.TakeBody()),
+      base::BindOnce(&ParseCryptoWidgetCreate,
+                     std::move(api_request_result).TakeBody()),
       base::BindOnce(&MeldIntegrationService::OnParseCryptoBuyWidgetCreate,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
@@ -846,7 +853,8 @@ void MeldIntegrationService::OnCryptoSellWidgetCreate(
 
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE,
-      base::BindOnce(&ParseCryptoWidgetCreate, api_request_result.TakeBody()),
+      base::BindOnce(&ParseCryptoWidgetCreate,
+                     std::move(api_request_result).TakeBody()),
       base::BindOnce(&MeldIntegrationService::OnParseCryptoSellWidgetCreate,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
@@ -908,7 +916,8 @@ void MeldIntegrationService::OnCryptoTransferWidgetCreate(
 
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE,
-      base::BindOnce(&ParseCryptoWidgetCreate, api_request_result.TakeBody()),
+      base::BindOnce(&ParseCryptoWidgetCreate,
+                     std::move(api_request_result).TakeBody()),
       base::BindOnce(&MeldIntegrationService::OnParseCryptoTransferWidgetCreate,
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }

diff --git a/components/brave_wallet/browser/simple_hash_client.cc b/components/brave_wallet/browser/simple_hash_client.cc
@@ -273,8 +273,8 @@ void SimpleHashClient::OnFetchNFTsFromSimpleHash(
 
   std::optional<std::pair<std::optional<std::string>,
                           std::vector<mojom::BlockchainTokenPtr>>>
-      result = ParseNFTsFromSimpleHash(api_request_result.TakeBody(), skip_spam,
-                                       only_spam);
+      result = ParseNFTsFromSimpleHash(api_request_result.value_body(),
+                                       skip_spam, only_spam);
   if (!result) {
     std::move(callback).Run(std::move(nfts), std::nullopt);
     return;
@@ -369,7 +369,7 @@ void SimpleHashClient::OnFetchSolCompressedNftProofData(
   }
 
   std::move(callback).Run(
-      ParseSolCompressedNftProofData(api_request_result.TakeBody()));
+      ParseSolCompressedNftProofData(api_request_result.value_body()));
 }
 
 void SimpleHashClient::GetNftBalances(
@@ -410,7 +410,7 @@ void SimpleHashClient::OnGetNftsForBalances(
     return;
   }
 
-  auto owners = ParseBalances(api_request_result.TakeBody());
+  auto owners = ParseBalances(api_request_result.value_body());
 
   if (!owners) {
     std::move(callback).Run(base::unexpected(InternalError()));
@@ -475,7 +475,7 @@ void SimpleHashClient::OnGetNftsForMetadatas(
   }
 
   // A map of NftIdentifierPtr to their metadata
-  auto metadatas = ParseMetadatas(api_request_result.TakeBody());
+  auto metadatas = ParseMetadatas(api_request_result.value_body());
   if (!metadatas) {
     std::move(callback).Run(base::unexpected(InternalError()));
     return;
@@ -534,7 +534,7 @@ void SimpleHashClient::OnGetNfts(
   }
 
   auto result =
-      ParseNFTsFromSimpleHash(api_request_result.TakeBody(),
+      ParseNFTsFromSimpleHash(api_request_result.value_body(),
                               false /* skip_spam */, false /* only_spam */);
 
   // Add the NFT results