Pass `local_state` to ash::input_method::InputMethodPersistence ctor [chromium/src : main]
Antonio Gomes (Gerrit)
Antonio Gomes voted and added 1 comment![]( "Open in Gerrit")
Votes added by Antonio Gomes
| Commit-Queue | +1 |
1 comment
PTAL
junis, refactory OWNER
hidehiko, chrome/browser OWNER
Related details
- Hidehiko Abe
- Jun Ishiguro
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Gomes (Gerrit)
Antonio Gomes added 1 comment![]( "Open in Gerrit")
PTAL
junis, refactory OWNER
hidehiko, chrome/browser OWNER
ok, there is actually two approaches of this CL:
PS 1: basically it passes `local_state` in as parameter whenever needed
PS 3: it changes the static method to be non-static, improves the ownership and adds a GetInstance() method.
PLMK if you have a preferred approach 🙏
Related details
- Hidehiko Abe
- Jun Ishiguro
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jun Ishiguro (Gerrit)
Jun Ishiguro added 5 comments![]( "Open in Gerrit")
Antonio GomesPTAL
junis, refactory OWNER
hidehiko, chrome/browser OWNER
ok, there is actually two approaches of this CL:
PS 1: basically it passes `local_state` in as parameter whenever needed
PS 3: it changes the static method to be non-static, improves the ownership and adds a GetInstance() method.
PLMK if you have a preferred approach 🙏
Could you split this CL into two? It looks like taking the local state pointer in the ctor and adding GetInstance() can be one CL, and moving the ownership can be another.
void SetUserLastInputMethodPreferenceForTesting(const AccountId& account_id,Considering manipulating the local state is InputMethodPersistence's responsibility, I think this method should be a (non-static) public method of it.
Could you try moving it into InputMethodPersistence?
void PersistUserInputMethod(const std::string& input_method_id,Can you make this a private method of `InputMethodPersistence`? Then, the GetInstance() at L70 can be removed.
DCHECK(!g_input_method_persistence);Let's use CHECK.
DCHECK(g_input_method_persistence);nit: `CHECK_EQ(g_input_method_persistence, this);`
Related details
- Antonio Gomes
- Hidehiko Abe
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. |
Antonio Gomes (Gerrit)
Antonio Gomes voted and added 5 comments![]( "Open in Gerrit")
Votes added by Antonio Gomes
| Commit-Queue | +1 |
Antonio GomesPTAL
junis, refactory OWNER
hidehiko, chrome/browser OWNER
Jun Ishigurook, there is actually two approaches of this CL:
PS 1: basically it passes `local_state` in as parameter whenever needed
PS 3: it changes the static method to be non-static, improves the ownership and adds a GetInstance() method.
PLMK if you have a preferred approach 🙏
Could you split this CL into two? It looks like taking the local state pointer in the ctor and adding GetInstance() can be one CL, and moving the ownership can be another.
CL that changes the ownership: crrev.com/c/7046780.
There is actually some other preparation CLs in the relation chain.
This one now became much simpler.
Please review.
void SetUserLastInputMethodPreferenceForTesting(const AccountId& account_id,Considering manipulating the local state is InputMethodPersistence's responsibility, I think this method should be a (non-static) public method of it.
Could you try moving it into InputMethodPersistence?
void PersistUserInputMethod(const std::string& input_method_id,Can you make this a private method of `InputMethodPersistence`? Then, the GetInstance() at L70 can be removed.
DCHECK(!g_input_method_persistence);Antonio GomesLet's use CHECK.
Done as part of CL crrev.com/c/7046780 (that changes the ownership)
DCHECK(g_input_method_persistence);Antonio Gomesnit: `CHECK_EQ(g_input_method_persistence, this);`
Related details
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jun Ishiguro (Gerrit)
Jun Ishiguro voted and added 4 comments![]( "Open in Gerrit")
Votes added by Jun Ishiguro
| Code-Review | +1 |
4 comments
InputMethodPersistence(PrefService* local_state,Please add a comment about the requirements.
#include "chrome/test/base/testing_browser_process.h"In browser_tests, we should use browser_process.h instead.
TestingBrowserProcess::GetGlobal()->local_state(), test_users_[0],Could you use `g_browser_process` directly since it's not a TestingBrowserProcess in browser_tests.
Related details
- Antonio Gomes
- Hidehiko Abe
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. |
Antonio Gomes (Gerrit)
Antonio Gomes added 3 comments![]( "Open in Gerrit")
Please add a comment about the requirements.
Done
In browser_tests, we should use browser_process.h instead.
Done
TestingBrowserProcess::GetGlobal()->local_state(), test_users_[0],Could you use `g_browser_process` directly since it's not a TestingBrowserProcess in browser_tests.
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe voted and added 2 comments![]( "Open in Gerrit")
Votes added by Hidehiko Abe
| Code-Review | +1 |
2 comments
PrefService* local_state,nit: should be `PrefService&` too.
Related details
- Antonio Gomes
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. |
Antonio Gomes (Gerrit)
Antonio Gomes voted and added 1 comment![]( "Open in Gerrit")
Votes added by Antonio Gomes
| Commit-Queue | +2 |
1 comment
nit: should be `PrefService&` too.
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
9 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/ash/input_method/input_method_persistence.h
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/ash/login/login_ui_keyboard_browsertest.cc
Insertions: 6, Deletions: 6.
The diff is too large to show. Please review the diff.
```
```
The name of the file: chrome/browser/ash/input_method/input_method_persistence.cc
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Change information
Pass `local_state` to ash::input_method::InputMethodPersistence ctor
This CL passes to the constructor of InputMethodPersistence, an
`local_state` object, so that it stops referring to `g_browser_process`.
R=hide...@chromium.org, ju...@google.com
- M chrome/browser/ash/input_method/input_method_manager_impl.cc
- M chrome/browser/ash/input_method/input_method_persistence.cc
- M chrome/browser/ash/input_method/input_method_persistence.h
- M chrome/browser/ash/input_method/input_method_persistence_unittest.cc
- M chrome/browser/ash/login/login_ui_keyboard_browsertest.cc
Code-Review: +1 by Jun Ishiguro, +1 by Hidehiko Abe
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |