Skip to content

Commit

Permalink
[ads] Do not discard API calls during ads initialization
Browse files Browse the repository at this point in the history
  • Loading branch information
@aseren
aseren committed Oct 15, 2024
1 parent cdfe99b commit abdf5da
Show file tree
Hide file tree
Showing 21 changed files with 233 additions and 140 deletions.
4 changes: 2 additions & 2 deletions components/brave_ads/core/internal/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,6 @@ static_library("internal") {
"ads.cc",
"ads_client/ads_client_notifier.cc",
"ads_client/ads_client_notifier_observer.cc",
"ads_client/ads_client_notifier_queue.cc",
"ads_client/ads_client_notifier_queue.h",
"ads_client/ads_client_pref_provider.cc",
"ads_client/ads_client_pref_provider.h",
"ads_client/ads_client_util.cc",
Expand Down Expand Up @@ -413,6 +411,8 @@ static_library("internal") {
"common/database/database_table_util.h",
"common/database/database_transaction_util.cc",
"common/database/database_transaction_util.h",
"common/functional/once_closure_queue.cc",
"common/functional/once_closure_queue.h",
"common/instance_id.cc",
"common/instance_id.h",
"common/locale/locale_util.cc",
Expand Down
55 changes: 29 additions & 26 deletions components/brave_ads/core/internal/ads_client/ads_client_notifier.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@
#include <memory>

#include "base/functional/bind.h"
#include "brave/components/brave_ads/core/internal/ads_client/ads_client_notifier_queue.h"
#include "brave/components/brave_ads/core/internal/common/functional/once_closure_queue.h"
#include "url/gurl.h"

namespace brave_ads {

AdsClientNotifier::AdsClientNotifier()
: queue_(std::make_unique<AdsClientNotifierQueue>()) {}
: pending_task_queue_(std::make_unique<OnceClosureQueue>()) {}

AdsClientNotifier::~AdsClientNotifier() = default;

Expand All @@ -34,12 +34,12 @@ void AdsClientNotifier::RemoveObserver(

void AdsClientNotifier::NotifyPendingObservers() {
should_queue_ = false;
queue_->Process();
pending_task_queue_->Process();
}

void AdsClientNotifier::NotifyDidInitializeAds() {
if (should_queue_) {
return queue_->Add(
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyDidInitializeAds,
weak_factory_.GetWeakPtr()));
}
Expand All @@ -53,7 +53,7 @@ void AdsClientNotifier::NotifyRewardsWalletDidUpdate(
const std::string& payment_id,
const std::string& recovery_seed_base64) {
if (should_queue_) {
return queue_->Add(base::BindOnce(
return pending_task_queue_->Add(base::BindOnce(
&AdsClientNotifier::NotifyRewardsWalletDidUpdate,
weak_factory_.GetWeakPtr(), payment_id, recovery_seed_base64));
}
Expand All @@ -65,8 +65,9 @@ void AdsClientNotifier::NotifyRewardsWalletDidUpdate(

void AdsClientNotifier::NotifyLocaleDidChange(const std::string& locale) {
if (should_queue_) {
return queue_->Add(base::BindOnce(&AdsClientNotifier::NotifyLocaleDidChange,
weak_factory_.GetWeakPtr(), locale));
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyLocaleDidChange,
weak_factory_.GetWeakPtr(), locale));
}

for (auto& observer : observers_) {
Expand All @@ -76,8 +77,9 @@ void AdsClientNotifier::NotifyLocaleDidChange(const std::string& locale) {

void AdsClientNotifier::NotifyPrefDidChange(const std::string& path) {
if (should_queue_) {
return queue_->Add(base::BindOnce(&AdsClientNotifier::NotifyPrefDidChange,
weak_factory_.GetWeakPtr(), path));
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyPrefDidChange,
weak_factory_.GetWeakPtr(), path));
}

for (auto& observer : observers_) {
Expand All @@ -89,7 +91,7 @@ void AdsClientNotifier::NotifyResourceComponentDidChange(
const std::string& manifest_version,
const std::string& id) {
if (should_queue_) {
return queue_->Add(
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyResourceComponentDidChange,
weak_factory_.GetWeakPtr(), manifest_version, id));
}
Expand All @@ -102,7 +104,7 @@ void AdsClientNotifier::NotifyResourceComponentDidChange(
void AdsClientNotifier::NotifyDidUnregisterResourceComponent(
const std::string& id) {
if (should_queue_) {
return queue_->Add(
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyDidUnregisterResourceComponent,
weak_factory_.GetWeakPtr(), id));
}
Expand All @@ -117,7 +119,7 @@ void AdsClientNotifier::NotifyTabTextContentDidChange(
const std::vector<GURL>& redirect_chain,
const std::string& text) {
if (should_queue_) {
return queue_->Add(base::BindOnce(
return pending_task_queue_->Add(base::BindOnce(
&AdsClientNotifier::NotifyTabTextContentDidChange,
weak_factory_.GetWeakPtr(), tab_id, redirect_chain, text));
}
Expand All @@ -132,7 +134,7 @@ void AdsClientNotifier::NotifyTabHtmlContentDidChange(
const std::vector<GURL>& redirect_chain,
const std::string& html) {
if (should_queue_) {
return queue_->Add(base::BindOnce(
return pending_task_queue_->Add(base::BindOnce(
&AdsClientNotifier::NotifyTabHtmlContentDidChange,
weak_factory_.GetWeakPtr(), tab_id, redirect_chain, html));
}
Expand All @@ -144,7 +146,7 @@ void AdsClientNotifier::NotifyTabHtmlContentDidChange(

void AdsClientNotifier::NotifyTabDidStartPlayingMedia(const int32_t tab_id) {
if (should_queue_) {
return queue_->Add(
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyTabDidStartPlayingMedia,
weak_factory_.GetWeakPtr(), tab_id));
}
Expand All @@ -156,7 +158,7 @@ void AdsClientNotifier::NotifyTabDidStartPlayingMedia(const int32_t tab_id) {

void AdsClientNotifier::NotifyTabDidStopPlayingMedia(const int32_t tab_id) {
if (should_queue_) {
return queue_->Add(
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyTabDidStopPlayingMedia,
weak_factory_.GetWeakPtr(), tab_id));
}
Expand All @@ -173,7 +175,7 @@ void AdsClientNotifier::NotifyTabDidChange(
const bool is_restoring,
const bool is_visible) {
if (should_queue_) {
return queue_->Add(base::BindOnce(
return pending_task_queue_->Add(base::BindOnce(
&AdsClientNotifier::NotifyTabDidChange, weak_factory_.GetWeakPtr(),
tab_id, redirect_chain, is_new_navigation, is_restoring, is_visible));
}
Expand All @@ -199,8 +201,9 @@ void AdsClientNotifier::NotifyTabDidLoad(const int32_t tab_id,

void AdsClientNotifier::NotifyDidCloseTab(const int32_t tab_id) {
if (should_queue_) {
return queue_->Add(base::BindOnce(&AdsClientNotifier::NotifyDidCloseTab,
weak_factory_.GetWeakPtr(), tab_id));
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyDidCloseTab,
weak_factory_.GetWeakPtr(), tab_id));
}

for (auto& observer : observers_) {
Expand All @@ -211,7 +214,7 @@ void AdsClientNotifier::NotifyDidCloseTab(const int32_t tab_id) {
void AdsClientNotifier::NotifyUserGestureEventTriggered(
const int32_t page_transition_type) {
if (should_queue_) {
return queue_->Add(
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyUserGestureEventTriggered,
weak_factory_.GetWeakPtr(), page_transition_type));
}
Expand All @@ -223,7 +226,7 @@ void AdsClientNotifier::NotifyUserGestureEventTriggered(

void AdsClientNotifier::NotifyUserDidBecomeIdle() {
if (should_queue_) {
return queue_->Add(
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyUserDidBecomeIdle,
weak_factory_.GetWeakPtr()));
}
Expand All @@ -237,7 +240,7 @@ void AdsClientNotifier::NotifyUserDidBecomeActive(
const base::TimeDelta idle_time,
const bool screen_was_locked) {
if (should_queue_) {
return queue_->Add(base::BindOnce(
return pending_task_queue_->Add(base::BindOnce(
&AdsClientNotifier::NotifyUserDidBecomeActive,
weak_factory_.GetWeakPtr(), idle_time, screen_was_locked));
}
Expand All @@ -249,7 +252,7 @@ void AdsClientNotifier::NotifyUserDidBecomeActive(

void AdsClientNotifier::NotifyBrowserDidEnterForeground() {
if (should_queue_) {
return queue_->Add(
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyBrowserDidEnterForeground,
weak_factory_.GetWeakPtr()));
}
Expand All @@ -261,7 +264,7 @@ void AdsClientNotifier::NotifyBrowserDidEnterForeground() {

void AdsClientNotifier::NotifyBrowserDidEnterBackground() {
if (should_queue_) {
return queue_->Add(
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyBrowserDidEnterBackground,
weak_factory_.GetWeakPtr()));
}
Expand All @@ -273,7 +276,7 @@ void AdsClientNotifier::NotifyBrowserDidEnterBackground() {

void AdsClientNotifier::NotifyBrowserDidBecomeActive() {
if (should_queue_) {
return queue_->Add(
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyBrowserDidBecomeActive,
weak_factory_.GetWeakPtr()));
}
Expand All @@ -285,7 +288,7 @@ void AdsClientNotifier::NotifyBrowserDidBecomeActive() {

void AdsClientNotifier::NotifyBrowserDidResignActive() {
if (should_queue_) {
return queue_->Add(
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyBrowserDidResignActive,
weak_factory_.GetWeakPtr()));
}
Expand All @@ -297,7 +300,7 @@ void AdsClientNotifier::NotifyBrowserDidResignActive() {

void AdsClientNotifier::NotifyDidSolveAdaptiveCaptcha() {
if (should_queue_) {
return queue_->Add(
return pending_task_queue_->Add(
base::BindOnce(&AdsClientNotifier::NotifyDidSolveAdaptiveCaptcha,
weak_factory_.GetWeakPtr()));
}
Expand Down
6 changes: 6 additions & 0 deletions components/brave_ads/core/internal/ads_client/ads_client_notifier_for_testing.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@

namespace brave_ads {

void AdsClientNotifierForTesting::NotifyPendingObservers() {
AdsClientNotifier::NotifyPendingObservers();

RunTaskEnvironmentUntilIdle();
}

void AdsClientNotifierForTesting::NotifyDidInitializeAds() {
AdsClientNotifier::NotifyDidInitializeAds();

Expand Down
2 changes: 2 additions & 0 deletions components/brave_ads/core/internal/ads_client/ads_client_notifier_for_testing.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ class AdsClientNotifierForTesting : public AdsClientNotifier {
task_environment_ = task_environment;
}

void NotifyPendingObservers() override;

void NotifyDidInitializeAds() override;

void NotifyLocaleDidChange(const std::string& locale) override;
Expand Down
35 changes: 0 additions & 35 deletions components/brave_ads/core/internal/ads_client/ads_client_notifier_queue.h
View file

This file was deleted.

6 changes: 2 additions & 4 deletions components/brave_ads/core/internal/ads_client/ads_client_notifier_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,6 @@ class BraveAdsAdsClientNotifierTest : public ::testing::Test {

TEST_F(BraveAdsAdsClientNotifierTest, FireQueuedAdsClientNotifications) {
// Arrange
ads_client_notifier_.set_should_queue_for_testing(true);

// Act & Assert
ExpectAdsClientNotifierCallCount(0);
Expand All @@ -198,17 +197,16 @@ TEST_F(
BraveAdsAdsClientNotifierTest,
DoNotFireQueuedAdsClientNotificationsIfNotifyPendingObserversIsNotCalled) {
// Arrange
ads_client_notifier_.set_should_queue_for_testing(true);

// Act & Assert
ExpectAdsClientNotifierCallCount(0);
FireAdsClientNotifiers();
}

TEST_F(BraveAdsAdsClientNotifierTest,
FireAdsClientNotificationsImmediatelyIfNotQueued) {
FireAdsClientNotificationsImmediatelyIfNotifyPendingObserversWasCalled) {
// Arrange
ads_client_notifier_.set_should_queue_for_testing(false);
ads_client_notifier_.NotifyPendingObservers();

// Act & Assert
ExpectAdsClientNotifierCallCount(1);
Expand Down
Loading

0 comments on commit abdf5da

Please sign in to comment.