-
Notifications
You must be signed in to change notification settings - Fork 862
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
Remove unused lastSearchTime, searchActivity, searchUrl, lastShopTime, shopActivity and shopUrl from locally persisted Brave Ads state #5159
Conversation
@@ -700,13 +697,11 @@ void AdsImpl::OnPageLoaded( | |||
return; | |||
} | |||
|
|||
if (TestSearchState(url)) { | |||
if (SearchProviders::IsSearchEngine(url)) { | |||
BLOG(INFO) << "Site visited " << url << ", URL is a search engine"; |
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.
should the URL even be logged? logs can get persisted to disk unintentionally
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.
yea, this seems like it should be a VLOG entry anyway if we kept it at all. INFO
doesn't seem like the right logging level even without the persistence issue
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.
left a comment but overall lgtm
@@ -21,12 +20,7 @@ | |||
"adUUID": "", | |||
"locale": "en", | |||
"lastUserActivity": 0, |
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.
can we clear this from existing state or does that happen somewhere that I missed?
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.
approved with discussed changes to remove existing state values and logging of urls
…, shopActivity and shopUrl from locally persisted Brave Ads state
Resolves brave/brave-browser#9022
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Confirm lastSearchTime, searchActivity, searchUrl, lastShopTime, shopActivity and shopUrl are not persisted in
Default/ads_service/client.json
Reviewer Checklist:
After-merge Checklist:
changes has landed on.