Fix input method reset in Managed Guest Sessions [chromium/src : main]
Sergey Poromov (Gerrit)
Sergey Poromov voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Bao-Duy Tran
- Istvan Nagy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Istvan Nagy (Gerrit)
Istvan Nagy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Istvan Nagy
| Code-Review | +1 |
1 comment
// Without Fix 1, this pref is not populated during InitLocaleAndInputMethodsForNewUser.nit: I guess Fix 1 is the fix in the CL description. Would be better to write a comment independently from the CL description.
Related details
- Bao-Duy Tran
- Sergey Poromov
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sergey Poromov
| Commit-Queue | +1 |
1 comment
// Without Fix 1, this pref is not populated during InitLocaleAndInputMethodsForNewUser.nit: I guess Fix 1 is the fix in the CL description. Would be better to write a comment independently from the CL description.
Oops, you are right, that's an artifact from asking AI to write the test.
Related details
- Bao-Duy Tran
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bao-Duy Tran (Gerrit)
Bao-Duy Tran added 3 comments![]( "Open in Gerrit")
prefs->SetString(ash::prefs::kLanguagePreviousInputMethod,why must "previous" be set here too, besides "current"? (this isn't discussed in the CL desc as to how it's related to the issue and/or contributes to the fix). It might need to be explicitly set for consistency, but in that case, should it be the old value of "current" instead by definition?
IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, MGS_AllowedInputMethods_PreservesActive) {nit: spell it out in full (currently cryptic, and resembles DISBLED_ and friends)
// Without saving this pref the input method might be overridden by policy.I think it's much more worthwhile to have this comment in the prod code than here in the test. Even consider a more elaborate comment block (but less verbose than the CL desc) over there, given the subtlety but severity of the issue.
Related details
- Sergey Poromov
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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Sergey Poromov
| Commit-Queue | +1 |
3 comments
prefs->SetString(ash::prefs::kLanguagePreviousInputMethod,why must "previous" be set here too, besides "current"? (this isn't discussed in the CL desc as to how it's related to the issue and/or contributes to the fix). It might need to be explicitly set for consistency, but in that case, should it be the old value of "current" instead by definition?
Right, good finding. Even that `kLanguageCurrentInputMethod` is most likely empty, let's set to this value for consistency.
IN_PROC_BROWSER_TEST_F(DeviceLocalAccountTest, MGS_AllowedInputMethods_PreservesActive) {nit: spell it out in full (currently cryptic, and resembles DISBLED_ and friends)
Done
// Without saving this pref the input method might be overridden by policy.I think it's much more worthwhile to have this comment in the prod code than here in the test. Even consider a more elaborate comment block (but less verbose than the CL desc) over there, given the subtlety but severity of the issue.
- Bao-Duy Tran
- Istvan Nagy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bao-Duy Tran (Gerrit)
Bao-Duy Tran voted and added 3 comments![]( "Open in Gerrit")
Votes added by Bao-Duy Tran
| Code-Review | +1 |
3 comments
// Ensure that kLanguageCurrentInputMethod is explicitly populated here so thatnit: Add blank lines before line 361 and after line 372, to demarcate the block with attached comment.
IN_PROC_BROWSER_TEST_F(LoginPolicyTestBase, ManagedGuestSession_AllowedInputMethods_PreservesActive) {nit: Just to confirm, the test has been intentionally moved and thoroughly rewritten? This test passes with the fix and fails without?
Profile* const profile = GetProfileForActiveUser();
auto* imm = ash::input_method::InputMethodManager::Get();
scoped_refptr<ash::input_method::InputMethodManager::State> ime_state =
imm->GetActiveIMEState();nit: Move these closer to, or inline them where each of them is used.
Related details
- Istvan Nagy
- Sergey Poromov
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")
// Ensure that kLanguageCurrentInputMethod is explicitly populated here so thatnit: Add blank lines before line 361 and after line 372, to demarcate the block with attached comment.
Done
IN_PROC_BROWSER_TEST_F(LoginPolicyTestBase, ManagedGuestSession_AllowedInputMethods_PreservesActive) {nit: Just to confirm, the test has been intentionally moved and thoroughly rewritten? This test passes with the fix and fails without?
Oops, no, I mistakenly applied rename it on top of Patchset 3...
Profile* const profile = GetProfileForActiveUser();
auto* imm = ash::input_method::InputMethodManager::Get();
scoped_refptr<ash::input_method::InputMethodManager::State> ime_state =
imm->GetActiveIMEState();nit: Move these closer to, or inline them where each of them is used.
- Bao-Duy Tran
- Istvan Nagy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bao-Duy Tran (Gerrit)
Bao-Duy Tran added 1 comment![]( "Open in Gerrit")
prefs->GetString(ash::prefs::kLanguagePreviousInputMethod));tweak test case so this "previous" can be different from "current", thus more specifically tested?
Related details
- Istvan Nagy
- Sergey Poromov
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sergey Poromov
| Commit-Queue | +1 |
1 comment
prefs->GetString(ash::prefs::kLanguagePreviousInputMethod));tweak test case so this "previous" can be different from "current", thus more specifically tested?
Done. Also I found that it's better to set `kLanguageCurrentInputMethod` before `kLanguagePreloadEngines` that could potentially asynchronously write it too.
Related details
- Bao-Duy Tran
- Istvan Nagy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bao-Duy Tran (Gerrit)
Bao-Duy Tran added 2 comments![]( "Open in Gerrit")
manager->GetInputMethodUtil()->GetMigratedInputMethod(Is this required? Same preferred_input_method is used directly elsewhere in this method? The "migrated" conversion is just meant for some [legacy IDs from VPD](https://source.chromium.org/chromium/chromium/src/+/main:ui/base/ime/ash/input_method_util.cc;l=81;drc=d3fbdffe8a999ab6afdfb2ad6d1edb188f7c3582)?
scoped_refptr<input_method::InputMethodManager::State> ime_state =
session_manager->GetDefaultIMEState(profile);
ime_state->EnableInputMethod(migrated_preferred_input_method_id);
ime_state->ChangeInputMethod(migrated_preferred_input_method_id,
false /* show_message */);why are these now needed? They haven't been discussed in the CL desc or anywhere so far?
Related details
- Istvan Nagy
- Sergey Poromov
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 2 comments![]( "Open in Gerrit")
manager->GetInputMethodUtil()->GetMigratedInputMethod(Is this required? Same preferred_input_method is used directly elsewhere in this method? The "migrated" conversion is just meant for some [legacy IDs from VPD](https://source.chromium.org/chromium/chromium/src/+/main:ui/base/ime/ash/input_method_util.cc;l=81;drc=d3fbdffe8a999ab6afdfb2ad6d1edb188f7c3582)?
My mistake, replied below.
scoped_refptr<input_method::InputMethodManager::State> ime_state =
session_manager->GetDefaultIMEState(profile);
ime_state->EnableInputMethod(migrated_preferred_input_method_id);
ime_state->ChangeInputMethod(migrated_preferred_input_method_id,
false /* show_message */);why are these now needed? They haven't been discussed in the CL desc or anywhere so far?
Sorry for disturbance - the updated CL on Friday (patchset 12) unexpectedly failed the test and when I asked AI to try to fix it over the weekend, it updated this part too, for some reason. I removed you from attention set, but should've moved the CL to WIP (or notify as not ready for review yet) until I'll manually test and fix the CL on Monday.
I confirmed that the change made here is correct and seems that the tests now work (so, only test had to be updated).
Related details
- Bao-Duy Tran
- Istvan Nagy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bao-Duy Tran (Gerrit)
Bao-Duy Tran voted Code-Review+1![]( "Open in Gerrit")
- Istvan Nagy
- Sergey Poromov
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sergey Poromov
| Commit-Queue | +2 |
1 comment
Thanks for the review and all suggestions, Bao-Duy!
Related details
- Istvan Nagy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Fix input method reset in Managed Guest Sessions
When launching a Managed Guest Session (MGS), the user's chosen keyboard
layout on the login screen was being lost. This occurred due to a race
condition between the asynchronous Ozone layout switch and the
synchronous application of device policies:
1. UserSessionManager initiates an Ozone keyboard layout switch, which
happens asynchronously over Mojo IPC.
2. The MGS user profile initializes and synchronously applies the cached
`AllowedInputMethods` device policy via `Preferences::Init()`.
3. Because the Ozone layout change hasn't been completed yet,
`kLanguageCurrentInputMethod` is still temporarily empty.
4. `Preferences::SetInputMethodList` sees the empty layout preference
and aggressively falls back to the first layout in the allowed policy
list, discarding the user's login screen choice.
This CL fixes the issue by proactively persisting
`kLanguageCurrentInputMethod` directly inside
`InitLocaleAndInputMethodsForNewUser`. This ensures the user's chosen
layout is solidly populated in the preferences before the policy filter
is applied, successfully preserving the selection.
- M chrome/browser/ash/login/session/user_session_manager.cc
- M chrome/browser/ash/policy/core/device_local_account_browsertest.cc
Code-Review: +1 by Bao-Duy Tran
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |