Skip to content

Commit

Permalink
[CodeHealth][ads] Use base::ScopedObservation pt.1
Browse files Browse the repository at this point in the history
This change replaces the use of local `raw_ptr`s managing observers
manually, for the use of ``base::ScopedObservation`, which is more
idiomatic.

Resolves brave/brave-browser#41607
  • Loading branch information
cdesouza-chromium committed Oct 13, 2024
1 parent 27fdd69 commit 1e63064
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 62 deletions.
24 changes: 8 additions & 16 deletions browser/brave_ads/analytics/p3a/brave_stats_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,7 @@ BraveStatsHelper::BraveStatsHelper()
profile_manager_observer_.Observe(profile_manager_);
}

BraveStatsHelper::~BraveStatsHelper() {
if (current_profile_) {
current_profile_->RemoveObserver(this);
}
}
BraveStatsHelper::~BraveStatsHelper() = default;

void BraveStatsHelper::RegisterLocalStatePrefs(PrefRegistrySimple* registry) {
registry->RegisterBooleanPref(prefs::kEnabledForLastProfile, false);
Expand All @@ -65,25 +61,23 @@ void BraveStatsHelper::OnProfileAdded(Profile* profile) {
}

void BraveStatsHelper::OnProfileWillBeDestroyed(Profile* profile) {
if (profile != current_profile_) {
if (!current_profile_observation_.IsObservingSource(profile)) {
return;
}
profile->RemoveObserver(this);
current_profile_ = nullptr;
current_profile_observation_.Reset();
#if !BUILDFLAG(IS_ANDROID)
last_used_profile_pref_change_registrar_.RemoveAll();
#endif
ads_enabled_pref_change_registrar_.RemoveAll();
}

void BraveStatsHelper::OnProfileManagerDestroying() {
if (current_profile_ != nullptr) {
if (current_profile_observation_.IsObserving()) {
#if !BUILDFLAG(IS_ANDROID)
last_used_profile_pref_change_registrar_.RemoveAll();
#endif
ads_enabled_pref_change_registrar_.RemoveAll();
current_profile_->RemoveObserver(this);
current_profile_ = nullptr;
current_profile_observation_.Reset();
}
profile_manager_observer_.Reset();
}
Expand All @@ -109,12 +103,10 @@ PrefService* BraveStatsHelper::GetLastUsedProfilePrefs() {
if (profile == nullptr || profile->IsOffTheRecord()) {
return nullptr;
}
if (current_profile_ != nullptr) {
current_profile_->RemoveObserver(this);
current_profile_ = nullptr;
if (current_profile_observation_.IsObserving()) {
current_profile_observation_.Reset();
}
current_profile_ = profile;
profile->AddObserver(this);
current_profile_observation_.Observe(profile);
return profile->GetPrefs();
#endif
}
Expand Down
3 changes: 2 additions & 1 deletion browser/brave_ads/analytics/p3a/brave_stats_helper.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ class BraveStatsHelper : public ProfileManagerObserver, public ProfileObserver {
PrefChangeRegistrar last_used_profile_pref_change_registrar_;
#endif
PrefChangeRegistrar ads_enabled_pref_change_registrar_;
raw_ptr<Profile> current_profile_ = nullptr;
base::ScopedObservation<Profile, ProfileObserver>
current_profile_observation_{this};

base::ScopedObservation<ProfileManager, ProfileManagerObserver>
profile_manager_observer_{this};
Expand Down
9 changes: 2 additions & 7 deletions browser/ui/views/brave_ads/notification_ad_popup.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,18 +100,13 @@ NotificationAdPopup::NotificationAdPopup(

display::Screen* screen = display::Screen::GetScreen();
if (screen) {
screen->AddObserver(this);
screen_observation_.Observe(screen);
}

FadeIn();
}

NotificationAdPopup::~NotificationAdPopup() {
display::Screen* screen = display::Screen::GetScreen();
if (screen) {
screen->RemoveObserver(this);
}
}
NotificationAdPopup::~NotificationAdPopup() = default;

// static
void NotificationAdPopup::SetDisableFadeInAnimationForTesting(
Expand Down
7 changes: 7 additions & 0 deletions browser/ui/views/brave_ads/notification_ad_popup.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@

class Profile;

namespace display {
class Screen;
} // namespace display

namespace gfx {
class LinearAnimation;
class Insets;
Expand Down Expand Up @@ -157,6 +161,9 @@ class NotificationAdPopup : public views::WidgetDelegateView,

base::ScopedObservation<views::Widget, views::WidgetObserver>
widget_observation_{this};

base::ScopedObservation<display::Screen, display::DisplayObserver>
screen_observation_{this};
};

} // namespace brave_ads
Expand Down
54 changes: 23 additions & 31 deletions components/brave_ads/browser/ads_service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,6 @@ AdsServiceImpl::AdsServiceImpl(
local_state_(local_state),
url_loader_(std::move(url_loader)),
channel_name_(channel_name),
resource_component_(resource_component),
history_service_(history_service),
ads_tooltips_delegate_(std::move(ads_tooltips_delegate)),
device_id_(std::move(device_id)),
Expand All @@ -203,16 +202,17 @@ AdsServiceImpl::AdsServiceImpl(
{base::MayBlock(), base::TaskPriority::BEST_EFFORT,
base::TaskShutdownBehavior::BLOCK_SHUTDOWN})),
ads_service_path_(profile_path.AppendASCII("ads_service")),
rewards_service_(rewards_service),
bat_ads_client_associated_receiver_(this) {
CHECK(device_id_);
CHECK(bat_ads_service_factory_);
CHECK(rewards_service_);
CHECK(rewards_service);

if (!history_service_ || !local_state_) {
CHECK_IS_TEST();
}

rewards_service_observation_.Observe(rewards_service);

if (CanStartBatAdsService()) {
bat_ads_client_notifier_pending_receiver_ =
bat_ads_client_notifier_remote_.BindNewPipeAndPassReceiver();
Expand All @@ -224,32 +224,22 @@ AdsServiceImpl::AdsServiceImpl(

GetDeviceIdAndMaybeStartBatAdsService();

if (resource_component_) {
resource_component_->AddObserver(this);
if (resource_component) {
resource_component_observation_.Observe(resource_component);
} else {
CHECK_IS_TEST();
}

rewards_service_->AddObserver(this);
}

AdsServiceImpl::~AdsServiceImpl() {
if (resource_component_) {
resource_component_->RemoveObserver(this);
}

bat_ads_observer_receiver_.reset();

rewards_service_->RemoveObserver(this);
}
AdsServiceImpl::~AdsServiceImpl() = default;

///////////////////////////////////////////////////////////////////////////////

bool AdsServiceImpl::IsBatAdsServiceBound() const {
return bat_ads_service_remote_.is_bound();
}

void AdsServiceImpl::RegisterResourceComponents() const {
void AdsServiceImpl::RegisterResourceComponents() {
RegisterResourceComponentsForCurrentCountryCode();

if (UserHasOptedInToNotificationAds()) {
Expand All @@ -268,18 +258,19 @@ void AdsServiceImpl::Migrate() {
}
}

void AdsServiceImpl::RegisterResourceComponentsForCurrentCountryCode() const {
if (resource_component_) {
const std::string country_code = brave_l10n::GetCountryCode(local_state_);
resource_component_->RegisterComponentForCountryCode(country_code);
void AdsServiceImpl::RegisterResourceComponentsForCurrentCountryCode() {
if (resource_component_observation_.IsObserving()) {
resource_component_observation_.GetSource()
->RegisterComponentForCountryCode(
brave_l10n::GetCountryCode(local_state_));
}
}

void AdsServiceImpl::RegisterResourceComponentsForDefaultLanguageCode() const {
if (resource_component_) {
const std::string& locale = brave_l10n::GetDefaultLocaleString();
const std::string language_code = brave_l10n::GetISOLanguageCode(locale);
resource_component_->RegisterComponentForLanguageCode(language_code);
void AdsServiceImpl::RegisterResourceComponentsForDefaultLanguageCode() {
if (resource_component_observation_.IsObserving()) {
resource_component_observation_.GetSource()
->RegisterComponentForLanguageCode(brave_l10n::GetISOLanguageCode(
brave_l10n::GetDefaultLocaleString()));
}
}

Expand Down Expand Up @@ -434,7 +425,7 @@ void AdsServiceImpl::InitializeDatabase() {

void AdsServiceImpl::InitializeRewardsWallet(
const size_t current_start_number) {
rewards_service_->GetRewardsWallet(
rewards_service_observation_.GetSource()->GetRewardsWallet(
base::BindOnce(&AdsServiceImpl::InitializeRewardsWalletCallback,
weak_ptr_factory_.GetWeakPtr(), current_start_number));
}
Expand Down Expand Up @@ -730,7 +721,7 @@ void AdsServiceImpl::NotifyPrefChanged(const std::string& path) const {
}

void AdsServiceImpl::GetRewardsWallet() {
rewards_service_->GetRewardsWallet(
rewards_service_observation_.GetSource()->GetRewardsWallet(
base::BindOnce(&AdsServiceImpl::NotifyRewardsWalletDidUpdate,
weak_ptr_factory_.GetWeakPtr()));
}
Expand Down Expand Up @@ -1664,12 +1655,12 @@ void AdsServiceImpl::LoadResourceComponent(
const std::string& id,
const int version,
LoadResourceComponentCallback callback) {
if (!resource_component_) {
if (!resource_component_observation_.IsObserving()) {
return std::move(callback).Run({});
}

std::optional<base::FilePath> file_path =
resource_component_->MaybeGetPath(id, version);
resource_component_observation_.GetSource()->MaybeGetPath(id, version);
if (!file_path) {
return std::move(callback).Run({});
}
Expand Down Expand Up @@ -1811,7 +1802,8 @@ void AdsServiceImpl::Log(const std::string& file,
const int32_t line,
const int32_t verbose_level,
const std::string& message) {
rewards_service_->WriteDiagnosticLog(file, line, verbose_level, message);
rewards_service_observation_.GetSource()->WriteDiagnosticLog(
file, line, verbose_level, message);

const int vlog_level =
::logging::GetVlogLevelHelper(file.c_str(), file.length());
Expand Down
16 changes: 9 additions & 7 deletions components/brave_ads/browser/ads_service_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,9 @@ class AdsServiceImpl final : public AdsService,

bool IsBatAdsServiceBound() const;

void RegisterResourceComponents() const;
void RegisterResourceComponentsForCurrentCountryCode() const;
void RegisterResourceComponentsForDefaultLanguageCode() const;
void RegisterResourceComponents();
void RegisterResourceComponentsForCurrentCountryCode();
void RegisterResourceComponentsForDefaultLanguageCode();

void Migrate();

Expand Down Expand Up @@ -449,8 +449,9 @@ class AdsServiceImpl final : public AdsService,

const std::string channel_name_;

const raw_ptr<brave_ads::ResourceComponent> resource_component_ =
nullptr; // NOT OWNED
base::ScopedObservation<brave_ads::ResourceComponent,
ResourceComponentObserver>
resource_component_observation_{this};

const raw_ptr<history::HistoryService> history_service_ =
nullptr; // NOT OWNED
Expand All @@ -465,8 +466,9 @@ class AdsServiceImpl final : public AdsService,

const base::FilePath ads_service_path_;

const raw_ptr<brave_rewards::RewardsService> rewards_service_{
nullptr}; // NOT OWNED
base::ScopedObservation<brave_rewards::RewardsService,
brave_rewards::RewardsServiceObserver>
rewards_service_observation_{this};

mojo::Receiver<bat_ads::mojom::BatAdsObserver> bat_ads_observer_receiver_{
this};
Expand Down

0 comments on commit 1e63064

Please sign in to comment.