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.
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
Initial implementation of P3A (Privacy-Preserving Product Analytics) #3242
Initial implementation of P3A (Privacy-Preserving Product Analytics) #3242
Changes from all commits
6982b45
8b7dbea
017e463
86d933a
81770af
e995099
b6ee44b
15a7505
a2cf68a
9a82351
1d8126d
01f3598
a56222f
b07100f
7127bb4
575f652
c1c8654
f780b64
462d9c5
b90ddb9
47123c1
d0e63f4
91ca988
88339fc
b8848e6
4ced136
bb15b1b
2e573bf
72ceefc
6088598
2f11a62
19fcaac
1ec80b1
5474b74
526aec3
e20f494
da2a2bb
2ef207e
b964eef
23bb018
d164e02
30438b8
d5f297f
b5c9ff9
5fc97f8
c6077e4
ed109c2
f91e896
242e1ea
f8d6f99
56dec6c
c288726
dc04788
db85f06
6850f1b
4af229a
f485f60
cb79e1e
2f114cc
faf20d2
2e6802d
a9e0160
8657cdd
7beddf1
0a06899
54bedf0
26ec2c1
795ef88
6637ae8
05a27da
5505737
d428fa1
5c5fe9f
24e28f6
66dd2fd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
do we need a separate call for this? Can it just happen as part of
new
?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.
Making this a part of the constructor can make testing problematic
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 move https://github.com/brave/brave-core/pull/3242/files#diff-ce86d8f829640e5e88ce2b71c4e22b01R63 here then and call
InitCallbacks
insideInit
? It just seems weird that we're callingInitCallbacks()
here and thenInit()
in PreMainMessageLoopRun?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.
I see the comments, but
PreMainMessageLoopRun
is the first time this gets called, right? So it's all happening in the same place anyway?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.
not exactly, this code is eagerly called in
BraveBrowserProcessImpl
. So I think we can't do much about this separation.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.
here? https://github.com/brave/brave-core/pull/3242/files#diff-f4fa6d8248b71f0edf636150386e89ceR88
why are we eagerly calling it there? I think either way we need some comments to help explain
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.
here, in constructor https://github.com/brave/brave-core/pull/3242/files#diff-321b7f43759aaadc37d4925c6163cc1bR88
already has a comment
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.
why don't we just have a static override for testing? I just saw this again recently and brought me back to this conversation. This doesn't seem like a great way to handle the testing issue imo
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.
the comment now says
Can't call it in constructor because of refcounted peculiarities.
- but why is this refcounted in the first place? The lifetime is already tied to BraveBrowserProcessImpl so it will outlive everythingThere 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.
well there is a TODO in p3a_service.h saying that we should get rid of recounting :D
I don't remember the exact reason, probably it is due tothe fact that histogram events can happen really early and really late. But maybe just switching it to unique_ptr + Unretained will be fine as is, actualy