-
Notifications
You must be signed in to change notification settings - Fork 891
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ads] Optimize database queries serialization/deserialization overhead #26866
Conversation
Discussed using run loops rather than test and background handlers. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM++ (please see comments)
components/brave_ads/core/internal/common/test/ads_client_notifier_waiter.cc
Outdated
Show resolved
Hide resolved
components/brave_ads/core/internal/common/test/ads_client_notifier_waiter.h
Outdated
Show resolved
Hide resolved
components/brave_ads/core/internal/user_engagement/conversions/conversions_test_base.cc
Outdated
Show resolved
Hide resolved
components/brave_ads/core/internal/user_engagement/conversions/conversions_unittest.cc
Outdated
Show resolved
Hide resolved
components/brave_ads/core/internal/user_engagement/conversions/conversions_unittest.cc
Outdated
Show resolved
Hide resolved
[puLL-Merge] - brave/brave-core@26866 DescriptionThis PR makes significant changes to the Brave Ads core implementation, primarily focusing on database management and initialization. The main changes include:
The motivation for these changes appears to be improving the encapsulation of database operations and simplifying the public API of the Brave Ads core. ChangesChanges
sequenceDiagram
participant Ads
participant GlobalState
participant DatabaseManager
participant Database
Ads->>GlobalState: Create
GlobalState->>DatabaseManager: Create(database_path)
DatabaseManager->>Database: Create
Ads->>GlobalState: GetDatabaseManager()
GlobalState->>DatabaseManager: RunDBTransaction()
DatabaseManager->>Database: RunDBTransaction()
Database-->>DatabaseManager: Result
DatabaseManager-->>GlobalState: Result
GlobalState-->>Ads: Result
Possible Issues
Security HotspotsNo significant security issues are apparent in this change. The modifications to database management might indirectly improve security by better encapsulating database operations. |
Released in v1.75.91 |
Resolves brave/brave-browser#42598
Resolves brave/brave-browser#41632
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Make sure that Brave Ads core functionality works as expected: