[SmartRestart] Add UpdateOnZeroWindowEnabled enterprise policy [chromium/src : main]
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
hend...@chromium.org is from context(chrome/enterprise/gwsq/enterprise-policy-review.gwsq)
Related details
- Alexander Hendrich
- Darryl James
- Mike Taylor
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike Taylor (Gerrit)
Mike Taylor voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike Taylor
| Code-Review | +1 |
1 comment
components/policy/resources/templates/policy_definitions/Miscellaneous/UpdateOnZeroWindowEnabled.yaml LGTM
Related details
- Alexander Hendrich
- Darryl James
- Victor Tan
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 5 comments![]( "Open in Gerrit")
Shape looks good; small asks 👍; thanks!
histogram_tester.ExpectTotalCount(
"Session.SmartRestart.ZeroWindow.ExecutionOutcome", 0);Should this expectation be preserved in the updated test?
bool IsZeroWindowBlockedByEnterprisePolicy() {Can this be renamed to something like `SupportsZeroWindowRestarts` or similar? This way the implementation aligns with the function name. Right now it performs a negation in the pref path, and returns false in the default case when it should return true(?). If it returns true it should return false in `CanZeroWindowRestartProceed`
Would like to find a way to simplify this while we are here 👍
// If the policy is unset, default to true (allow relaunch).Does this comment need to be updated or does the return value need to change?
if (policy::ManagementServiceFactory::GetForPlatform()->IsManaged()) {Should this logic be kept in some form within `IsZeroWindowBlockedByEnterprisePolicy` (maybe as the final return value)? Or is this behavior change intentional?
Related details
- Alexander Hendrich
- Victor Tan
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. |
Victor Tan (Gerrit)
Victor Tan added 4 comments![]( "Open in Gerrit")
histogram_tester.ExpectTotalCount(
"Session.SmartRestart.ZeroWindow.ExecutionOutcome", 0);Should this expectation be preserved in the updated test?
Done. add verify metrics that zero-window restart is allowed by default when managed.
Can this be renamed to something like `SupportsZeroWindowRestarts` or similar? This way the implementation aligns with the function name. Right now it performs a negation in the pref path, and returns false in the default case when it should return true(?). If it returns true it should return false in `CanZeroWindowRestartProceed`
Would like to find a way to simplify this while we are here 👍
Done. renamed to `IsZeroWindowRestartAllowedByPolicy` and simplified to return true by default.
// If the policy is unset, default to true (allow relaunch).Does this comment need to be updated or does the return value need to change?
update the return values
if (policy::ManagementServiceFactory::GetForPlatform()->IsManaged()) {Should this logic be kept in some form within `IsZeroWindowBlockedByEnterprisePolicy` (maybe as the final return value)? Or is this behavior change intentional?
yea, confirm with the team, they would like to keep the enterprise policy default as true (which is allow to restart) while disable the restart feature before m152 which is weird. :)
Related details
- Alexander Hendrich
- Darryl James
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
1 comment
Related details
- Alexander Hendrich
- Victor Tan
Alexander Hendrich (Gerrit)
Alexander Hendrich voted and added 2 comments![]( "Open in Gerrit")
Votes added by Alexander Hendrich
| Code-Review | +1 |
2 comments
lgtm
1471: UpdateOnZeroWindowEnabledjust double checking: shouldn't this be called `UpdateOnZeroWindowsEnabled` (plural for "windows")? The code also seems to be using the singular form
Related details
- Victor Tan
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. |
Victor Tan (Gerrit)
Victor Tan added 1 comment![]( "Open in Gerrit")
1471: UpdateOnZeroWindowEnabledjust double checking: shouldn't this be called `UpdateOnZeroWindowsEnabled` (plural for "windows")? The code also seems to be using the singular form
Let's keep the singular form (ZeroWindow). It follows the pattern (similar to "zero-day vulnerability" or "zero-click attack"). This also maintains consistency with existing Chromium internal naming and UMA metrics(e.g., `CanZeroWindowRestartProceed()`, `Session.SmartRestart.ZeroWindow.*` ).
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Tan (Gerrit)
Victor Tan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[SmartRestart] Add UpdateOnZeroWindowEnabled enterprise policy
Add the UpdateOnZeroWindowEnabled boolean policy to control restarts
when all browser windows are closed on macOS.
This allows explicitly enabling or disabling automatic restart for
updates when no windows are open. By default (if the policy is
unconfigured), zero-window restarts are allowed.
- M chrome/browser/lifetime/BUILD.gn
- M chrome/browser/lifetime/smart_restart_manager_browsertest.cc
- M chrome/browser/lifetime/smart_restart_policy.cc
- M chrome/browser/lifetime/smart_restart_policy_unittest.cc
- M chrome/browser/policy/configuration_policy_handler_list_factory.cc
- M chrome/browser/prefs/browser_prefs.cc
- M chrome/common/pref_names.h
- M components/policy/resources/templates/policies.yaml
- A components/policy/resources/templates/policy_definitions/Miscellaneous/UpdateOnZeroWindowEnabled.yaml
- A components/policy/test/data/pref_mapping/UpdateOnZeroWindowEnabled.json
- M tools/metrics/histograms/metadata/enterprise/enums.xml
Code-Review: +1 by Alexander Hendrich, +1 by Mike Taylor, +1 by Darryl James
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |