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