Merge pull request #26120 from brave/issues/41767

[ads] General code health

components/brave_ads/core/internal/BUILD.gn

@@ -1251,6 +1251,7 @@ static_library("internal") {
     "//crypto",
     "//net",
     "//sql",
+    "//third_party/abseil-cpp:absl",
     "//third_party/boringssl",
     "//third_party/re2",
     "//third_party/zlib",

components/brave_ads/core/internal/account/issuers/issuers_constants.h

@@ -8,7 +8,7 @@
 
 namespace brave_ads {
 
-constexpr int kIssuersServerVersion = 3;
+inline constexpr int kIssuersServerVersion = 3;
 
 }  // namespace brave_ads
 

components/brave_ads/core/internal/account/tokens/payment_tokens/payment_token_value_util_unittest.cc

@@ -23,14 +23,14 @@ constexpr char kPaymentTokensAsJson[] =
             "ad_type": "ad_notification",
             "confirmation_type": "view",
             "public_key": "RJ2i/o/pZkrH+i0aGEMY1G9FXtd7Q7gfRi3YdNRnDDk=",
-            "transaction_id": "0d9de7ce-b3f9-4158-8726-23d52b9457c6",
+            "transaction_id": "8b742869-6e4a-490c-ac31-31b49130098a",
             "unblinded_token": "PLowz2WF2eGD5zfwZjk9p76HXBLDKMq/3EAZHeG/fE2XGQ48jyte+Ve50ZlasOuYL5mwA8CU2aFMlJrt3DDgC3B1+VD/uyHPfa/+bwYRrpVH5YwNSDEydVx8S4r+BYVY"
           },
           {
             "ad_type": "ad_notification",
             "confirmation_type": "view",
             "public_key": "RJ2i/o/pZkrH+i0aGEMY1G9FXtd7Q7gfRi3YdNRnDDk=",
-            "transaction_id": "0d9de7ce-b3f9-4158-8726-23d52b9457c6",
+            "transaction_id": "8b742869-6e4a-490c-ac31-31b49130098a",
             "unblinded_token": "hfrMEltWLuzbKQ02Qixh5C/DWiJbdOoaGaidKZ7Mv+cRq5fyxJqemE/MPlARPhl6NgXPHUeyaxzd6/Lk6YHlfXbBA023DYvGMHoKm15NP/nWnZ1V3iLkgOOHZuk80Z4K"
           }
         ])";

components/brave_ads/core/internal/account/tokens/payment_tokens/payment_tokens_test_util.cc

@@ -3,13 +3,15 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at https://mozilla.org/MPL/2.0/. */
 
+#include "brave/components/brave_ads/core/internal/account/tokens/payment_tokens/payment_tokens_test_util.h"
+
 #include <cstddef>
 #include <string>
 #include <vector>
 
 #include "base/check_op.h"
 #include "brave/components/brave_ads/core/internal/account/tokens/payment_tokens/payment_tokens.h"
-#include "brave/components/brave_ads/core/internal/account/tokens/payment_tokens/payment_tokens_test_util.h"
+#include "brave/components/brave_ads/core/internal/account/transactions/transaction_test_constants.h"
 #include "brave/components/brave_ads/core/internal/common/challenge_bypass_ristretto/public_key.h"
 #include "brave/components/brave_ads/core/internal/common/challenge_bypass_ristretto/unblinded_token.h"
 #include "brave/components/brave_ads/core/internal/deprecated/confirmations/confirmation_state_manager.h"
