Implement ad conversion attribution for Brave Ads #4253
Conversation
tmancey
requested review from
bridiver,
moritzhaller,
emerick,
SergeyZhukovsky,
samartnik and
gdregalo
|
@karenkliu @jenn-rhim Could you please confirm if the design below for "Opt-out of ad conversions" is ok, or if you would like changes:
|
| @@ -173,6 +174,17 @@ std::unique_ptr<BundleState> Bundle::GenerateFromCatalog( | |||
| } | |||
| } | |||
|
|
|||
| // Conversions | |||
| for (const auto& conversion : creative_set.conversions) { | |||
| AdConversionInfo ad_conversion_info; | |||
There was a problem hiding this comment.
Can we initialize ad_conversion_info with copy constructor here?
There was a problem hiding this comment.
Fixed as refactored code to use a single data structure
| GetDB().RollbackTransaction(); | ||
| return false; | ||
| } | ||
|
|
||
| auto categories = bundle_state.categories; | ||
| for (std::map<std::string, std::vector<ads::AdInfo>>::iterator it = | ||
| categories.begin(); it != categories.end(); ++it) { | ||
| for (auto it = categories.begin(); it != categories.end(); ++it) { |
There was a problem hiding this comment.
end is called on every iteration if not optimized by compiler. We can read this value before for-loop and use it in condition evaluation statement. range-based for loops are even more readable.
|
|
||
| AdConversionInfo::~AdConversionInfo() = default; | ||
|
|
||
| const std::string AdConversionInfo::ToJson() const { |
There was a problem hiding this comment.
as far as std::string is a component of a standard library we can assume it is a built-in type. You should avoid confusing the client programmer and leave off the const when returning a built-in type by value. That technique works good with user-defined types otherwise it creates unneeded confusion.
tmancey
force-pushed
the
issues/6536
branch
11 times, most recently
from
eb41207 to
ec08a77
Compare
tmancey
added
CI/skip-android
|
All passed other than macOS, restarting macOS |
tmancey
removed
CI/skip-android
Fixes brave/brave-browser#6536
Requires: #3980
Submitter Checklist:
npm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
See brave/brave-browser#6536
Reviewer Checklist:
After-merge Checklist:
changes has landed on.