Enhance and refactor the Brave Ads component updater #8502
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
moritzhaller
force-pushed
the
issues/12856
branch
2 times, most recently
from
3051742 to
770d45d
Compare
moritzhaller
changed the title
moritzhaller
force-pushed
the
issues/12856
branch
from
770d45d to
b91552a
Compare
moritzhaller
force-pushed
the
issues/12856
branch
from
b91552a to
f81643d
Compare
moritzhaller
requested review from
fmarier,
kylehickinson and
tmancey
as code owners
tmancey
reviewed
Apr 13, 2021
| @@ -1903,17 +1903,19 @@ void AdsServiceImpl::Save(const std::string& name, | |||
| std::move(callback))); | |||
| } | |||
|
|
|||
| void AdsServiceImpl::LoadUserModelForId(const std::string& id, | |||
| ads::LoadCallback callback) { | |||
| // TODO(Moritz Haller): Naming clash with |LoadResource| | |||
tmancey
reviewed
Apr 13, 2021
| const base::Optional<ComponentInfo> component = | ||
| GetComponentInfo(country_code); | ||
| if (!component) { | ||
| VLOG(1) << "no ads resources support for " << country_code; |
There was a problem hiding this comment.
nit: Ads resource not supported for ...
tmancey
reviewed
Apr 13, 2021
| const base::Optional<ComponentInfo> component = | ||
| GetComponentInfo(language_code); | ||
| if (!component) { | ||
| VLOG(1) << "no ads resources support for " << language_code; |
There was a problem hiding this comment.
nit: Ads resource not supported for ...
tmancey
reviewed
Apr 13, 2021
|
|
||
| const base::Optional<int> version = manifest->FindIntPath(kSchemaVersionPath); | ||
| if (!version) { | ||
| VLOG(1) << "resource schema version is missing"; |
There was a problem hiding this comment.
nit: Capitalize R in resource
tmancey
reviewed
Apr 13, 2021
| } | ||
|
|
||
| if (*version != kCurrentSchemaVersion) { | ||
| VLOG(1) << "resource schema version mismatch"; |
There was a problem hiding this comment.
nit: Capitalize R in resource
tmancey
reviewed
Apr 13, 2021
|
|
||
| const std::string* id = resource_value.FindStringPath(kResourceIdPath); | ||
| if (!id) { | ||
| VLOG(1) << *id << " resource id is missing"; |
There was a problem hiding this comment.
nit: instead of *id you id.value()
tmancey
reviewed
Apr 13, 2021
| @@ -0,0 +1,57 @@ | |||
| /* Copyright (c) 2020 The Brave Authors. All rights reserved. | |||
There was a problem hiding this comment.
Copyright for new files should be 2021
tmancey
reviewed
Apr 13, 2021
| virtual void OnUserModelUpdated(const std::string& id) = 0; | ||
| // Should be called when a resource component has been updated by | ||
| // |brave_ads::ResourceComponent| | ||
| virtual void OnResourceComponentUpdate(const std::string& id) = 0; |
There was a problem hiding this comment.
OnResourceComponentUpdated
tmancey
reviewed
Apr 13, 2021
| anti_targeting_resource_->Load(); | ||
| } else { | ||
| BLOG(0, "Unknown " << id << " user model"); | ||
| BLOG(0, "Unknown resource " << id); |
There was a problem hiding this comment.
nit: Unknown resource for ...
tmancey
reviewed
Apr 13, 2021
| @@ -33,7 +33,9 @@ class NativeAdsClient : public ads::AdsClient { | |||
| void UrlRequest(ads::UrlRequestPtr url_request, ads::UrlRequestCallback callback) override; | |||
| void Save(const std::string & name, const std::string & value, ads::ResultCallback callback) override; | |||
| void Load(const std::string & name, ads::LoadCallback callback) override; | |||
| void LoadUserModelForId(const std::string & id, ads::LoadCallback callback) override; | |||
| // TODO(Moritz Haller): Add version | |||
tmancey
reviewed
Apr 13, 2021
Comment on lines
22
to
23
| const int kCurrentVersion = 1; | ||
| const int kVersion = 1; |
There was a problem hiding this comment.
What is the difference between kCurrentVersion and kVersion please. If we could rename kVersion to be more verbose that would be great
There was a problem hiding this comment.
as discussed yesterday - removed redundant version and added as feature parameter
fmarier
approved these changes
Apr 13, 2021
moritzhaller
force-pushed
the
issues/12856
branch
5 times, most recently
from
bed7aa3 to
996fa2f
Compare
kylehickinson
approved these changes
Apr 15, 2021
moritzhaller
force-pushed
the
issues/12856
branch
6 times, most recently
from
3f38111 to
a93442a
Compare
Enable a/b testing for different versions of a resource, e.g. text classification. Change name from UserModel to AdsResource and move code into the brave ads component. Finally removing the bat-native-usermodel dependency from ads.
moritzhaller
force-pushed
the
issues/12856
branch
from
a93442a to
f62e5af
Compare
|
CI fails with unrelated issues on macos and linux |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves brave/brave-browser#12856
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
Run with staging endpoint for component updater
--use-dev-goupdater-urland confirm all resources load for Beta and Nightly, e.g. for localeen-GBthe following output is expectedBeta for en-GB:
Nightly for en-GB:
Then verify that the models work by spot-checking one purchase intent site/keyword and one text classification on each Beta and Nightly, e.g.:
[text_classification_processor.cc(62)] Classified text with the top segment as technology & computing-software[purchase_intent_processor.cc(99)] Extracted purchase intent signal from visited URLAnalogous for iOS, verify that resources load for old and new versions: