Add policy for Live Caption and Live Translate on ChromeOS [chromium/src : main]
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From chrome/enterprise/gwsq/enterprise-policy-review.gwsq:
Note: A shadow reviewer was assigned to this CL. go/new-policy-review-process
Shadowed: seblalancette
Reviewer source(s):
hend...@chromium.org, seblalancette is from context(chrome/enterprise/gwsq/enterprise-policy-review.gwsq)
Related details
- Alexander Hendrich
- Sébastien Lalancette
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Moylan (Gerrit)
Andrew Moylan added 1 comment![]( "Open in Gerrit")
"reason_for_missing_test": "On ChromeOS, this preference is enabled by default by a non-policy mechanism, so it will not be at its registered default value when the policy is unset."Hi Evan, what's this referring to?
Related details
- Evan Liu
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. |
Alexander Hendrich (Gerrit)
Alexander Hendrich added 1 comment![]( "Open in Gerrit")
"reason_for_missing_test": "On ChromeOS, this preference is enabled by default by a non-policy mechanism, so it will not be at its registered default value when the policy is unset."Hi Evan, what's this referring to?
In that case, you can use the "non-simple" pref mapping test and skip the default case where no policy is set:
```
{
"os": [ "chromeos" ],
"note": "intentionally skipping the default value test because of ... INSERT EXPLANATION HERE",
"policy_pref_mapping_tests": [
{
"policies": {
"LiveCaptionEnabled": true
},
"prefs": {
"accessibility.captions.live_caption_enabled": {
"value": true
}
}
},
{
"policies": {
"LiveCaptionEnabled": false
},
"prefs": {
"accessibility.captions.live_caption_enabled": {
"value": false
}
}
}
]
}
```
note the use of `value` instead of `default_value`
Alexander Hendrich (Gerrit)
out of curiosity: where is that different default value defined for CrOS? I can't find anything in code
Evan Liu (Gerrit)
Evan Liu added 1 comment![]( "Open in Gerrit")
If you enable this test on ChromeOS as-is it fails because apparently the default value for the Live Caption preference is True instead of False. I don't know why or how this is set to True on ChromeOS. IIRC, it's supposed to be set to false here: https://source.chromium.org/chromium/chromium/src/+/eceda04c64f9395f81def4fa025576615a1f7169:components/live_caption/live_caption_controller.cc;l=85;bpv=0;bpt=0
I haven't investigated thoroughly, which is why I never ended up sending out this CL for review. It's possible there's an underlying bug somewhere.
Related details
- Alexander Hendrich
- Andrew Moylan
- Sébastien Lalancette
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. |
Sergey Poromov (Gerrit)
Sergey Poromov added 1 comment![]( "Open in Gerrit")
It seems that for some reason the pref is registered also here: https://source.chromium.org/chromium/chromium/src/+/main:ash/ash_prefs.cc;l=231;drc=14124cfccffd291611a9062ec583940c27895941
Related details
- Alexander Hendrich
- Andrew Moylan
- Evan Liu
- Sébastien Lalancette
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. |
Sergey Poromov (Gerrit)
So, I found that apparently the pref is being set (as a user value) here: https://source.chromium.org/chromium/chromium/src/+/main:ash/accessibility/accessibility_controller.cc;l=3818;drc=7a1eefbb103a46af7139cecf4315fa1b8a895bfd
This happens also when the pref is being cleared before test - this is a bit unexpected behavior.
Actually, for now it could be avoided if testing first `false`, then `true` value - as in this case we don't have to clear and change the value.
But ultimately it's an expected behavior in pref handling code.
Sergey Poromov (Gerrit)
So, please change for now to
```
[
{
"os": [
"win",
"mac",
"linux",
"chromeos"
],
"simple_policy_pref_mapping_test": {
"pref_name": "accessibility.captions.live_caption_enabled",
"default_value": false,
"values_to_test": [
false,
true
]
}
}
]
```
Sébastien Lalancette (Gerrit)
Sébastien Lalancette added 1 comment![]( "Open in Gerrit")
- chrome_os:145-Is the feature code fully completed and ready to ship? If not, it is preferable to put this new platform as `future_on` until all of the code has landed and we're shipping the policy in that milestone.
Related details
- Alexander Hendrich
- Andrew Moylan
- Evan Liu
Alexander Hendrich (Gerrit)
Alexander Hendrich added 1 comment![]( "Open in Gerrit")
@por...@chromium.org the order in which these values are tested doesn't really matter. Your suggestion would then still fail on the `default_value` case as that also checks that it actually is a default and not a user set value.
If we do want to check that the pref value is `true` in the case where no policy is set and it's coming from a user setting, not the default's pref value, then we would need to do this:
```
{
"os": [ "chromeos" ],
"note": "intentionally skipping the default value test because of ... INSERT EXPLANATION HERE",
"policy_pref_mapping_tests": [
{
"policies": {},
"prefs": {
"accessibility.captions.live_caption_enabled": {
"value": true
}
}
},{
"policies": {
"LiveCaptionEnabled": true
},
"prefs": {
"accessibility.captions.live_caption_enabled": {
"value": true
}
}
},
{
"policies": {
"LiveCaptionEnabled": false
},
"prefs": {
"accessibility.captions.live_caption_enabled": {
"value": false
}
}
}
]
}
```
note the use of `value` instead of `default_value` in the first test case
Related details
- Andrew Moylan
- Evan Liu
Sergey Poromov (Gerrit)
Sergey Poromov added 2 comments![]( "Open in Gerrit")
@Evan Liu, could you please update the tests according to the comment and check it?
Yes, your test is better.
However, it didn't fail in my case too as the value was not set as user value if it's the same as current (https://source.chromium.org/chromium/chromium/src/+/main:ash/accessibility/accessibility_controller.cc;l=953;drc=85adc28ab1a46f69c6f24479242288f0edc4589d) - so, when testing `policy value = false` it was not setting it to `false` at user level.
Sergey Poromov (Gerrit)
Sergey Poromov added 1 comment![]( "Open in Gerrit")
"reason_for_missing_test": "On ChromeOS, this preference is enabled by default by a non-policy mechanism, so it will not be at its registered default value when the policy is unset."Seems that there is still strange behavior in pref handling code - I hope to fix it by https://crrev.com/c/7545844
However, for now feel free to use tests like this for both prefs:
```
[
{
"os": [
"win",
"mac",
"linux",
"chromeos"
],
"simple_policy_pref_mapping_test": {
"pref_name": "accessibility.captions.live_caption_enabled",
"default_value": false,
"values_to_test": [
false,
true
]
}
}
]
```
Related details
- Andrew Moylan
- Evan Liu
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. |
Evan Liu (Gerrit)
Evan Liu voted and added 3 comments![]( "Open in Gerrit")
Votes added by Evan Liu
| Commit-Queue | +1 |
@Evan Liu, could you please update the tests according to the comment and check it?
Done
Is the feature code fully completed and ready to ship? If not, it is preferable to put this new platform as `future_on` until all of the code has landed and we're shipping the policy in that milestone.
The Live Caption feature has been shipped on ChromeOS for years now.
"reason_for_missing_test": "On ChromeOS, this preference is enabled by default by a non-policy mechanism, so it will not be at its registered default value when the policy is unset."- Alexander Hendrich
- Sergey Poromov
- Sébastien Lalancette
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergey Poromov (Gerrit)
Sergey Poromov added 3 comments![]( "Open in Gerrit")
Please, return to the state of Patchset 2 - it's the most correct way that now passes all the tests too.
false,
trueAs the policy fix has landed, this is not needed anymore - it works both ways.
[
{
"os": [ "linux", "mac", "win" ],
"simple_policy_pref_mapping_test": {
"pref_name": "accessibility.captions.live_translate_enabled",
"default_value": false,
"values_to_test": [ true, false ]
}
},
{
"os": [ "chromeos" ],
"reason_for_missing_test": "On ChromeOS, this preference is enabled by default by a non-policy mechanism, so it will not be at its registered default value when the policy is unset."
}
]This is not needed, just add "chromeos" to the list of "os" and it will work.
Related details
- Alexander Hendrich
- Evan Liu
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. |
Sergey Poromov (Gerrit)
Sergey Poromov added 3 comments![]( "Open in Gerrit")
Seems that Evan is OOO next week, while we'd like to land the change before M146 branch point on Monday - it's getting on CaPSE top FR list, so there are customers asking for this.
Alex, could you PTAL and we'll land on Monday if all is good?
As the policy fix has landed, this is not needed anymore - it works both ways.
Done
{
"os": [ "linux", "mac", "win" ],
"simple_policy_pref_mapping_test": {
"pref_name": "accessibility.captions.live_translate_enabled",
"default_value": false,
"values_to_test": [ true, false ]
}
},
{
"os": [ "chromeos" ],
"reason_for_missing_test": "On ChromeOS, this preference is enabled by default by a non-policy mechanism, so it will not be at its registered default value when the policy is unset."
}
]This is not needed, just add "chromeos" to the list of "os" and it will work.
Done
Related details
- Alexander Hendrich
- Evan Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |