Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2274013003:
Keeps the opt-in IME menu in lock screen. (Closed)
|
Created:
4 years, 4 months ago by Azure Wei Modified:
4 years, 3 months ago CC:
chromium-reviews, nona+watch_chromium.org, oshima+watch_chromium.org, shuchen+watch_chromium.org, yusukes+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThe opt-in IME menu was implemented with panel in shelf, which could not show in lock screen. Thus, the menu changed back the system menu. Since now the opt-in IME menu is created with native views on tray, we don't need to change back to system menu in lock screen.
When switching users, the opt-in IME menu could not get preferences change event to update the menu activation state. This cl also fix this issue by saving the state by profile in InputMethodManager and sending the event of switching active user.
BUG=640266
TEST=Verified on local build.
Committed: https://crrev.com/47a06020c728d53c5f3c900234f7b6def397816a
Cr-Commit-Position: refs/heads/master@{#414988}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Send message when switch user. #
Total comments: 6
Patch Set 3 : Addressed comments. #
Total comments: 6
Patch Set 4 : Trigger ImeMenuActivationChanged only state is changed. #Patch Set 5 : Sate menu_activated in StateImpl. #
Total comments: 6
Patch Set 6 : Addressed comments. #
Messages
Total messages: 33 (16 generated)
Description was changed from ========== Keeps the opt-in IME menu in lock screen. BUG=640266 TEST=Verified on local build. ========== to ========== Not change back to the old menu when locking the screen, if users have checked with the opt-in IME menu. BUG=640266 TEST=Verified on local build. ==========
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
azurewei@chromium.org changed reviewers: + shuchen@chromium.org
Please review this cl. Thanks!
https://codereview.chromium.org/2274013003/diff/1/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (left): https://codereview.chromium.org/2274013003/diff/1/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:938: } Per offline discussion, this logic should be moved to UserSessionManager::ActiveUserChanged().
Description was changed from ========== Not change back to the old menu when locking the screen, if users have checked with the opt-in IME menu. BUG=640266 TEST=Verified on local build. ========== to ========== The opt-in IME menu was implemented with panel in shelf, which could not show in lock screen. Thus, the menu changed back the system menu. Since now the opt-in IME menu is created with native views on tray, we don't need to change back to system menu in lock screen. When switching users, the opt-in IME menu could not get preferences change event to update the menu activation state. This cl also fix this issue by saving the state by profile in InputMethodManager and sending the event of switching active user. BUG=640266 TEST=Verified on local build. ==========
The CQ bit was checked by azurewei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2274013003/diff/1/chrome/browser/chromeos/inp... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (left): https://codereview.chromium.org/2274013003/diff/1/chrome/browser/chromeos/inp... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:938: } On 2016/08/25 11:49:32, Shu Chen wrote: > Per offline discussion, this logic should be moved to > UserSessionManager::ActiveUserChanged(). Done. PTAL.
azurewei@chromium.org changed reviewers: + stevenjb@chromium.org
R+ stevenjb@ for owner of: chrome/browser/chromeos/login/session/user_session_manager.cc
https://codereview.chromium.org/2274013003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2274013003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1207: return menu_map_.find(profile) != menu_map_.end() && menu_map_[profile]; Instead of doing two lookups, use an iterator, e.g. auto iter = menu_map_.find(profile); return iter != menu_map_.end() && iter->second; https://codereview.chromium.org/2274013003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right): https://codereview.chromium.org/2274013003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.h:289: bool is_ime_menu_activated_; Unused now? https://codereview.chromium.org/2274013003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.h:297: std::map<Profile*, bool> menu_map_; This should be named something like 'menu_activated_for_profile_'.
https://codereview.chromium.org/2274013003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2274013003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1207: return menu_map_.find(profile) != menu_map_.end() && menu_map_[profile]; On 2016/08/26 17:15:02, stevenjb wrote: > Instead of doing two lookups, use an iterator, e.g. > auto iter = menu_map_.find(profile); > return iter != menu_map_.end() && iter->second; Done. https://codereview.chromium.org/2274013003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right): https://codereview.chromium.org/2274013003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.h:289: bool is_ime_menu_activated_; On 2016/08/26 17:15:02, stevenjb wrote: > Unused now? Removed. Thanks. https://codereview.chromium.org/2274013003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.h:297: std::map<Profile*, bool> menu_map_; On 2016/08/26 17:15:02, stevenjb wrote: > This should be named something like 'menu_activated_for_profile_'. Done.
lgtm
https://codereview.chromium.org/2274013003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2274013003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1179: menu_activated_for_profile_[state_->profile] = is_active; Notifying ImeMenuActivationChanged() is a non-trivial op. Please make sure to only notify when the state is changed. https://codereview.chromium.org/2274013003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2274013003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1658: manager->ImeMenuActivationChanged(manager->GetImeMenuActivation(profile)); This Set(Get(...)) pattern is kinda confusing. I'm thinking to add the ImeMenuActivation state in StateImpl. WDYT?
https://codereview.chromium.org/2274013003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2274013003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1179: menu_activated_for_profile_[state_->profile] = is_active; On 2016/08/27 03:11:01, Shu Chen wrote: > Notifying ImeMenuActivationChanged() is a non-trivial op. > > Please make sure to only notify when the state is changed. The state is save by profile. We can't tell if it is 'changed' when switching users as we don't save the previous user. So we trigger the event when switching users if the two states are the same. https://codereview.chromium.org/2274013003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2274013003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1658: manager->ImeMenuActivationChanged(manager->GetImeMenuActivation(profile)); On 2016/08/27 03:11:02, Shu Chen wrote: > This Set(Get(...)) pattern is kinda confusing. > > I'm thinking to add the ImeMenuActivation state in StateImpl. > > WDYT? The saved menu_activated_ in StateImpl will also be casted into false here (cast from State to StateImpl). But if we put menu_activated_ in InputMethodManager::State, there are many other problems: InputMethodManager::SetState() will be called in many places (login/adding user/lock screen/switching user), but we don't really want to set the menu activation state in those places. We only want to set/change menu activation state when switching users. Lock/Adding user screen should keep the same with the current user. I tried to put menu_activaed_ in InputMethodManager::State or InputMethodManagerImpl::StateImpl, there are many bugs under these conditions. So, do you mind we keeping the current way? Or would you prefer adding a new event like ActiveUserChanged() in InputMethodManager?
https://codereview.chromium.org/2274013003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2274013003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1179: menu_activated_for_profile_[state_->profile] = is_active; On 2016/08/27 06:55:30, Azure Wei wrote: > On 2016/08/27 03:11:01, Shu Chen wrote: > > Notifying ImeMenuActivationChanged() is a non-trivial op. > > > > Please make sure to only notify when the state is changed. > > The state is save by profile. We can't tell if it is 'changed' when switching > users as we don't save the previous user. So we trigger the event when switching > users if the two states are the same. Oh, we could save the previous state. Sorry. Done. Added |menu_activated_| and trigger the event only when the state is changed.
https://codereview.chromium.org/2274013003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/2274013003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1658: manager->ImeMenuActivationChanged(manager->GetImeMenuActivation(profile)); On 2016/08/27 06:55:30, Azure Wei wrote: > On 2016/08/27 03:11:02, Shu Chen wrote: > > This Set(Get(...)) pattern is kinda confusing. > > > > I'm thinking to add the ImeMenuActivation state in StateImpl. > > > > WDYT? > > The saved menu_activated_ in StateImpl will also be casted into false here > (cast from State to StateImpl). But if we put menu_activated_ in > InputMethodManager::State, there are many other problems: > InputMethodManager::SetState() will be called in many places (login/adding > user/lock screen/switching user), but we don't really want to set the menu > activation state in those places. We only want to set/change menu activation > state when switching users. Lock/Adding user screen should keep the same with > the current user. > I tried to put menu_activaed_ in InputMethodManager::State or > InputMethodManagerImpl::StateImpl, there are many bugs under these conditions. > > So, do you mind we keeping the current way? > Or would you prefer adding a new event like ActiveUserChanged() in > InputMethodManager? Per offline discussion, saved menu_activated state in StateImple. Thus we don't need menu_activated_for_profile_in InputMethodManagerImpl any more.
https://codereview.chromium.org/2274013003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2274013003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1180: // active user changed. revert this comment change. https://codereview.chromium.org/2274013003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1186: menu_activated_); void InputMethodManagerImpl::ImeMenuActivationChanged(bool is_active) { state_->menu_activated = menu_activated_; MaybeNotifyImeMenuActivationChanged(); } void InputMethodManagerImpl::MaybeNotifyImeMenuActivationChanged() { if (menu_activated_ == state_->menu_activated) return; menu_activated_ = state_->menu_activated; FOR_EACH_OBSERVER(InputMethodManager::ImeMenuObserver, ime_menu_observers_, ImeMenuActivationChanged(menu_activated_)); UMA_HISTOGRAM_BOOLEAN("InputMethod.ImeMenu.ActivationChanged", menu_activated_); } https://codereview.chromium.org/2274013003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right): https://codereview.chromium.org/2274013003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.h:297: bool menu_activated_; maybe revert this rename change?
https://codereview.chromium.org/2274013003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2274013003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1180: // active user changed. On 2016/08/29 08:20:08, Shu Chen wrote: > revert this comment change. Done. https://codereview.chromium.org/2274013003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1186: menu_activated_); On 2016/08/29 08:20:08, Shu Chen wrote: > void InputMethodManagerImpl::ImeMenuActivationChanged(bool is_active) { > state_->menu_activated = menu_activated_; > MaybeNotifyImeMenuActivationChanged(); > } > > void InputMethodManagerImpl::MaybeNotifyImeMenuActivationChanged() { > if (menu_activated_ == state_->menu_activated) > return; > menu_activated_ = state_->menu_activated; > FOR_EACH_OBSERVER(InputMethodManager::ImeMenuObserver, ime_menu_observers_, > ImeMenuActivationChanged(menu_activated_)); > UMA_HISTOGRAM_BOOLEAN("InputMethod.ImeMenu.ActivationChanged", > menu_activated_); > } Done. https://codereview.chromium.org/2274013003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right): https://codereview.chromium.org/2274013003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.h:297: bool menu_activated_; On 2016/08/29 08:20:08, Shu Chen wrote: > maybe revert this rename change? Done.
lgtm
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2274013003/#ps100001 (title: "Addressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== The opt-in IME menu was implemented with panel in shelf, which could not show in lock screen. Thus, the menu changed back the system menu. Since now the opt-in IME menu is created with native views on tray, we don't need to change back to system menu in lock screen. When switching users, the opt-in IME menu could not get preferences change event to update the menu activation state. This cl also fix this issue by saving the state by profile in InputMethodManager and sending the event of switching active user. BUG=640266 TEST=Verified on local build. ========== to ========== The opt-in IME menu was implemented with panel in shelf, which could not show in lock screen. Thus, the menu changed back the system menu. Since now the opt-in IME menu is created with native views on tray, we don't need to change back to system menu in lock screen. When switching users, the opt-in IME menu could not get preferences change event to update the menu activation state. This cl also fix this issue by saving the state by profile in InputMethodManager and sending the event of switching active user. BUG=640266 TEST=Verified on local build. ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== The opt-in IME menu was implemented with panel in shelf, which could not show in lock screen. Thus, the menu changed back the system menu. Since now the opt-in IME menu is created with native views on tray, we don't need to change back to system menu in lock screen. When switching users, the opt-in IME menu could not get preferences change event to update the menu activation state. This cl also fix this issue by saving the state by profile in InputMethodManager and sending the event of switching active user. BUG=640266 TEST=Verified on local build. ========== to ========== The opt-in IME menu was implemented with panel in shelf, which could not show in lock screen. Thus, the menu changed back the system menu. Since now the opt-in IME menu is created with native views on tray, we don't need to change back to system menu in lock screen. When switching users, the opt-in IME menu could not get preferences change event to update the menu activation state. This cl also fix this issue by saving the state by profile in InputMethodManager and sending the event of switching active user. BUG=640266 TEST=Verified on local build. Committed: https://crrev.com/47a06020c728d53c5f3c900234f7b6def397816a Cr-Commit-Position: refs/heads/master@{#414988} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/47a06020c728d53c5f3c900234f7b6def397816a Cr-Commit-Position: refs/heads/master@{#414988} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