@@ -21,7 +23,7 @@ namespace {
 PaymentTokenInfo BuildPaymentToken(const std::string& payment_token_base64) {
   PaymentTokenInfo payment_token;
 
-  payment_token.transaction_id = "0d9de7ce-b3f9-4158-8726-23d52b9457c6";
+  payment_token.transaction_id = test::kTransactionId;
 
   payment_token.unblinded_token = cbr::UnblindedToken(payment_token_base64);
   CHECK(payment_token.unblinded_token.has_value());

components/brave_ads/core/internal/account/utility/tokens_constants.h

@@ -8,7 +8,7 @@
 
 namespace brave_ads {
 
-constexpr int kTokensServerVersion = 3;
+inline constexpr int kTokensServerVersion = 3;
 
 }  // namespace brave_ads
 

components/brave_ads/core/internal/ad_units/ad_test_constants.h

@@ -9,27 +9,37 @@
 namespace brave_ads::test {
 
 inline constexpr char kPlacementId[] = "9bac9ae4-693c-4569-9b3e-300e357780cf";
+inline constexpr char kAnotherPlacementId[] =
+    "75994a99-65e4-4171-b134-10859dfc6d18";
 inline constexpr char kMissingPlacementId[] =
     "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx";
 inline constexpr char kInvalidPlacementId[] = "";
 
 inline constexpr char kCreativeInstanceId[] =
     "546fe7b0-5047-4f28-a11c-81f14edcf0f6";
+inline constexpr char kAnotherCreativeInstanceId[] =
+    "e1c4c9a3-a05b-4d6a-83f9-72260729ebb0";
 inline constexpr char kMissingCreativeInstanceId[] =
     "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx";
 inline constexpr char kInvalidCreativeInstanceId[] = "";
 
 inline constexpr char kCreativeSetId[] = "c2ba3e7d-f688-4bc4-a053-cbe7ac1e6123";
+inline constexpr char kAnotherCreativeSetId[] =
+    "e31e1905-e8a3-4461-9930-82f68e70d8ba";
 inline constexpr char kMissingCreativeSetId[] =
     "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx";
 inline constexpr char kInvalidCreativeSetId[] = "";
 
 inline constexpr char kCampaignId[] = "84197fc8-830a-4a8e-8339-7a70c2bfa104";
+inline constexpr char kAnotherCampaignId[] =
+    "b164214f-670d-4eb2-8a71-92fb2dd805ca";
 inline constexpr char kMissingCampaignId[] =
     "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx";
 inline constexpr char kInvalidCampaignId[] = "";
 
 inline constexpr char kAdvertiserId[] = "5484a63f-eb99-4ba5-a3b0-8c25d3c0e4b2";
+inline constexpr char kAnotherAdvertiserId[] =
+    "5b010b85-39ed-476a-b8c1-1746db2f86b7";
 inline constexpr char kMissingAdvertiserId[] =
     "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx";
 inline constexpr char kInvalidAdvertiserId[] = "";

components/brave_ads/core/internal/ad_units/creative_ad_cache.cc

@@ -5,8 +5,6 @@
 
 #include "brave/components/brave_ads/core/internal/ad_units/creative_ad_cache.h"
 
-#include <optional>
-
 #include "base/containers/contains.h"
 #include "base/functional/overloaded.h"
 #include "brave/components/brave_ads/core/internal/tabs/tab_info.h"
@@ -16,21 +14,21 @@ namespace brave_ads {
 
 namespace {
 
-bool IsCreativeAdVariantValid(const CreativeAdVariant& creative_ad_variant) {
+bool IsValid(const CreativeAdVariant& creative_ad_variant) {
   return absl::visit(
       base::Overloaded{
-          [](const mojom::CreativeSearchResultAdInfoPtr& search_result_ad)
-              -> bool { return !!search_result_ad; }},
+          [](const mojom::CreativeSearchResultAdInfoPtr& mojom_creative_ad)
+              -> bool { return !!mojom_creative_ad; }},
       creative_ad_variant);
 }
 
-std::optional<CreativeAdVariant> CloneCreativeAdVariant(
+std::optional<CreativeAdVariant> Clone(
     const CreativeAdVariant& creative_ad_variant) {
   return absl::visit(
       base::Overloaded{
-          [](const mojom::CreativeSearchResultAdInfoPtr& search_result_ad)
+          [](const mojom::CreativeSearchResultAdInfoPtr& mojom_creative_ad)
               -> std::optional<CreativeAdVariant> {
-            return search_result_ad.Clone();
+            return mojom_creative_ad.Clone();
           }},
       creative_ad_variant);
 }
@@ -47,7 +45,7 @@ CreativeAdCache::~CreativeAdCache() {
 
 void CreativeAdCache::MaybeAdd(const std::string& placement_id,
                                CreativeAdVariant creative_ad_variant) {
-  if (!IsCreativeAdVariantValid(creative_ad_variant)) {
+  if (!IsValid(creative_ad_variant)) {
     return;
   }
 
@@ -58,19 +56,17 @@ void CreativeAdCache::MaybeAdd(const std::string& placement_id,
   }
 }
 
-std::optional<CreativeAdVariant> CreativeAdCache::MaybeGet(
-    const std::string& placement_id) {
+///////////////////////////////////////////////////////////////////////////////
+
+std::optional<CreativeAdVariant> CreativeAdCache::MaybeGetCreativeAdVariant(
+    const std::string& placement_id) const {
   const auto iter = creative_ad_variants_.find(placement_id);
   if (iter == creative_ad_variants_.cend()) {
     return std::nullopt;
   }
   const auto& creative_ad_variant = iter->second;
 
-  return CloneCreativeAdVariant(creative_ad_variant);
-}
-
-void CreativeAdCache::OnDidCloseTab(const int32_t tab_id) {
-  PurgePlacements(tab_id);
+  return Clone(creative_ad_variant);
 }
 
 void CreativeAdCache::PurgePlacements(const int32_t tab_id) {
@@ -84,4 +80,8 @@ void CreativeAdCache::PurgePlacements(const int32_t tab_id) {
   placement_ids_.erase(tab_id);
 }
 
+void CreativeAdCache::OnDidCloseTab(const int32_t tab_id) {
+  PurgePlacements(tab_id);
+}
+
 }  // namespace brave_ads

components/brave_ads/core/internal/ad_units/creative_ad_cache.h

@@ -6,7 +6,9 @@
 #ifndef BRAVE_COMPONENTS_BRAVE_ADS_CORE_INTERNAL_AD_UNITS_CREATIVE_AD_CACHE_H_
 #define BRAVE_COMPONENTS_BRAVE_ADS_CORE_INTERNAL_AD_UNITS_CREATIVE_AD_CACHE_H_
 
+#include <cstdint>
 #include <map>
+#include <optional>
 #include <string>
 #include <utility>
 #include <vector>
@@ -36,13 +38,17 @@ class CreativeAdCache final : public TabManagerObserver {
 
   ~CreativeAdCache() override;
 
+  // Adds a creative ad to the cache for the given `placement_id` if it is valid
+  // and there is a visible tab.
   void MaybeAdd(const std::string& placement_id,
                 CreativeAdVariant creative_ad_variant);
 
+  // Gets a creative ad from the cache if it exists for the given
+  // `placement_id`.
   template <typename T>
   std::optional<T> MaybeGet(const std::string& placement_id) {
     std::optional<CreativeAdVariant> creative_ad_variant =
-        MaybeGet(placement_id);
+        MaybeGetCreativeAdVariant(placement_id);
     if (!creative_ad_variant) {
       return std::nullopt;
     }
@@ -55,13 +61,14 @@ class CreativeAdCache final : public TabManagerObserver {
   }
 
  private:
-  // TabManagerObserver:
-  void OnDidCloseTab(int32_t tab_id) override;
-
-  std::optional<CreativeAdVariant> MaybeGet(const std::string& placement_id);
+  std::optional<CreativeAdVariant> MaybeGetCreativeAdVariant(
+      const std::string& placement_id) const;
 
   void PurgePlacements(int32_t tab_id);
 
+  // TabManagerObserver:
+  void OnDidCloseTab(int32_t tab_id) override;
+
   CreativeAdVariantMap creative_ad_variants_;
   PlacementIdMap placement_ids_;
 };