Implement support for Global Privacy Control signal [chromium/src : main]
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: dch...@chromium.org, toyo...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): dch...@chromium.org, toyo...@chromium.org
Note: IPC gwsq added no new reviewers; existing reviewers satisfied requirements!
Reviewer source(s):
tr...@chromium.org is from context(googleclient/chrome/chromium_gwsq/components/sync/config.gwsq)
toyo...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Daniel Bratell
- Daniel Cheng
- Darryl James
- Marc Treib
- Marijn Kruisselbrink
- Takashi Toyoshima
- Yaron Friedman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
Removing myself since there is overlapping OWNERs coverage. Add me back if my input is needed thanks!
Related details
- Daniel Bratell
- Daniel Cheng
- Maciej Czarnecki
- Marc Treib
- Marijn Kruisselbrink
- Takashi Toyoshima
- Yaron Friedman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marc Treib (Gerrit)
Marc Treib added 1 comment![]( "Open in Gerrit")
Removing myself since there is overlapping OWNERs coverage. Add me back if my input is needed thanks!
Adding to this:
When adding multiple OWNERs, please specify what you expect everyone to review!
And there is usually no need to add overlapping OWNERs coverage; one OWNER per file is enough.
I assume I'm here for chrome_syncable_prefs_database.cc which LG, but I'll hold off on approving for now since I technically own a lot of other files here, but am not the best reviewer for them.
Related details
- Daniel Bratell
- Daniel Cheng
- Maciej Czarnecki
Daniel Bratell (Gerrit)
Daniel Bratell added 1 comment![]( "Open in Gerrit")
I second what Marc said. Looking through the list, I think there are more suitable reviewers assigned for each file so I'll remove myself, but do re-add me if my input is needed.
Related details
- Daniel Cheng
- Maciej Czarnecki
- Marijn Kruisselbrink
- Takashi Toyoshima
- Yaron Friedman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 1 comment![]( "Open in Gerrit")
Marc TreibRemoving myself since there is overlapping OWNERs coverage. Add me back if my input is needed thanks!
Adding to this:
When adding multiple OWNERs, please specify what you expect everyone to review!
And there is usually no need to add overlapping OWNERs coverage; one OWNER per file is enough.I assume I'm here for chrome_syncable_prefs_database.cc which LG, but I'll hold off on approving for now since I technically own a lot of other files here, but am not the best reviewer for them.
Thank you, when I was adding reviewers I was thinking that I've skipped all overlapping owners. I need to look closer into gerrit UI to spot redundant reviewers ;)
Related details
- Daniel Cheng
- Marc Treib
- Takashi Toyoshima
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 1 comment![]( "Open in Gerrit")
Just a note that I will resolve the conflicts once the rest of the files have been reviewed, as the enums keep changing.
Related details
- Jeremy Roman
- Kentaro Hara
- Takashi Toyoshima
- Yaron Friedman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jeremy Roman (Gerrit)
Jeremy Roman added 1 comment![]( "Open in Gerrit")
+arichiv
Suggest we get someone who understands this space well in Chromium to take point on reviewing this, to make sure this feature makes sense in Chromium and this is the overall way we'd like to implement it if so. From asking around it seems that's probably Ari.
I'm happy to review particular files that they don't have ownership over for more narrow style/structure/correctness, once Ari approves. (At that time, if you could please specifically say which reviewers you expect to review which files, that'd be helpful.)
Related details
- Ari Chivukula
- Kentaro Hara
- Maciej Czarnecki
- Takashi Toyoshima
- Yaron Friedman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 6 comments![]( "Open in Gerrit")
thanks for starting this work!
Bug: 1171730This should be 40745270, the new ID for the same task
inline constexpr char kEnableGlobalPrivacyControl[] =I'd hold off on all prefs related work for a future CL. It does make sense to have the hooks in a common area if other blink-derived browsers want it, but I think the first CL could just include a base::feature for automated tests and a chrome://flags entry for manual tests
String LocalFrameClientImpl::GlobalPrivacyControlValue() {nit: I'd bias towards returning a bool here, and only turning it into a "1" or "" at the time a header is written.
Otherwise you end up turning a bool into a string here, then back into a bool elsewhere.
// Global Privacy Control APII'm excited to see this! I'd suggest adding a chrome-flag (controlled via a manual feature) instead of a runtime flag (generating a dependent feature) for now so that it's easy to test and less complex to reason about (as the browser process needs access): https://chromium.googlesource.com/chromium/src/+/HEAD/docs/how_to_add_your_feature_flag.md
I don't really think a runtime-feature makes sense even when you implement the prefs control
status: "experimental",This enables it for some tests automatically, which I don't think we want.
Related details
- Kentaro Hara
- Maciej Czarnecki
- Takashi Toyoshima
- Yaron Friedman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
if (base::FeatureList::IsEnabled(blink::features::kGlobalPrivacyControl) &&For clarity on the flows I'm proposing:
*This CL:*
GPC header enabled if and only if blink::features::kGlobalPrivacyControlForce is enabled via chrome://flags entry.
This means kGlobalPrivacyControlForce is a single global enable/disable control.
*Next CL:*
GPC header enabled if either of the following:
(1) blink::features::kGlobalPrivacyControlForce is enabled via chrome://flags entry.
(2) prefs::kEnableGlobalPrivacyControl is enabled and blink::features::kGlobalPrivacyControlPrefs is enabled
This means kGlobalPrivacyControlForce stays a single global enable/disable control.
It also adds kGlobalPrivacyControlPrefs, which controls whether a preference is respected additionally.
Then in the future, kGlobalPrivacyControlPrefs would be what gated UX related to the GPC preference for rollout purposes.
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 1 comment![]( "Open in Gerrit")
+arichiv
Suggest we get someone who understands this space well in Chromium to take point on reviewing this, to make sure this feature makes sense in Chromium and this is the overall way we'd like to implement it if so. From asking around it seems that's probably Ari.
I'm happy to review particular files that they don't have ownership over for more narrow style/structure/correctness, once Ari approves. (At that time, if you could please specifically say which reviewers you expect to review which files, that'd be helpful.)
Thank you for suggesting Ari! This is my first CL spanning so many modules, so I apologise for managing the review process suboptimally. I have already received some comments to work on, so there is no pressure on review.
I'm not sure how to split the files among the reviewers, as I don't know Yours areas of expertise (I relied on Gerrit suggestions).
Here is my suggestion for who can review parts that have overlapping owners:
@toyo...@chromium.org
`third_party/blink/common/loader/loader_constants.cc`
`third_party/blink/public/common/loader/loader_constants.h`
`third_party/blink/public/common/renderer_preferences/renderer_preferences.h`
@har...@chromium.org
`third_party/blink/renderer/core/frame/local_frame_client.h`
`third_party/blink/renderer/core/frame/local_frame_client_impl.h`
`third_party/blink/renderer/core/frame/local_frame_client_impl.cc`
`third_party/blink/renderer/core/loader/empty_clients.h`
@jbr...@chromium.org
`third_party/blink/renderer/modules/global_privacy_control/*`
`third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc`
@m...@chromium.org
`third_party/blink/renderer/modules/service_worker/web_service_worker_fetch_context_impl.cc`
Related details
- Jeremy Roman
- Kentaro Hara
- Takashi Toyoshima
- Yaron Friedman
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 2 comments![]( "Open in Gerrit")
Sorry for yet another change to the reviewers' list! For now, I will focus on Ari's review to prepare the CL for broader analysis.
This should be 40745270, the new ID for the same task
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 5 comments![]( "Open in Gerrit")
I'd hold off on all prefs related work for a future CL. It does make sense to have the hooks in a common area if other blink-derived browsers want it, but I think the first CL could just include a base::feature for automated tests and a chrome://flags entry for manual tests
Done
if (base::FeatureList::IsEnabled(blink::features::kGlobalPrivacyControl) &&For clarity on the flows I'm proposing:
*This CL:*
GPC header enabled if and only if blink::features::kGlobalPrivacyControlForce is enabled via chrome://flags entry.
This means kGlobalPrivacyControlForce is a single global enable/disable control.
*Next CL:*
GPC header enabled if either of the following:
(1) blink::features::kGlobalPrivacyControlForce is enabled via chrome://flags entry.
(2) prefs::kEnableGlobalPrivacyControl is enabled and blink::features::kGlobalPrivacyControlPrefs is enabledThis means kGlobalPrivacyControlForce stays a single global enable/disable control.
It also adds kGlobalPrivacyControlPrefs, which controls whether a preference is respected additionally.Then in the future, kGlobalPrivacyControlPrefs would be what gated UX related to the GPC preference for rollout purposes.
Done
String LocalFrameClientImpl::GlobalPrivacyControlValue() {nit: I'd bias towards returning a bool here, and only turning it into a "1" or "" at the time a header is written.
Otherwise you end up turning a bool into a string here, then back into a bool elsewhere.
Done
I'm excited to see this! I'd suggest adding a chrome-flag (controlled via a manual feature) instead of a runtime flag (generating a dependent feature) for now so that it's easy to test and less complex to reason about (as the browser process needs access): https://chromium.googlesource.com/chromium/src/+/HEAD/docs/how_to_add_your_feature_flag.md
I don't really think a runtime-feature makes sense even when you implement the prefs control
Done
status: "experimental",Maciej CzarneckiThis enables it for some tests automatically, which I don't think we want.
Done
Related details
- Ari Chivukula
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 1 comment![]( "Open in Gerrit")
"expiry_milestone": 150The milestone number is arbitrary. It should be sufficient for testing purposes.
Related details
- Ari Chivukula
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 13 comments![]( "Open in Gerrit")
nice! This is in the right direction (aside from one error I made in the last set of feedback).
class ChromeGlobalPrivacyControlDisabledTestinstead of two test fixture classes, I'd use parameterization: https://source.chromium.org/chromium/chromium/src/+/main:third_party/googletest/src/googletest/include/gtest/gtest.h;drc=5e49278007a2cfa07feb558706818b54fe4bfaae;l=1657
we need to test the JS API for workers too right?
"Global Privacy Control signal";maybe 'force' instead of 'signal'?
if (base::FeatureList::IsEnabled(This should probably return a value from the same function as in third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc for now, though in future (per the GPC spec) it will need to cache the value at the time of frame navigation (might be good to make a TODO here about that).
blink::features::kGlobalPrivacyControlForce)) {nit: see comment on third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc
BASE_FEATURE(kGlobalPrivacyControlForce, base::FEATURE_DISABLED_BY_DEFAULT);nit: see comment on third_party/blink/renderer/modules/global_privacy_control/navigator_global_privacy_control.idl for why manual declaration can be removed.
return base::FeatureList::IsEnabled(This should probably return a value from the same function as in third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc for now, though in future (per the GPC spec) it will need to cache the value at the time of frame navigation (might be good to make a TODO here about that).
ImplementedAs=NavigatorGlobalPrivacyControlApologies, I forgot that the IDL change does require a runtime feature gate (as you had before), I'd define it as:
```
{
name: "GlobalPrivacyControl",
implied_by: "GlobalPrivacyControlForce"
base_feature: "none"
},
{
name: "GlobalPrivacyControlForce"
},
```
(0) The gate should be on `GlobalPrivacyControl`
(1) The lack of a `status` field on either should default them off in all contexts
(2) The `none` feature on `GlobalPrivacyControl` prevents generation of an unneeded feature
(3) The `implied_by` on `GlobalPrivacyControl` allows it to expand to a second feature in future when you need one
(4) The `GlobalPrivacyControlForce` feature will auto-generate a public feature that can be used instead of the one you manually declared
blink::features::kGlobalPrivacyControlForce)) {nit: see comment on third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc
if (base::FeatureList::IsEnabled(nit: since you have the same logic here and in third_party/blink/renderer/modules/service_worker/web_service_worker_fetch_context_impl.cc and content/renderer/render_frame_impl.cc, and you know in future CLs it will likely expand to multiple checks (aside from just one feature), it might make sense to factor the logic for this gate into a helper function even though now it would just return `base::FeatureList::IsEnabled(blink::features::kGlobalPrivacyControlForce)`. This ensures the other callsite isn't forgotten as the code evolves.
Related details
- Maciej Czarnecki
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 3 comments![]( "Open in Gerrit")
nit: it would be good to test web-feature measurement of the JS API
[MeasureAs=NavigatorGlobalPrivacyControl] readonly attribute boolean globalPrivacyControl;nit: I think you can just put 'Measure' and it will auto-generate a web-feature for you
"1");nit: it might be good to add some UMA logging for what contexts the header is sent in - https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 1 comment![]( "Open in Gerrit")
[MeasureAs=NavigatorGlobalPrivacyControl] readonly attribute boolean globalPrivacyControl;nit: I think you can just put 'Measure' and it will auto-generate a web-feature for you
I've used Measure and it generates separate IDs for Navigator and WorkerNavigator:
```
kV8WorkerNavigator_GlobalPrivacyControl_AttributeGetter = 5790,
kV8Navigator_GlobalPrivacyControl_AttributeGetter = 5791,
```
Do you think it's better to have these names generated or keep `MeasureAs` to have common id?
Related details
- Ari Chivukula
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
[MeasureAs=NavigatorGlobalPrivacyControl] readonly attribute boolean globalPrivacyControl;Maciej Czarneckinit: I think you can just put 'Measure' and it will auto-generate a web-feature for you
I've used Measure and it generates separate IDs for Navigator and WorkerNavigator:
```
kV8WorkerNavigator_GlobalPrivacyControl_AttributeGetter = 5790,
kV8Navigator_GlobalPrivacyControl_AttributeGetter = 5791,
```
Do you think it's better to have these names generated or keep `MeasureAs` to have common id?
I like the idea of independent IDs there since they are such different contexts
Related details
- Maciej Czarnecki
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 15 comments![]( "Open in Gerrit")
I've made the necessary changes addressing most of the comments. I still need to add UMA logging, but I'd like to share the current version with you so you can take a look.
nit: it would be good to add a link to https://www.w3.org/TR/gpc/ here
Done
instead of two test fixture classes, I'd use parameterization: https://source.chromium.org/chromium/chromium/src/+/main:third_party/googletest/src/googletest/include/gtest/gtest.h;drc=5e49278007a2cfa07feb558706818b54fe4bfaae;l=1657
Done
nit: it would be good to test web-feature measurement of the JS API
Done
might be good to test sub-frames too
Done
we need to test the JS API for workers too right?
I added few tests for different contexts. Please look into them carefully as this is my first contact with Blink and I had to use other tests as a reference.
Marked as resolved.
maybe 'force' instead of 'signal'?
Done
This should probably return a value from the same function as in third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc for now, though in future (per the GPC spec) it will need to cache the value at the time of frame navigation (might be good to make a TODO here about that).
Done
nit: see comment on third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc
Done
BASE_FEATURE(kGlobalPrivacyControlForce, base::FEATURE_DISABLED_BY_DEFAULT);nit: see comment on third_party/blink/renderer/modules/global_privacy_control/navigator_global_privacy_control.idl for why manual declaration can be removed.
Done
This should probably return a value from the same function as in third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc for now, though in future (per the GPC spec) it will need to cache the value at the time of frame navigation (might be good to make a TODO here about that).
Done
Apologies, I forgot that the IDL change does require a runtime feature gate (as you had before), I'd define it as:
```
{
name: "GlobalPrivacyControl",
implied_by: "GlobalPrivacyControlForce"
base_feature: "none"
},
{
name: "GlobalPrivacyControlForce"
},
```
(0) The gate should be on `GlobalPrivacyControl`
(1) The lack of a `status` field on either should default them off in all contexts
(2) The `none` feature on `GlobalPrivacyControl` prevents generation of an unneeded feature
(3) The `implied_by` on `GlobalPrivacyControl` allows it to expand to a second feature in future when you need one
(4) The `GlobalPrivacyControlForce` feature will auto-generate a public feature that can be used instead of the one you manually declared
Done
[MeasureAs=NavigatorGlobalPrivacyControl] readonly attribute boolean globalPrivacyControl;Maciej Czarneckinit: I think you can just put 'Measure' and it will auto-generate a web-feature for you
Ari ChivukulaI've used Measure and it generates separate IDs for Navigator and WorkerNavigator:
```
kV8WorkerNavigator_GlobalPrivacyControl_AttributeGetter = 5790,
kV8Navigator_GlobalPrivacyControl_AttributeGetter = 5791,
```
Do you think it's better to have these names generated or keep `MeasureAs` to have common id?
I like the idea of independent IDs there since they are such different contexts
Done
nit: see comment on third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc
Done
nit: since you have the same logic here and in third_party/blink/renderer/modules/service_worker/web_service_worker_fetch_context_impl.cc and content/renderer/render_frame_impl.cc, and you know in future CLs it will likely expand to multiple checks (aside from just one feature), it might make sense to factor the logic for this gate into a helper function even though now it would just return `base::FeatureList::IsEnabled(blink::features::kGlobalPrivacyControlForce)`. This ensures the other callsite isn't forgotten as the code evolves.
- Ari Chivukula
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
mind rebasing? I can run tests then so we can see what works or not. the current merge conflict prevents it
Related details
- Maciej Czarnecki
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 1 comment![]( "Open in Gerrit")
mind rebasing? I can run tests then so we can see what works or not. the current merge conflict prevents it
Done. Please run the tests :) The file with enums is changing frequently, so soon there will be a conflict again :/
Related details
- Ari Chivukula
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Message from Ari Chivukula![]( "Open in Gerrit")
Set Ready For Review
Related details
- Ari Chivukula
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 4 comments![]( "Open in Gerrit")
Nice! I think this is just about ready for code-owner review, but I haven't read tests yet (waiting on the CQ for that).
One thing to consider is adding public-facing (external) WPTs https://chromium.googlesource.com/chromium/src/+/HEAD/docs/testing/web_platform_tests.md
That doesn't have to be this CL though (browsertests are enough here), it could be a future one. Up to you
return IsGlobalPrivacyControlEnabled();see note on third_party/blink/renderer/modules/global_privacy_control/navigator_global_privacy_control.cc
->GlobalPrivacyControlValue();why do this when LocalFrameClientImpl::GlobalPrivacyControlValue just calls IsGlobalPrivacyControlEnabled? I know in the future we will need to cache the value on frame navigation so it's consistent, but since we aren't doing that yet it just feels unnecessary. Maybe just put a TODO with a reference to how we will need to do that in the future here? It can be resolved when it's a dynamic browser-pref instead of a static start-time flag
"1");Ari Chivukulanit: it might be good to add some UMA logging for what contexts the header is sent in - https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
Actually, maybe just another WebFeature counter (like the JS ones), also split by worker/non-worker
Related details
- Maciej Czarnecki
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
scoped_feature_list_.InitAndEnableFeature(nit: just use InitWithFeatureState
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 5 comments![]( "Open in Gerrit")
Nice! I think this is just about ready for code-owner review, but I haven't read tests yet (waiting on the CQ for that).
One thing to consider is adding public-facing (external) WPTs https://chromium.googlesource.com/chromium/src/+/HEAD/docs/testing/web_platform_tests.md
That doesn't have to be this CL though (browsertests are enough here), it could be a future one. Up to you
I would like to do it with a separate CL. This one took me already quite a lot of time and writing WPT is another new thing to me, so I will need some time to get familiar with it. Should I create an issue for it?
scoped_feature_list_.InitAndEnableFeature(Maciej Czarneckinit: just use InitWithFeatureState
Done
return IsGlobalPrivacyControlEnabled();Maciej Czarneckisee note on third_party/blink/renderer/modules/global_privacy_control/navigator_global_privacy_control.cc
Done
why do this when LocalFrameClientImpl::GlobalPrivacyControlValue just calls IsGlobalPrivacyControlEnabled? I know in the future we will need to cache the value on frame navigation so it's consistent, but since we aren't doing that yet it just feels unnecessary. Maybe just put a TODO with a reference to how we will need to do that in the future here? It can be resolved when it's a dynamic browser-pref instead of a static start-time flag
Done
"1");nit: it might be good to add some UMA logging for what contexts the header is sent in - https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
Actually, maybe just another WebFeature counter (like the JS ones), also split by worker/non-worker
I need a little help with it. I was trying to find instances of the use of 'Blink.UseCounter.Features' histograms in C++ code, but I couldn't find any. I assume that it should be done differently. Could you point me to an example of a similar metric that I can use? Or should I create a new histogram for it?
Related details
- Ari Chivukula
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 1 comment![]( "Open in Gerrit")
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
"1");Ari Chivukulanit: it might be good to add some UMA logging for what contexts the header is sent in - https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
Maciej CzarneckiActually, maybe just another WebFeature counter (like the JS ones), also split by worker/non-worker
I need a little help with it. I was trying to find instances of the use of 'Blink.UseCounter.Features' histograms in C++ code, but I couldn't find any. I assume that it should be done differently. Could you point me to an example of a similar metric that I can use? Or should I create a new histogram for it?
of course, here's a CL with an example:
https://crrev.com/c/7643254
The key modifications are:
UseCounter usage in third_party/blink/renderer/core/html/html_anchor_element.cc
enum addition to third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
xml update in tools/metrics/histograms/metadata/blink/enums.xml
Related details
- Maciej Czarnecki
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
Maciej CzarneckiNice! I think this is just about ready for code-owner review, but I haven't read tests yet (waiting on the CQ for that).
One thing to consider is adding public-facing (external) WPTs https://chromium.googlesource.com/chromium/src/+/HEAD/docs/testing/web_platform_tests.md
That doesn't have to be this CL though (browsertests are enough here), it could be a future one. Up to you
I would like to do it with a separate CL. This one took me already quite a lot of time and writing WPT is another new thing to me, so I will need some time to get familiar with it. Should I create an issue for it?
it can go on the same issue, and it's fine to add it later. Here's an example of how to run a test with and without an enabled feature flag that checks header information:
https://crrev.com/c/6774087
Ari Chivukula (Gerrit)
Ari Chivukula added 2 comments![]( "Open in Gerrit")
enum class GlobalPrivacyControlHeaderContext {is this used?
"1");Ari Chivukulanit: it might be good to add some UMA logging for what contexts the header is sent in - https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
Maciej CzarneckiActually, maybe just another WebFeature counter (like the JS ones), also split by worker/non-worker
Ari ChivukulaI need a little help with it. I was trying to find instances of the use of 'Blink.UseCounter.Features' histograms in C++ code, but I couldn't find any. I assume that it should be done differently. Could you point me to an example of a similar metric that I can use? Or should I create a new histogram for it?
of course, here's a CL with an example:
https://crrev.com/c/7643254The key modifications are:
UseCounter usage in third_party/blink/renderer/core/html/html_anchor_element.cc
enum addition to third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
xml update in tools/metrics/histograms/metadata/blink/enums.xml
I realize that you need to count in content/browser/loader/browser_initiated_resource_request.cc too, and using WebFeature counters there is tricky and likely a bad idea.
In that case, UMA is probably the better choice, documentation is here: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
you probably want to define it in: tools/metrics/histograms/metadata/network/histograms.xml
the histogram should log an enum defined in tools/metrics/histograms/metadata/network/enums.xml representing DedicatedWorker, SharedWorker, ServiceWorker, FrameNavigation, and FrameSubresource
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 1 comment![]( "Open in Gerrit")
"1");Ari Chivukulanit: it might be good to add some UMA logging for what contexts the header is sent in - https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
Maciej CzarneckiActually, maybe just another WebFeature counter (like the JS ones), also split by worker/non-worker
Ari ChivukulaI need a little help with it. I was trying to find instances of the use of 'Blink.UseCounter.Features' histograms in C++ code, but I couldn't find any. I assume that it should be done differently. Could you point me to an example of a similar metric that I can use? Or should I create a new histogram for it?
Ari Chivukulaof course, here's a CL with an example:
https://crrev.com/c/7643254The key modifications are:
UseCounter usage in third_party/blink/renderer/core/html/html_anchor_element.cc
enum addition to third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
xml update in tools/metrics/histograms/metadata/blink/enums.xml
I realize that you need to count in content/browser/loader/browser_initiated_resource_request.cc too, and using WebFeature counters there is tricky and likely a bad idea.
In that case, UMA is probably the better choice, documentation is here: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
you probably want to define it in: tools/metrics/histograms/metadata/network/histograms.xml
the histogram should log an enum defined in tools/metrics/histograms/metadata/network/enums.xml representing DedicatedWorker, SharedWorker, ServiceWorker, FrameNavigation, and FrameSubresource
I finally had time to look into it. I managed to add a new histogram in general, but I'm having an issue with the `UpdateAdditionalHeadersForBrowserInitiatedRequest` method. It's called from different contexts, and I can't figure out how to identify it from inside the method. Do you have any advice? I'm not sure how much I can change in the function to make UMA possible. Would it be OK if I added a parameter with an enum indicating the caller context? Or maybe use a shared enum for Navigation and Dedicated/Shared/Service Workers?
Related details
- Ari Chivukula
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
"1");Ari Chivukulanit: it might be good to add some UMA logging for what contexts the header is sent in - https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
Maciej CzarneckiActually, maybe just another WebFeature counter (like the JS ones), also split by worker/non-worker
Ari ChivukulaI need a little help with it. I was trying to find instances of the use of 'Blink.UseCounter.Features' histograms in C++ code, but I couldn't find any. I assume that it should be done differently. Could you point me to an example of a similar metric that I can use? Or should I create a new histogram for it?
Ari Chivukulaof course, here's a CL with an example:
https://crrev.com/c/7643254The key modifications are:
UseCounter usage in third_party/blink/renderer/core/html/html_anchor_element.cc
enum addition to third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
xml update in tools/metrics/histograms/metadata/blink/enums.xml
Maciej CzarneckiI realize that you need to count in content/browser/loader/browser_initiated_resource_request.cc too, and using WebFeature counters there is tricky and likely a bad idea.
In that case, UMA is probably the better choice, documentation is here: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
you probably want to define it in: tools/metrics/histograms/metadata/network/histograms.xml
the histogram should log an enum defined in tools/metrics/histograms/metadata/network/enums.xml representing DedicatedWorker, SharedWorker, ServiceWorker, FrameNavigation, and FrameSubresource
I finally had time to look into it. I managed to add a new histogram in general, but I'm having an issue with the `UpdateAdditionalHeadersForBrowserInitiatedRequest` method. It's called from different contexts, and I can't figure out how to identify it from inside the method. Do you have any advice? I'm not sure how much I can change in the function to make UMA possible. Would it be OK if I added a parameter with an enum indicating the caller context? Or maybe use a shared enum for Navigation and Dedicated/Shared/Service Workers?
It's okay to just note worker navigation vs frame navigation vs subresource fetch, It's probably not worth changing the API surface for this
Related details
- Maciej Czarnecki
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 2 comments![]( "Open in Gerrit")
enum class GlobalPrivacyControlHeaderContext {Maciej Czarneckiis this used?
Done
"1");Ari Chivukulanit: it might be good to add some UMA logging for what contexts the header is sent in - https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
Maciej CzarneckiActually, maybe just another WebFeature counter (like the JS ones), also split by worker/non-worker
Ari ChivukulaI need a little help with it. I was trying to find instances of the use of 'Blink.UseCounter.Features' histograms in C++ code, but I couldn't find any. I assume that it should be done differently. Could you point me to an example of a similar metric that I can use? Or should I create a new histogram for it?
Ari Chivukulaof course, here's a CL with an example:
https://crrev.com/c/7643254The key modifications are:
UseCounter usage in third_party/blink/renderer/core/html/html_anchor_element.cc
enum addition to third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
xml update in tools/metrics/histograms/metadata/blink/enums.xml
Maciej CzarneckiI realize that you need to count in content/browser/loader/browser_initiated_resource_request.cc too, and using WebFeature counters there is tricky and likely a bad idea.
In that case, UMA is probably the better choice, documentation is here: https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histograms/README.md
you probably want to define it in: tools/metrics/histograms/metadata/network/histograms.xml
the histogram should log an enum defined in tools/metrics/histograms/metadata/network/enums.xml representing DedicatedWorker, SharedWorker, ServiceWorker, FrameNavigation, and FrameSubresource
Ari ChivukulaI finally had time to look into it. I managed to add a new histogram in general, but I'm having an issue with the `UpdateAdditionalHeadersForBrowserInitiatedRequest` method. It's called from different contexts, and I can't figure out how to identify it from inside the method. Do you have any advice? I'm not sure how much I can change in the function to make UMA possible. Would it be OK if I added a parameter with an enum indicating the caller context? Or maybe use a shared enum for Navigation and Dedicated/Shared/Service Workers?
It's okay to just note worker navigation vs frame navigation vs subresource fetch, It's probably not worth changing the API surface for this
- Ari Chivukula
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 5 comments![]( "Open in Gerrit")
// LINT.ThenChange(//tools/metrics/histograms/metadata/network/enums.xml:GPCSignalSourceType)you need to define the other side in that file too
kGlobalPrivacyControlAddHeaderFrame = 5868,this isn't being used anymore right?
<int value="5868" label="GlobalPrivacyControlAddHeaderFrame"/>I think this is a holdover that's not needed anymore?
<enum name="GPCSignalSourceType">mind adding a LINT.IfChange? See other examples in this file
Diagnostic for tracking the source of Global Privacy Control (GPC) signals.please note when this is recorded and what it indicated when recorded, not just the intended use of the metric
Related details
- Maciej Czarnecki
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 5 comments![]( "Open in Gerrit")
// LINT.ThenChange(//tools/metrics/histograms/metadata/network/enums.xml:GPCSignalSourceType)you need to define the other side in that file too
Done
this isn't being used anymore right?
Done
<int value="5868" label="GlobalPrivacyControlAddHeaderFrame"/>I think this is a holdover that's not needed anymore?
Done
mind adding a LINT.IfChange? See other examples in this file
Done
Diagnostic for tracking the source of Global Privacy Control (GPC) signals.please note when this is recorded and what it indicated when recorded, not just the intended use of the metric
- Ari Chivukula
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 9 comments![]( "Open in Gerrit")
nearly ready for code-owner review, one minor issue in third_party/blink/renderer/core/frame/local_frame_client.h that changes the owners set
IN_PROC_BROWSER_TEST_P(ChromeGlobalPrivacyControlTest,up to you, but you could merge the histogram and counter tests below with the other tests above. It's okay to check results and counters in the same test, but this is also fine
base::UmaHistogramEnumeration(see third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc
base::UmaHistogramEnumeration("Network.GlobalPrivacyControlSource",see third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc
virtual bool GlobalPrivacyControlValue() = 0;is this being used right now? You might be able to discard it until the future CL where we cache it per-frame.
// TODO: Currently, the GPC signal is controlled by a feature flag, base::UmaHistogramEnumeration("Network.GlobalPrivacyControlSource",see third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc
base::UmaHistogramEnumeration("Network.GlobalPrivacyControlSource",nit: might be worth making this a constant in third_party/blink/public/common/global_privacy_control/global_privacy_control_util.h or even a function in that file that this spot just calls with the correct GPCSignalSourceType
header set, categorized by the type of the request.please explicitly state when the metric is triggered, e.g. "Recorded during request initialization time"
Related details
- Maciej Czarnecki
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 9 comments![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_P(ChromeGlobalPrivacyControlTest,up to you, but you could merge the histogram and counter tests below with the other tests above. It's okay to check results and counters in the same test, but this is also fine
Done
base::UmaHistogramEnumeration(Maciej Czarneckisee third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc
Done
base::UmaHistogramEnumeration("Network.GlobalPrivacyControlSource",Maciej Czarneckisee third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc
Done
is this being used right now? You might be able to discard it until the future CL where we cache it per-frame.
Done
// TODO: Currently, the GPC signal is controlled by a feature flag, base::UmaHistogramEnumeration("Network.GlobalPrivacyControlSource",Maciej Czarneckisee third_party/blink/renderer/platform/loader/fetch/url_loader/dedicated_or_shared_worker_global_scope_context_impl.cc
Done
base::UmaHistogramEnumeration("Network.GlobalPrivacyControlSource",nit: might be worth making this a constant in third_party/blink/public/common/global_privacy_control/global_privacy_control_util.h or even a function in that file that this spot just calls with the correct GPCSignalSourceType
I've picked the option with the constant.
please explicitly state when the metric is triggered, e.g. "Recorded during request initialization time"
- Ari Chivukula
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
request.SetHttpHeaderField(WebString::FromUTF8(kGlobalPrivacyControlHeader),I see the one above got code-moded to FromUtf8 (lowercase tf) can you check other spots too?
Related details
- Daniel Cheng
- Erik Chen
- Maciej Czarnecki
- Nidhi Jaju
- mmenke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula voted Code-Review+1![]( "Open in Gerrit")
- Kentaro Hara
- Maciej Czarnecki
- Nidhi Jaju
- mmenke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
You just want me to sign off on content/browser/loader/browser_initiated_resource_request.cc? Or is there something more?
With a CL this large, particularly with a lot of files with * ownership, you should be clear on what you want folks to do.
Related details
- Erik Chen
- Kentaro Hara
- Maciej Czarnecki
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
You just want me to sign off on content/browser/loader/browser_initiated_resource_request.cc? Or is there something more?
With a CL this large, particularly with a lot of files with * ownership, you should be clear on what you want folks to do.
Good point, I'm looking for holistic review from everyone to some degree as this is code from a non-committer. That said, mostly looking for owner-specific review as it does LGTM
Related details
- Erik Chen
- Kentaro Hara
- Maciej Czarnecki
- Nidhi Jaju
- mmenke
Erik Chen (Gerrit)
Erik Chen added 1 comment![]( "Open in Gerrit")
I was a bit surprised to see a bunch of tests added to //chrome given that there is no business logic in //chrome. Should the tests be elsewhere instead?
Related details
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
I was a bit surprised to see a bunch of tests added to //chrome given that there is no business logic in //chrome. Should the tests be elsewhere instead?
That's a good point, they probably could be content browser tests like https://source.chromium.org/chromium/chromium/src/+/main:content/browser/loader/background_resource_fetch_browsertest.cc
Nidhi Jaju (Gerrit)
Nidhi Jaju added 3 comments![]( "Open in Gerrit")
bool globalPrivacyControl(NavigatorBase& navigator) {It doesn't look like this is used?
<histogram name="Network.GlobalPrivacyControlSource" enum="GPCSignalSourceType"It looks like this histogram is recorded for every fetch request, which may cause performance issues since it's on the critical path. Can we consider subsampling the histogram or aggregating at a higher level i.e. once per page load or once per worker initialization? Also, maybe we should use histogram macros instead?
During request initialization, this metric records the source of eachCould you be a bit more specific about when this metric is recorded (i.e. once per request being finalized, etc.?)
Related details
- Kentaro Hara
- Maciej Czarnecki
- mmenke
mmenke (Gerrit)
mmenke added 3 comments![]( "Open in Gerrit")
}
headers->SetHeaderIfMissing(blink::kGlobalPrivacyControlHeader, "1");I hate being the one to push back here, but what's the plan here? Just enable the feature for everyone globally, so the header is attached to every single request unconditionally?
Sending a 1 for all users seems to explicitly violate the spec. "A value of 1 means the user has indicated that they prefer their information not be shared with, or sold to, third parties." that seems to imply sending a "1" should be opt-in, not opt-out, for better or for worse. Admittedly, you could consider installing the browser itself an indication of a preference, but I'm skeptical we want to do that.
My understanding is that from past experiments with similar headers, sending them by default results in them universally being ignored, while making them opt-in tends to result in at least some sites respecting them.
<histogram name="Network.GlobalPrivacyControlSource" enum="GPCSignalSourceType"It looks like this histogram is recorded for every fetch request, which may cause performance issues since it's on the critical path. Can we consider subsampling the histogram or aggregating at a higher level i.e. once per page load or once per worker initialization? Also, maybe we should use histogram macros instead?
Do we even need this histogram? It's not clear to me what we intend to gain from it. If Ari has plans to use it, that's fine, but I'm just not seeing the purpose.
<owner>mccz...@gmail.com</owner>Are you a Chrome committer yet? If not, you probably shouldn't be a histogram owner.
Note: Consider this comment non-authoritative.
Related details
- Kentaro Hara
- Maciej Czarnecki
Ari Chivukula (Gerrit)
Ari Chivukula added 2 comments![]( "Open in Gerrit")
}
headers->SetHeaderIfMissing(blink::kGlobalPrivacyControlHeader, "1");I hate being the one to push back here, but what's the plan here? Just enable the feature for everyone globally, so the header is attached to every single request unconditionally?
Sending a 1 for all users seems to explicitly violate the spec. "A value of 1 means the user has indicated that they prefer their information not be shared with, or sold to, third parties." that seems to imply sending a "1" should be opt-in, not opt-out, for better or for worse. Admittedly, you could consider installing the browser itself an indication of a preference, but I'm skeptical we want to do that.
My understanding is that from past experiments with similar headers, sending them by default results in them universally being ignored, while making them opt-in tends to result in at least some sites respecting them.
I agree with all your critiques, but that's the approach taken in https://www.w3.org/TR/gpc/
The difference with this header (as argued in the spec) is the legislative/policy backing in some localities
<histogram name="Network.GlobalPrivacyControlSource" enum="GPCSignalSourceType"mmenkeIt looks like this histogram is recorded for every fetch request, which may cause performance issues since it's on the critical path. Can we consider subsampling the histogram or aggregating at a higher level i.e. once per page load or once per worker initialization? Also, maybe we should use histogram macros instead?
Do we even need this histogram? It's not clear to me what we intend to gain from it. If Ari has plans to use it, that's fine, but I'm just not seeing the purpose.
I'd like to have something here to track moving forward
mmenke (Gerrit)
mmenke voted and added 1 comment![]( "Open in Gerrit")
Votes added by mmenke
| Code-Review | -1 |
1 comment
}
headers->SetHeaderIfMissing(blink::kGlobalPrivacyControlHeader, "1");Ari ChivukulaI hate being the one to push back here, but what's the plan here? Just enable the feature for everyone globally, so the header is attached to every single request unconditionally?
Sending a 1 for all users seems to explicitly violate the spec. "A value of 1 means the user has indicated that they prefer their information not be shared with, or sold to, third parties." that seems to imply sending a "1" should be opt-in, not opt-out, for better or for worse. Admittedly, you could consider installing the browser itself an indication of a preference, but I'm skeptical we want to do that.
My understanding is that from past experiments with similar headers, sending them by default results in them universally being ignored, while making them opt-in tends to result in at least some sites respecting them.
I agree with all your critiques, but that's the approach taken in https://www.w3.org/TR/gpc/
The difference with this header (as argued in the spec) is the legislative/policy backing in some localities
```
This document does not specify what information must be presented to a user
before activating GPC.
```
That seems to be very different from what you just said - the document explicitly suggests no policies for when to set the header.
Also, IANAL, but:
```
Different jurisdictions have different prerequisites before a platform can enable
a universal opt-out like GPC. Many US states say that a user agent may not send a
universal opt-out signal by "default," though at least one state has said that
selecting a privacy focused user agent is a sufficient indicator of user intent.
```
That being the case, I'm not comfortable signing off on this CL without approval from legal.
Related details
- Kentaro Hara
- Maciej Czarnecki
Code-Coverage
Code-Owners
Code-Review
Ari Chivukula (Gerrit)
Ari Chivukula voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ari Chivukula
| Code-Review | -1 |
1 comment
Sorry about this, will have to block for now. I hope to get back shortly
Maciej Czarnecki (Gerrit)
Maciej Czarnecki added 2 comments![]( "Open in Gerrit")
}
headers->SetHeaderIfMissing(blink::kGlobalPrivacyControlHeader, "1");Ari ChivukulaI hate being the one to push back here, but what's the plan here? Just enable the feature for everyone globally, so the header is attached to every single request unconditionally?
Sending a 1 for all users seems to explicitly violate the spec. "A value of 1 means the user has indicated that they prefer their information not be shared with, or sold to, third parties." that seems to imply sending a "1" should be opt-in, not opt-out, for better or for worse. Admittedly, you could consider installing the browser itself an indication of a preference, but I'm skeptical we want to do that.
My understanding is that from past experiments with similar headers, sending them by default results in them universally being ignored, while making them opt-in tends to result in at least some sites respecting them.
mmenkeI agree with all your critiques, but that's the approach taken in https://www.w3.org/TR/gpc/
The difference with this header (as argued in the spec) is the legislative/policy backing in some localities
```
This document does not specify what information must be presented to a user
before activating GPC.
```That seems to be very different from what you just said - the document explicitly suggests no policies for when to set the header.
Also, IANAL, but:
```
Different jurisdictions have different prerequisites before a platform can enable
a universal opt-out like GPC. Many US states say that a user agent may not send a
universal opt-out signal by "default," though at least one state has said that
selecting a privacy focused user agent is a sufficient indicator of user intent.
```That being the case, I'm not comfortable signing off on this CL without approval from legal.
This CL currently enables GPC as a feature without a defining if it will be enabled by default. I was planning to make it controllable via settings, similar to DNT header. I agree that a legal opinion would be helpful given the varying regional requirements. A few points for consideration:
- Brave (and other privacy-focused browsers) already enables this by default
- Chromium needs to be compliant with CCPA by 2027.
- Digital Omnibus Act mentions an automatic signal for personal data choices, having GPC already implemented in chromium can lead to reusing it instead of requiring implementation of a new standard.
- Could you reach out to legal for their perspective and loop back once you have an update?
<owner>mccz...@gmail.com</owner>Are you a Chrome committer yet? If not, you probably shouldn't be a histogram owner.
Note: Consider this comment non-authoritative.
@ari...@chromium.org Can I make you the sole owner here, or would you like to propose someone extra here?
(Maybe one day I will become a committer, but first I need to learn more code in areas beyond my current expertise. I see that I've picked a hard one for the first bigger CL. Personally, I would like to work with different components to those I use in my job.)
Related details
- Ari Chivukula
- mmenke
Ari Chivukula (Gerrit)
Ari Chivukula added 2 comments![]( "Open in Gerrit")
We will reach out once we know more. Until then you're welcome to take more feedback or let this sit, it's up to you
<owner>mccz...@gmail.com</owner>Maciej CzarneckiAre you a Chrome committer yet? If not, you probably shouldn't be a histogram owner.
Note: Consider this comment non-authoritative.
@ari...@chromium.org Can I make you the sole owner here, or would you like to propose someone extra here?
(Maybe one day I will become a committer, but first I need to learn more code in areas beyond my current expertise. I see that I've picked a hard one for the first bigger CL. Personally, I would like to work with different components to those I use in my job.)
I can be the sole owner, in the future after more contributions you could be put up as a committer
Related details
- Maciej Czarnecki
- mmenke
Ari Chivukula (Gerrit)
Ari Chivukula voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ari Chivukula
| Code-Review | +0 |
2 comments
Looks like GPC's back on the menu, contributors!
This can move forward with the current scope, and supporting existing WPTs after might be possible too (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/gpc/), though that would require some tooling to support per-page settings (still without UX).
}
headers->SetHeaderIfMissing(blink::kGlobalPrivacyControlHeader, "1");Ari ChivukulaI hate being the one to push back here, but what's the plan here? Just enable the feature for everyone globally, so the header is attached to every single request unconditionally?
Sending a 1 for all users seems to explicitly violate the spec. "A value of 1 means the user has indicated that they prefer their information not be shared with, or sold to, third parties." that seems to imply sending a "1" should be opt-in, not opt-out, for better or for worse. Admittedly, you could consider installing the browser itself an indication of a preference, but I'm skeptical we want to do that.
My understanding is that from past experiments with similar headers, sending them by default results in them universally being ignored, while making them opt-in tends to result in at least some sites respecting them.
mmenkeI agree with all your critiques, but that's the approach taken in https://www.w3.org/TR/gpc/
The difference with this header (as argued in the spec) is the legislative/policy backing in some localities
Maciej Czarnecki```
This document does not specify what information must be presented to a user
before activating GPC.
```That seems to be very different from what you just said - the document explicitly suggests no policies for when to set the header.
Also, IANAL, but:
```
Different jurisdictions have different prerequisites before a platform can enable
a universal opt-out like GPC. Many US states say that a user agent may not send a
universal opt-out signal by "default," though at least one state has said that
selecting a privacy focused user agent is a sufficient indicator of user intent.
```That being the case, I'm not comfortable signing off on this CL without approval from legal.
Ari ChivukulaThis CL currently enables GPC as a feature without a defining if it will be enabled by default. I was planning to make it controllable via settings, similar to DNT header. I agree that a legal opinion would be helpful given the varying regional requirements. A few points for consideration:
- Brave (and other privacy-focused browsers) already enables this by default
- Chromium needs to be compliant with CCPA by 2027.
- Digital Omnibus Act mentions an automatic signal for personal data choices, having GPC already implemented in chromium can lead to reusing it instead of requiring implementation of a new standard.
- Could you reach out to legal for their perspective and loop back once you have an update?
We will reach out once we know more. Until then you're welcome to take more feedback or let this sit, it's up to you
Done
mmenke (Gerrit)
mmenke voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Related details
- Maciej Czarnecki
Code-Coverage
Code-Owners
Code-Review