From 479a0c5d55663b5492fea14abb245f4e72c74ec0 Mon Sep 17 00:00:00 2001 From: brave-builds Date: Fri, 3 Jan 2020 10:03:59 +0000 Subject: [PATCH] Uplift of #3931 (squashed) to beta --- .../brave_ads/browser/ads_service_factory.cc | 3 ++- components/brave_ads/common/pref_names.cc | 6 +++++- components/brave_ads/common/pref_names.h | 3 ++- .../brave_rewards/browser/rewards_p3a.cc | 15 ++++++++++--- .../brave_rewards/browser/rewards_p3a.h | 4 ++++ .../browser/rewards_service_impl.cc | 3 ++- components/p3a/brave_histogram_rewrite.cc | 21 +++++++++++++++++++ components/p3a/brave_p3a_log_store.cc | 20 ++++++++++++------ components/p3a/brave_p3a_log_store.h | 12 ++++++----- components/p3a/brave_p3a_service.cc | 12 ++++++++++- components/p3a/brave_p3a_service.h | 6 ++++-- 11 files changed, 84 insertions(+), 21 deletions(-) diff --git a/components/brave_ads/browser/ads_service_factory.cc b/components/brave_ads/browser/ads_service_factory.cc index 4fb479b56f10..b9d221cc7cf6 100644 --- a/components/brave_ads/browser/ads_service_factory.cc +++ b/components/brave_ads/browser/ads_service_factory.cc @@ -84,7 +84,8 @@ void AdsServiceFactory::RegisterProfilePrefs( registry->RegisterUint64Pref(prefs::kAdsPerDay, 20); registry->RegisterIntegerPref(prefs::kIdleThreshold, 15); - registry->RegisterBooleanPref(prefs::kBraveAdsWereDisabled, false); + registry->RegisterBooleanPref(prefs::kAdsWereDisabled, false); + registry->RegisterBooleanPref(prefs::kHasAdsP3AState, false); registry->RegisterBooleanPref(prefs::kShouldShowMyFirstAdNotification, true); diff --git a/components/brave_ads/common/pref_names.cc b/components/brave_ads/common/pref_names.cc index 4617e3d0748e..95c20abcfb17 100644 --- a/components/brave_ads/common/pref_names.cc +++ b/components/brave_ads/common/pref_names.cc @@ -13,7 +13,11 @@ namespace prefs { const char kEnabled[] = "brave.brave_ads.enabled"; // Stores whether ads were disabled at least once -const char kBraveAdsWereDisabled[] = "brave.brave_ads.were_disabled"; +const char kAdsWereDisabled[] = "brave.brave_ads.were_disabled"; + +// Indicates whether we have any initial state of the ads status metric, besides +// "No Wallet". +const char kHasAdsP3AState[] = "brave.brave_ads.has_p3a_state"; // Stores the maximum amount of ads per hour const char kAdsPerHour[] = "brave.brave_ads.ads_per_hour"; diff --git a/components/brave_ads/common/pref_names.h b/components/brave_ads/common/pref_names.h index 74d2e6acb3ce..87e65a2341b2 100644 --- a/components/brave_ads/common/pref_names.h +++ b/components/brave_ads/common/pref_names.h @@ -14,7 +14,8 @@ extern const char kEnabled[]; extern const char kAdsPerHour[]; extern const char kAdsPerDay[]; -extern const char kBraveAdsWereDisabled[]; +extern const char kAdsWereDisabled[]; +extern const char kHasAdsP3AState[]; extern const char kIdleThreshold[]; diff --git a/components/brave_rewards/browser/rewards_p3a.cc b/components/brave_rewards/browser/rewards_p3a.cc index 8501d465708a..f3d48111387e 100644 --- a/components/brave_rewards/browser/rewards_p3a.cc +++ b/components/brave_rewards/browser/rewards_p3a.cc @@ -88,17 +88,17 @@ void UpdateAdsP3AOnPreferenceChange(PrefService *prefs, if (pref == brave_ads::prefs::kEnabled) { if (prefs->GetBoolean(brave_ads::prefs::kEnabled)) { brave_rewards::RecordAdsState(AdsP3AState::kAdsEnabled); - prefs->SetBoolean(brave_ads::prefs::kBraveAdsWereDisabled, false); + prefs->SetBoolean(brave_ads::prefs::kAdsWereDisabled, false); } else { // Apparently, the pref was disabled. brave_rewards::RecordAdsState( rewards_enabled ? AdsP3AState::kAdsEnabledThenDisabledRewardsOn : AdsP3AState::kAdsEnabledThenDisabledRewardsOff); - prefs->SetBoolean(brave_ads::prefs::kBraveAdsWereDisabled, true); + prefs->SetBoolean(brave_ads::prefs::kAdsWereDisabled, true); } } else if (pref == brave_rewards::prefs::kBraveRewardsEnabled) { // Rewards pref was changed - if (prefs->GetBoolean(brave_ads::prefs::kBraveAdsWereDisabled)) { + if (prefs->GetBoolean(brave_ads::prefs::kAdsWereDisabled)) { brave_rewards::RecordAdsState( rewards_enabled ? AdsP3AState::kAdsEnabledThenDisabledRewardsOn : AdsP3AState::kAdsEnabledThenDisabledRewardsOff); @@ -107,6 +107,15 @@ void UpdateAdsP3AOnPreferenceChange(PrefService *prefs, } } +void MaybeRecordInitialAdsP3AState(PrefService* prefs) { + if (!prefs->GetBoolean(brave_ads::prefs::kHasAdsP3AState)) { + const bool ads_state = prefs->GetBoolean(brave_ads::prefs::kEnabled); + RecordAdsState(ads_state ? AdsP3AState::kAdsEnabled + : AdsP3AState::kAdsDisabled); + prefs->SetBoolean(brave_ads::prefs::kHasAdsP3AState, true); + } +} + void RecordNoWalletCreatedForAllMetrics() { RecordWalletBalanceP3A(false, 0); RecordAutoContributionsState(AutoContributionsP3AState::kNoWallet, 0); diff --git a/components/brave_rewards/browser/rewards_p3a.h b/components/brave_rewards/browser/rewards_p3a.h index ea9f08c73236..7b059334572e 100644 --- a/components/brave_rewards/browser/rewards_p3a.h +++ b/components/brave_rewards/browser/rewards_p3a.h @@ -50,6 +50,10 @@ void RecordAdsState(AdsP3AState state); void UpdateAdsP3AOnPreferenceChange(PrefService* prefs, const std::string& pref); +// Records an initial metric state ("disabled" or "enabled") if it was not done +// before. Intended to be called if the user has already created a wallet. +void MaybeRecordInitialAdsP3AState(PrefService* local_state); + void RecordNoWalletCreatedForAllMetrics(); double CalcWalletBalanceForP3A(base::flat_map wallets, diff --git a/components/brave_rewards/browser/rewards_service_impl.cc b/components/brave_rewards/browser/rewards_service_impl.cc index d3e813321ca4..b48330fea784 100644 --- a/components/brave_rewards/browser/rewards_service_impl.cc +++ b/components/brave_rewards/browser/rewards_service_impl.cc @@ -927,7 +927,7 @@ void RewardsServiceImpl::OnWalletInitialized(ledger::Result result) { RecordWalletBalanceP3A(true, 0); #if BUILDFLAG(BRAVE_ADS_ENABLED) const bool ads_enabled = - profile_->GetPrefs()->GetBoolean(brave_ads::prefs::kEnabled); + profile_->GetPrefs()->GetBoolean(brave_ads::prefs::kEnabled); RecordAdsState(ads_enabled ? AdsP3AState::kAdsEnabled : AdsP3AState::kAdsDisabled); #endif @@ -1067,6 +1067,7 @@ void RewardsServiceImpl::OnLedgerStateLoaded( } // Record stats. RecordBackendP3AStats(); + MaybeRecordInitialAdsP3AState(profile_->GetPrefs()); } if (state.first.empty()) { RecordNoWalletCreatedForAllMetrics(); diff --git a/components/p3a/brave_histogram_rewrite.cc b/components/p3a/brave_histogram_rewrite.cc index d074c1704713..f4373247a254 100644 --- a/components/p3a/brave_histogram_rewrite.cc +++ b/components/p3a/brave_histogram_rewrite.cc @@ -16,11 +16,13 @@ namespace { // Please keep this list sorted and synced with |DoHistogramBravezation|. constexpr const char* kBravezationHistograms[] = { "Bookmarks.Count.OnProfileLoad", + "DefaultBrowser.State", "Extensions.LoadExtension", "Tabs.TabCount", "Tabs.WindowCount", }; +// TODO(iefremov): Replace a bunch of 'if's with something more elegant. // Records the given sample using the proper Brave way. void DoHistogramBravezation(base::StringPiece histogram_name, base::HistogramBase::Sample sample) { @@ -40,6 +42,25 @@ void DoHistogramBravezation(base::StringPiece histogram_name, return; } + if ("DefaultBrowser.State" == histogram_name) { + int answer = 0; + switch (sample) { + case 0: // Not default. + case 1: // Default. + answer = sample; + break; + case 2: // Unknown, merging to "Not default". + answer = 0; + break; + case 3: // Other mode is default, merging to "Default". + answer = 1; + break; + default: + NOTREACHED(); + } + UMA_HISTOGRAM_BOOLEAN("Brave.Core.IsDefault", answer); + } + if ("Extensions.LoadExtension" == histogram_name) { int answer = 0; if (sample == 1) diff --git a/components/p3a/brave_p3a_log_store.cc b/components/p3a/brave_p3a_log_store.cc index a379b562c689..514464d6d120 100644 --- a/components/p3a/brave_p3a_log_store.cc +++ b/components/p3a/brave_p3a_log_store.cc @@ -34,10 +34,10 @@ void RecordP3A(uint64_t answers_count) { } // namespace -BraveP3ALogStore::BraveP3ALogStore(LogSerializer* serializer, +BraveP3ALogStore::BraveP3ALogStore(Delegate* delegate, PrefService* local_state) - : serializer_(serializer), local_state_(local_state) { - DCHECK(serializer_); + : delegate_(delegate), local_state_(local_state) { + DCHECK(delegate_); DCHECK(local_state); } @@ -123,7 +123,8 @@ void BraveP3ALogStore::StageNextLog() { DCHECK(!log_.find(staged_entry_key_)->second.sent); uint64_t staged_entry_value = log_[staged_entry_key_].value; - staged_log_ = serializer_->Serialize(staged_entry_key_, staged_entry_value); + staged_log_ = delegate_->Serialize(staged_entry_key_, staged_entry_value); + VLOG(2) << "BraveP3ALogStore::StageNextLog: staged " << staged_entry_key_; } @@ -161,10 +162,17 @@ void BraveP3ALogStore::LoadPersistedUnsentLogs() { DCHECK(log_.empty()); DCHECK(unsent_entries_.empty()); - const base::DictionaryValue* list = local_state_->GetDictionary(kPrefName); + DictionaryPrefUpdate update(local_state_, kPrefName); + base::DictionaryValue* list = update.Get(); for (auto dict_item : list->DictItems()) { LogEntry entry; - std::string name = dict_item.first; + const std::string name = dict_item.first; + // Check if the metric is obsolete. + if (!delegate_->IsActualMetric(name)) { + // Drop it from the local state. + list->RemoveKey(name); + continue; + } const base::Value& dict = dict_item.second; // Value. if (const base::Value* v = diff --git a/components/p3a/brave_p3a_log_store.h b/components/p3a/brave_p3a_log_store.h index 73c4636cad8e..e080a92d3c2c 100644 --- a/components/p3a/brave_p3a_log_store.h +++ b/components/p3a/brave_p3a_log_store.h @@ -25,15 +25,17 @@ namespace brave { // for now persisted entries never expire. class BraveP3ALogStore : public metrics::LogStore { public: - // Prepares a string representaion of an entry. - class LogSerializer { + class Delegate { public: + // Prepares a string representaion of an entry. virtual std::string Serialize(base::StringPiece histogram_name, uint64_t value) const = 0; - virtual ~LogSerializer() {} + // Returns false if the metric is obsolete and should be cleaned up. + virtual bool IsActualMetric(base::StringPiece histogram_name) const = 0; + virtual ~Delegate() {} }; - explicit BraveP3ALogStore(LogSerializer* serializer, + explicit BraveP3ALogStore(Delegate* delegate, PrefService* local_state); // TODO(iefremov): Make parent destructor virtual? @@ -78,7 +80,7 @@ class BraveP3ALogStore : public metrics::LogStore { base::Time sent_timestamp; // At the moment only for debugging purposes. }; - const LogSerializer* const serializer_ = nullptr; // Weak. + const Delegate* const delegate_ = nullptr; // Weak. PrefService* const local_state_ = nullptr; // TODO(iefremov): Try to replace with base::StringPiece? diff --git a/components/p3a/brave_p3a_service.cc b/components/p3a/brave_p3a_service.cc index 22689bb6eada..47dbac42672e 100644 --- a/components/p3a/brave_p3a_service.cc +++ b/components/p3a/brave_p3a_service.cc @@ -51,11 +51,13 @@ constexpr uint64_t kDefaultUploadIntervalSeconds = 60 * 60; // 1 hour. constexpr const char* kCollectedHistograms[] = { "Brave.P3A.SentAnswersCount", "Brave.Sync.Status", - "DefaultBrowser.State", + // Deprecated: + // "DefaultBrowser.State", "Brave.Importer.ImporterSource", "Brave.Shields.UsageStatus", // Do not gather detailed info regarding TOR usage for now. // "Brave.Core.LastTimeTorUsed", + "Brave.Core.IsDefault", "Brave.Core.TorEverUsed", "Brave.Core.LastTimeIncognitoUsed", "Brave.Core.NumberOfExtensions", @@ -205,6 +207,14 @@ std::string BraveP3AService::Serialize(base::StringPiece histogram_name, return message.SerializeAsString(); } +bool +BraveP3AService::IsActualMetric(base::StringPiece histogram_name) const { + static const base::NoDestructor> + metric_names {std::begin(kCollectedHistograms), + std::end(kCollectedHistograms)}; + return metric_names->contains(histogram_name); +} + void BraveP3AService::MaybeOverrideSettingsFromCommandLine() { base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess(); diff --git a/components/p3a/brave_p3a_service.h b/components/p3a/brave_p3a_service.h index 9545242fe2fe..14ba5baa9b40 100644 --- a/components/p3a/brave_p3a_service.h +++ b/components/p3a/brave_p3a_service.h @@ -30,7 +30,7 @@ class BraveP3AUploader; // on any thread. // TODO(iefremov): It should be possible to get rid of refcounted here. class BraveP3AService : public base::RefCountedThreadSafe, - public BraveP3ALogStore::LogSerializer { + public BraveP3ALogStore::Delegate { public: explicit BraveP3AService(PrefService* local_state); @@ -43,10 +43,12 @@ class BraveP3AService : public base::RefCountedThreadSafe, // Needs a living browser process to complete the initialization. void Init(); - // BraveP3ALogStore::LogSerializer + // BraveP3ALogStore::Delegate std::string Serialize(base::StringPiece histogram_name, uint64_t value) const override; + bool IsActualMetric(base::StringPiece histogram_name) const override; + private: friend class base::RefCountedThreadSafe; ~BraveP3AService() override;