Feature/ads diagnostics ads internals #7004
Conversation
yachtcaptain23
force-pushed
the
feature/ads_diagnostics_ads-internals
branch
from
6d30f95
to
2fa40d7
Compare
| return; | ||
| } | ||
|
|
||
| base::DictionaryValue info_dict; |
There was a problem hiding this comment.
This should be serialized from the ads lib using base::JSONReader and passed via std::string as then we do not need to update Rewards internals for future changes
| @@ -51,6 +51,12 @@ void AdsService::RegisterProfilePrefs( | |||
| registry->RegisterBooleanPref( | |||
| ads::prefs::kShouldAllowConversionTracking, true); | |||
|
|
|||
| // Diagnostics | |||
| registry->RegisterIntegerPref(ads::prefs::kEligibleAdsCount, -1); | |||
There was a problem hiding this comment.
Why are we persisting some values in prefs as all of these values can be returned in the serialized JSON from the ads library?
| onClick={this.props.onGet} | ||
| /> | ||
| </ButtonWrapper> | ||
| <div> |
There was a problem hiding this comment.
We should be dynamically building the following based upon the JSON
| @@ -459,6 +459,7 @@ | |||
| <message name="IDS_BRAVE_REWARDS_INTERNALS_EVENT_LOG_TIME" desc="when event was logged">Logged at</message> | |||
| <message name="IDS_BRAVE_REWARDS_INTERNALS_MAIN_DISCLAIMER" desc="">WARNING: Data on this page may be sensitive. Treat them as you would your wallet private keys. Be careful who you share them with.</message> | |||
| <message name="IDS_BRAVE_REWARDS_INTERNALS_CONTRIBUTION_TYPE" desc="Contribution type">Type:</message> | |||
| <message name="IDS_BRAVE_REWARDS_INTERNALS_TAB_GENERAL_ADS_INFO" desc="">General Ads Info</message> | |||
There was a problem hiding this comment.
Can we please change General Ads Info to Ad Diagnostics
| using InternalsInfoCallback = | ||
| std::function<void(InternalsInfoPtr)>; |
| #include "brave/vendor/bat-native-ads/include/bat/ads/public/interfaces/ads.mojom.h" | ||
| #include "brave/vendor/bat-native-ads/include/bat/ads/public/interfaces/ads_database.mojom.h" |
There was a problem hiding this comment.
Why did we add brave/vendor/bat-native-ads-include/ as this should not be needed?
| using InternalsInfo = mojom::BraveAdsInternalsInfo; | ||
| using InternalsInfoPtr = mojom::BraveAdsInternalsInfoPtr; | ||
|
|
There was a problem hiding this comment.
Please move to after BuildChannel/BuildChannelPtr
| @@ -10,6 +10,16 @@ enum BraveAdsEnvironment { | |||
| DEVELOPMENT | |||
| }; | |||
|
|
|||
| struct BraveAdsInternalsInfo { | |||
There was a problem hiding this comment.
As above, as discussed we should be serializing as JSON in the library and rendering the JSON in the UI as this will reduce changes within the rewards code for future additions
| @@ -174,7 +174,7 @@ void AdServing::MaybeServeAdForParentChildCategories( | |||
| const CreativeAdNotificationList eligible_ads = | |||
| eligible_ad_notifications.Get(ads, | |||
| last_delivered_creative_ad_, ad_events); | |||
|
|
|||
| ads_client_->SetIntegerPref(prefs::kEligibleAdsCount, eligible_ads.size()); | |||
There was a problem hiding this comment.
We should not be storing diagnostics under a pref, we should have a diagnostics object, which has a data structure and this data structure is serialized, i.e. AdDiagnostics()->Get()->SetEligibleAdCount(...). Then AdsImpl::GetInternalsInfo can get the serialized JSON from this object
| InternalsInfo info_ = *info; | ||
| info->catalog_id = AdsClientHelper::Get()->GetStringPref(prefs::kCatalogId); | ||
| info->catalog_last_updated = AdsClientHelper::Get()->GetInt64Pref(prefs::kCatalogLastUpdated); | ||
| info->enabled = IsInitialized(); | ||
| info->eligible_ads_count = 0; | ||
| info->flagged_ads = ""; | ||
| info->last_filtered_ads = ""; | ||
| callback(std::move(info)); |
There was a problem hiding this comment.
AdsImpl should not have any business logic other than calling out to fetch the required information, please move to an AdDiagnostics class, see UserActivity for an example
| @@ -472,7 +490,7 @@ void AdsImpl::InitializeStep6( | |||
| is_initialized_ = true; | |||
|
|
|||
| BLOG(1, "Successfully initialized ads"); | |||
|
|
|||
| AdsClientHelper::Get()->SetIntegerPref(prefs::kEligibleAdsCount, -1); | |||
There was a problem hiding this comment.
See above concerning state
| @@ -10,6 +10,7 @@ | |||
| #include <string> | |||
| #include <vector> | |||
|
|
|||
| #include "base/strings/stringprintf.h" | |||
Resolves brave/brave-browser#10964
Submitter Checklist:
npm run lint,npm run gn_check)git rebase master(if needed).git rebase -ito squash commits (if needed).QA/YesorQA/No) to the associated issuerelease-notes/includeorrelease-notes/exclude) to the associated issueTest Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.