Merge pull request #1352 from brave/fix-2972

Hardening is_enabled check for Ads Service
brave · Jan 16, 2019 · 6446479 · 6446479
2 parents bd404eb + 13deb15
commit 6446479
Show file tree

Hide file tree

Showing 18 changed files with 111 additions and 72 deletions.
diff --git a/browser/ui/webui/brave_rewards_ui.cc b/browser/ui/webui/brave_rewards_ui.cc
@@ -610,22 +610,7 @@ void RewardsDOMHandler::SaveSetting(const base::ListValue* args) {
     args->GetString(1, &value);
 
     if (key == "enabledMain") {
-      bool rewards_enabled = (value == "true");
-
-      if (ads_service_) {
-        if (rewards_enabled) {
-          // When Rewards are enabled, set Ads enabled to whatever UI value is
-          bool ads_ui_enabled = ads_service_->is_ui_enabled();
-          ads_service_->set_ads_enabled(ads_ui_enabled);
-        } else {
-          // When Rewards are disabled, cache UI value and disable Ads
-          bool ads_enabled = ads_service_->is_enabled();
-          ads_service_->set_ads_ui_enabled(ads_enabled);
-          ads_service_->set_ads_enabled(rewards_enabled);
-        }
-      }
-
-      rewards_service_->SetRewardsMainEnabled(rewards_enabled);
+      rewards_service_->SetRewardsMainEnabled(value == "true");
     }
 
     if (key == "contributionMonthly") {
@@ -824,7 +809,7 @@ void RewardsDOMHandler::GetAdsData(const base::ListValue *args) {
     base::DictionaryValue adsData;
 
     bool ads_ui_enabled;
-    bool ads_enabled = ads_service_->is_ui_enabled();
+    bool ads_enabled = ads_service_->is_enabled();
     int ads_per_hour = ads_service_->ads_per_hour();
 
     #if BUILDFLAG(BRAVE_ADS_ENABLED)
@@ -850,7 +835,6 @@ void RewardsDOMHandler::SaveAdsSetting(const base::ListValue* args) {
 
     if (key == "adsEnabled") {
       ads_service_->set_ads_enabled(value == "true");
-      ads_service_->set_ads_ui_enabled(value == "true");
     }
 
     if (key == "adsPerHour") {

diff --git a/common/pref_names.cc b/common/pref_names.cc
@@ -31,15 +31,6 @@ const char kReferralHeaders[] = "brave.referral.headers";
 const char kReferralCheckedForPromoCodeFile[] = "brave.referral.checked_for_promo_code_file";
 const char kHTTPSEVerywhereControlType[] = "brave.https_everywhere_default";
 const char kNoScriptControlType[] = "brave.no_script_default";
-const char kRewardsNotifications[] = "brave.rewards.notifications";
-const char kRewardsNotificationTimerInterval[] = "brave.rewards.notification_timer_interval";
-const char kRewardsBackupNotificationFrequency[] =
-    "brave.rewards.backup_notification_frequency";
-const char kRewardsBackupNotificationInterval[] =
-    "brave.rewards.backup_notification_interval";
-const char kRewardsBackupSucceeded[] = "brave.rewards.backup_succeeded";
-const char kRewardsUserHasFunded[] = "brave.rewards.user_has_funded";
-const char kRewardsAddFundsNotification[] = "brave.rewards.add_funds_notification";
 const char kMigratedMuonProfile[] = "brave.muon.migrated_profile";
 const char kBravePaymentsPinnedItemCount[] = "brave.muon.import_pinned_item_count";
 const char kWebTorrentEnabled[] = "brave.webtorrent_enabled";

diff --git a/common/pref_names.h b/common/pref_names.h
@@ -30,13 +30,6 @@ extern const char kReferralHeaders[];
 extern const char kReferralCheckedForPromoCodeFile[];
 extern const char kHTTPSEVerywhereControlType[];
 extern const char kNoScriptControlType[];
-extern const char kRewardsNotifications[];
-extern const char kRewardsNotificationTimerInterval[];
-extern const char kRewardsBackupNotificationFrequency[];
-extern const char kRewardsBackupNotificationInterval[];
-extern const char kRewardsBackupSucceeded[];
-extern const char kRewardsUserHasFunded[];
-extern const char kRewardsAddFundsNotification[];
 extern const char kMigratedMuonProfile[];
 extern const char kBravePaymentsPinnedItemCount[];
 extern const char kWebTorrentEnabled[];

diff --git a/components/brave_ads/browser/BUILD.gn b/components/brave_ads/browser/BUILD.gn
@@ -16,6 +16,8 @@ source_set("browser") {
   deps = [
     "//base",
     "//brave/components/brave_ads/common",
+    "//brave/components/brave_rewards/common",
+    "//brave/components/brave_rewards/browser",
     "//components/dom_distiller/content/browser",
     "//components/dom_distiller/core",
     "//components/keyed_service/content",

diff --git a/components/brave_ads/browser/ads_service.h b/components/brave_ads/browser/ads_service.h
@@ -18,11 +18,9 @@ class AdsService : public KeyedService {
   AdsService() = default;
 
   virtual bool is_enabled() const = 0;
-  virtual bool is_ui_enabled() const = 0;
   virtual uint64_t ads_per_hour() const = 0;
 
   virtual void set_ads_enabled(bool enabled) = 0;
-  virtual void set_ads_ui_enabled(bool enabled) = 0;
   virtual void set_ads_per_hour(int ads_per_hour) = 0;
 
   // ads::Ads proxy

diff --git a/components/brave_ads/browser/ads_service_factory.cc b/components/brave_ads/browser/ads_service_factory.cc
@@ -16,6 +16,7 @@
 #include "brave/components/brave_ads/browser/ads_service_impl.h"
 #include "chrome/browser/dom_distiller/dom_distiller_service_factory.h"
 #include "chrome/browser/notifications/notification_display_service_factory.h"
+#include "brave/components/brave_rewards/browser/rewards_service_factory.h"
 #endif
 
 namespace brave_ads {
@@ -42,6 +43,7 @@ AdsServiceFactory::AdsServiceFactory()
 #if BUILDFLAG(BRAVE_ADS_ENABLED)
   DependsOn(NotificationDisplayServiceFactory::GetInstance());
   DependsOn(dom_distiller::DomDistillerServiceFactory::GetInstance());
+  DependsOn(brave_rewards::RewardsServiceFactory::GetInstance());
 #endif
 }
 
@@ -75,7 +77,6 @@ bool AdsServiceFactory::ServiceIsNULLWhileTesting() const {
 void AdsServiceFactory::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterBooleanPref(prefs::kBraveAdsEnabled, false);
-  registry->RegisterBooleanPref(prefs::kBraveAdsUIEnabled, false);
   registry->RegisterUint64Pref(prefs::kBraveAdsPerHour, 2);
   registry->RegisterUint64Pref(prefs::kBraveAdsPerDay, 6);
   registry->RegisterIntegerPref(prefs::kBraveAdsIdleThreshold, 15);

diff --git a/components/brave_ads/browser/ads_service_impl.cc b/components/brave_ads/browser/ads_service_impl.cc
@@ -21,6 +21,7 @@
 #include "brave/components/brave_ads/browser/ad_notification.h"
 #include "brave/components/brave_ads/browser/bundle_state_database.h"
 #include "brave/components/brave_ads/common/pref_names.h"
+#include "brave/components/brave_rewards/common/pref_names.h"
 #include "brave/components/brave_ads/common/switches.h"
 #include "brave/components/services/bat_ads/public/cpp/ads_client_mojo_bridge.h"
 #include "brave/components/services/bat_ads/public/interfaces/bat_ads.mojom.h"
@@ -266,6 +267,10 @@ AdsServiceImpl::AdsServiceImpl(Profile* profile) :
       prefs::kBraveAdsEnabled,
       base::Bind(&AdsServiceImpl::OnPrefsChanged,
                  base::Unretained(this)));
+  profile_pref_change_registrar_.Add(
+      brave_rewards::prefs::kBraveRewardsEnabled,
+      base::Bind(&AdsServiceImpl::OnPrefsChanged,
+                 base::Unretained(this)));
   profile_pref_change_registrar_.Add(
       prefs::kBraveAdsIdleThreshold,
       base::Bind(&AdsServiceImpl::OnPrefsChanged,
@@ -429,7 +434,8 @@ void AdsServiceImpl::Shutdown() {
 }
 
 void AdsServiceImpl::OnPrefsChanged(const std::string& pref) {
-  if (pref == prefs::kBraveAdsEnabled) {
+  if (pref == prefs::kBraveAdsEnabled ||
+      pref == brave_rewards::prefs::kBraveRewardsEnabled) {
     if (is_enabled()) {
       Start();
     } else if (!is_enabled()) {
@@ -441,11 +447,9 @@ void AdsServiceImpl::OnPrefsChanged(const std::string& pref) {
 }
 
 bool AdsServiceImpl::is_enabled() const {
-  return profile_->GetPrefs()->GetBoolean(prefs::kBraveAdsEnabled);
-}
-
-bool AdsServiceImpl::is_ui_enabled() const {
-  return profile_->GetPrefs()->GetBoolean(prefs::kBraveAdsUIEnabled);
+  bool ads_enabled = profile_->GetPrefs()->GetBoolean(prefs::kBraveAdsEnabled);
+  bool rewards_enabled = profile_->GetPrefs()->GetBoolean(brave_rewards::prefs::kBraveRewardsEnabled);
+  return (ads_enabled && rewards_enabled);
 }
 
 bool AdsServiceImpl::IsAdsEnabled() const {
@@ -456,10 +460,6 @@ void AdsServiceImpl::set_ads_enabled(bool enabled) {
   profile_->GetPrefs()->SetBoolean(prefs::kBraveAdsEnabled, enabled);
 }
 
-void AdsServiceImpl::set_ads_ui_enabled(bool enabled) {
-  profile_->GetPrefs()->SetBoolean(prefs::kBraveAdsUIEnabled, enabled);
-}
-
 void AdsServiceImpl::set_ads_per_hour(int ads_per_hour) {
   profile_->GetPrefs()->SetUint64(prefs::kBraveAdsPerHour, ads_per_hour);
 }

diff --git a/components/brave_ads/browser/ads_service_impl.h b/components/brave_ads/browser/ads_service_impl.h
@@ -52,11 +52,9 @@ class AdsServiceImpl : public AdsService,
 
   // AdsService implementation
   bool is_enabled() const override;
-  bool is_ui_enabled() const override;
   uint64_t ads_per_hour() const override;
 
   void set_ads_enabled(bool enabled) override;
-  void set_ads_ui_enabled(bool enabled) override;
   void set_ads_per_hour(int ads_per_hour) override;
 
   void TabUpdated(

diff --git a/components/brave_ads/common/pref_names.cc b/components/brave_ads/common/pref_names.cc
@@ -8,7 +8,6 @@ namespace brave_ads {
 namespace prefs {
 
 const char kBraveAdsEnabled[] = "brave.brave_ads.enabled";
-const char kBraveAdsUIEnabled[] = "brave.brave_ads_ui_enabled";
 const char kBraveAdsPerHour[] = "brave.brave_ads.ads_per_hour";
 const char kBraveAdsPerDay[] = "brave.brave_ads.ads_per_day";
 const char kBraveAdsIdleThreshold[] = "brave.brave_ads.idle_threshold";

diff --git a/components/brave_ads/common/pref_names.h b/components/brave_ads/common/pref_names.h
@@ -9,7 +9,6 @@ namespace brave_ads {
 namespace prefs {
 
 extern const char kBraveAdsEnabled[];
-extern const char kBraveAdsUIEnabled[];
 extern const char kBraveAdsPerHour[];
 extern const char kBraveAdsPerDay[];
 extern const char kBraveAdsIdleThreshold[];

diff --git a/components/brave_rewards/browser/BUILD.gn b/components/brave_rewards/browser/BUILD.gn
@@ -48,6 +48,7 @@ source_set("browser") {
 
   deps = [
     "//base",
+    "//brave/components/brave_rewards/common",
     "//components/keyed_service/content",
     "//components/keyed_service/core",
     "//components/sessions",

diff --git a/components/brave_rewards/browser/rewards_notification_service_impl.cc b/components/brave_rewards/browser/rewards_notification_service_impl.cc
@@ -14,7 +14,7 @@
 #include "base/values.h"
 #include "bat/ledger/ledger_callback_handler.h"
 #include "bat/ledger/publisher_info.h"
-#include "brave/common/pref_names.h"
+#include "brave/components/brave_rewards/common/pref_names.h"
 #include "brave/components/brave_rewards/browser/rewards_notification_service_observer.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/prefs/pref_service.h"
@@ -113,7 +113,7 @@ RewardsNotificationServiceImpl::GenerateRewardsNotificationTimestamp() const {
 }
 
 void RewardsNotificationServiceImpl::ReadRewardsNotificationsJSON() {
-  std::string json = profile_->GetPrefs()->GetString(kRewardsNotifications);
+  std::string json = profile_->GetPrefs()->GetString(prefs::kRewardsNotifications);
   if (json.empty())
     return;
   std::unique_ptr<base::DictionaryValue> dictionary =
@@ -227,7 +227,7 @@ void RewardsNotificationServiceImpl::StoreRewardsNotifications() {
     return;
   }
 
-  profile_->GetPrefs()->SetString(kRewardsNotifications, result);
+  profile_->GetPrefs()->SetString(prefs::kRewardsNotifications, result);
 }
 
 void RewardsNotificationServiceImpl::TriggerOnNotificationAdded(

diff --git a/components/brave_rewards/browser/rewards_service.cc b/components/brave_rewards/browser/rewards_service.cc
@@ -5,7 +5,7 @@
 
 #include "base/logging.h"
 #include "base/time/time.h"
-#include "brave/common/pref_names.h"
+#include "brave/components/brave_rewards/common/pref_names.h"
 #include "brave/components/brave_rewards/browser/buildflags/buildflags.h"
 #include "brave/components/brave_rewards/browser/rewards_notification_service_impl.h"
 #include "brave/components/brave_rewards/browser/rewards_service_observer.h"
@@ -41,16 +41,18 @@ void RewardsService::RemoveObserver(RewardsServiceObserver* observer) {
 
 // static
 void RewardsService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
-  registry->RegisterStringPref(kRewardsNotifications, "");
-  registry->RegisterTimeDeltaPref(kRewardsNotificationTimerInterval,
+  registry->RegisterStringPref(prefs::kRewardsNotifications, "");
+  registry->RegisterTimeDeltaPref(prefs::kRewardsNotificationTimerInterval,
                                   base::TimeDelta::FromDays(1));
-  registry->RegisterTimeDeltaPref(kRewardsBackupNotificationFrequency,
+  registry->RegisterTimeDeltaPref(prefs::kRewardsBackupNotificationFrequency,
                                   base::TimeDelta::FromDays(7));
-  registry->RegisterTimeDeltaPref(kRewardsBackupNotificationInterval,
+  registry->RegisterTimeDeltaPref(prefs::kRewardsBackupNotificationInterval,
                                   base::TimeDelta::FromDays(7));
-  registry->RegisterBooleanPref(kRewardsBackupSucceeded, false);
-  registry->RegisterBooleanPref(kRewardsUserHasFunded, false);
-  registry->RegisterTimePref(kRewardsAddFundsNotification, base::Time());
+  registry->RegisterBooleanPref(prefs::kRewardsBackupSucceeded, false);
+  registry->RegisterBooleanPref(prefs::kRewardsUserHasFunded, false);
+  registry->RegisterTimePref(prefs::kRewardsAddFundsNotification, base::Time());
+  registry->RegisterBooleanPref(prefs::kBraveRewardsEnabled, false);
+  registry->RegisterBooleanPref(prefs::kBraveRewardsEnabledMigrated, false);
 }
 
 }  // namespace brave_rewards
diff --git a/components/brave_rewards/browser/rewards_service_impl.cc b/components/brave_rewards/browser/rewards_service_impl.cc
@@ -29,6 +29,7 @@
 #include "bat/ledger/publisher_info.h"
 #include "bat/ledger/wallet_info.h"
 #include "brave/browser/ui/webui/brave_rewards_source.h"
+#include "brave/components/brave_rewards/common/pref_names.h"
 #include "brave/common/pref_names.h"
 #include "brave/components/brave_rewards/browser/auto_contribution_props.h"
 #include "brave/components/brave_rewards/browser/balance_report.h"
@@ -356,20 +357,20 @@ void RewardsServiceImpl::StartLedger() {
 
 void RewardsServiceImpl::MaybeShowBackupNotification(uint64_t boot_stamp) {
   PrefService* pref_service = profile_->GetPrefs();
-  bool user_has_funded = pref_service->GetBoolean(kRewardsUserHasFunded);
-  bool backup_succeeded = pref_service->GetBoolean(kRewardsBackupSucceeded);
+  bool user_has_funded = pref_service->GetBoolean(prefs::kRewardsUserHasFunded);
+  bool backup_succeeded = pref_service->GetBoolean(prefs::kRewardsBackupSucceeded);
   if (user_has_funded && !backup_succeeded) {
     base::Time now = base::Time::Now();
     base::Time boot_timestamp = base::Time::FromDoubleT(boot_stamp);
     base::TimeDelta backup_notification_frequency =
-        pref_service->GetTimeDelta(kRewardsBackupNotificationFrequency);
+        pref_service->GetTimeDelta(prefs::kRewardsBackupNotificationFrequency);
     base::TimeDelta backup_notification_interval =
-        pref_service->GetTimeDelta(kRewardsBackupNotificationInterval);
+        pref_service->GetTimeDelta(prefs::kRewardsBackupNotificationInterval);
     base::TimeDelta elapsed = now - boot_timestamp;
     if (elapsed > backup_notification_interval) {
       base::TimeDelta next_backup_notification_interval =
           backup_notification_interval + backup_notification_frequency;
-      pref_service->SetTimeDelta(kRewardsBackupNotificationInterval,
+      pref_service->SetTimeDelta(prefs::kRewardsBackupNotificationInterval,
                                  next_backup_notification_interval);
       RewardsNotificationService::RewardsNotificationArgs args;
       notification_service_->AddNotification(
@@ -812,6 +813,11 @@ void RewardsServiceImpl::OnLedgerStateLoaded(
 
 void RewardsServiceImpl::LoadPublisherState(
     ledger::LedgerCallbackHandler* handler) {
+  if (!profile_->GetPrefs()->GetBoolean(prefs::kBraveRewardsEnabledMigrated)) {
+    bat_ledger_->GetRewardsMainEnabled(
+        base::BindOnce(&RewardsServiceImpl::SetRewardsMainEnabledPref,
+          AsWeakPtr()));
+  }
   base::PostTaskAndReplyWithResult(file_task_runner_.get(), FROM_HERE,
       base::Bind(&LoadStateOnFileTaskRunner, publisher_state_path_),
       base::Bind(&RewardsServiceImpl::OnPublisherStateLoaded,
@@ -1090,7 +1096,7 @@ void RewardsServiceImpl::TriggerOnWalletInitialized(int error_code) {
 void RewardsServiceImpl::TriggerOnWalletProperties(int error_code,
     std::unique_ptr<ledger::WalletInfo> wallet_info) {
   if (wallet_info && wallet_info->balance_ > 0)
-    profile_->GetPrefs()->SetBoolean(kRewardsUserHasFunded, true);
+    profile_->GetPrefs()->SetBoolean(prefs::kRewardsUserHasFunded, true);
 
   std::unique_ptr<brave_rewards::WalletProperties> wallet_properties;
   for (auto& observer : observers_) {
@@ -1262,6 +1268,7 @@ void RewardsServiceImpl::SetRewardsMainEnabled(bool enabled) {
     return;
   }
 
+  SetRewardsMainEnabledPref(enabled);
   bat_ledger_->SetRewardsMainEnabled(enabled);
   TriggerOnRewardsMainEnabled(enabled);
 }
@@ -1275,6 +1282,15 @@ void RewardsServiceImpl::GetRewardsMainEnabled(
   bat_ledger_->GetRewardsMainEnabled(callback);
 }
 
+void RewardsServiceImpl::SetRewardsMainEnabledPref(bool enabled) {
+  profile_->GetPrefs()->SetBoolean(prefs::kBraveRewardsEnabled, enabled);
+  SetRewardsMainEnabledMigratedPref(true);
+}
+
+void RewardsServiceImpl::SetRewardsMainEnabledMigratedPref(bool enabled) {
+  profile_->GetPrefs()->SetBoolean(prefs::kBraveRewardsEnabledMigrated, enabled);
+}
+
 void RewardsServiceImpl::GetPublisherMinVisitTime(
     const GetPublisherMinVisitTimeCallback& callback) {
   if (!Connected()) {
@@ -1991,7 +2007,7 @@ void RewardsServiceImpl::StartNotificationTimers(bool main_enabled) {
   // Periodic timer, runs once per day by default.
   PrefService* pref_service = profile_->GetPrefs();
   base::TimeDelta periodic_timer_interval =
-      pref_service->GetTimeDelta(kRewardsNotificationTimerInterval);
+      pref_service->GetTimeDelta(prefs::kRewardsNotificationTimerInterval);
   notification_periodic_timer_ = std::make_unique<base::RepeatingTimer>();
   notification_periodic_timer_->Start(
       FROM_HERE, periodic_timer_interval, this,
@@ -2024,15 +2040,15 @@ void RewardsServiceImpl::MaybeShowNotificationAddFunds() {
 
 bool RewardsServiceImpl::ShouldShowNotificationAddFunds() const {
   base::Time next_time =
-      profile_->GetPrefs()->GetTime(kRewardsAddFundsNotification);
+      profile_->GetPrefs()->GetTime(prefs::kRewardsAddFundsNotification);
   return (next_time.is_null() || base::Time::Now() > next_time);
 }
 
 void RewardsServiceImpl::ShowNotificationAddFunds(bool sufficient) {
   if (sufficient) return;
 
   base::Time next_time = base::Time::Now() + base::TimeDelta::FromDays(3);
-  profile_->GetPrefs()->SetTime(kRewardsAddFundsNotification, next_time);
+  profile_->GetPrefs()->SetTime(prefs::kRewardsAddFundsNotification, next_time);
   RewardsNotificationService::RewardsNotificationArgs args;
   notification_service_->AddNotification(
       RewardsNotificationService::REWARDS_NOTIFICATION_INSUFFICIENT_FUNDS, args,
@@ -2122,7 +2138,7 @@ bool RewardsServiceImpl::CheckImported() {
 }
 
 void RewardsServiceImpl::SetBackupCompleted() {
-  profile_->GetPrefs()->SetBoolean(kRewardsBackupSucceeded, true);
+  profile_->GetPrefs()->SetBoolean(prefs::kRewardsBackupSucceeded, true);
 }
 
 void RewardsServiceImpl::OnDonate(

diff --git a/components/brave_rewards/browser/rewards_service_impl.h b/components/brave_rewards/browser/rewards_service_impl.h
@@ -343,6 +343,8 @@ class RewardsServiceImpl : public RewardsService,
   void OnGetAutoContributeProps(
       const GetAutoContributePropsCallback& callback,
       const std::string& json_props);
+  void SetRewardsMainEnabledPref(bool enabled);
+  void SetRewardsMainEnabledMigratedPref(bool enabled);
 
   bool Connected() const;
   void ConnectionClosed();

diff --git a/components/brave_rewards/common/BUILD.gn b/components/brave_rewards/common/BUILD.gn
@@ -0,0 +1,6 @@
+source_set("common") {
+  sources = [
+    "pref_names.cc",
+    "pref_names.h"
+  ]
+}