-
Notifications
You must be signed in to change notification settings - Fork 868
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
brave_stats: Create preference to disable ping #9229
Conversation
f6d439c
to
9ad9133
Compare
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
899d98c
to
b72095a
Compare
UI lgtm but i didn't see any usage pings with or without the setting enabled. what's the expected IP of the ping? |
I think since you are on a dev build it's not set. You can pass the cli flag |
i tried both and still no pings. npm start runs the build with |
browser/resources/settings/brave_privacy_page/brave_personalization_options.js
Show resolved
Hide resolved
@diracdeltas the browser tests pass, which wait for the ping to fire, and i am able to see it log in the URL loader handler. You should see if you can see the ping without this patchset. If you run into any issues can follow up on Slack. |
b72095a
to
f0bbfc2
Compare
f0bbfc2
to
9780fc0
Compare
@karenkliu the screenshots you asked for |
@keur I'm not seeing the updated text for crash reporting, the usage ping, and P3A? Also it should be ordered P3A, usage ping, then crash reporting: |
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: #9229 (comment)
cc @karenkliu |
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.
Perfect! 💯
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.
++
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.
chromium_src
++
@@ -308,8 +308,8 @@ By installing this extension, you are agreeing to the Google Widevine Terms of U | |||
<translation id="1872784258300710671">Brave uses completely private product analytics to estimate the overall usage of certain features</translation> |
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.
.xtb files are generated automatically when we pull translations from Transifex. There's no need to edit them.
5348e83
to
7a38aa6
Compare
* Usage ping text updated * P3A text updated * Crash reporting text updated Reorder these settings
7a38aa6
to
992d0af
Compare
Test failure in ads. Unrelated |
Resolves brave/brave-browser#16583
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
,npm run lint
,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
See issue