Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2953033002:
Hide handwriting and voice buttons when keyboard is in restricted state (Closed)
|
Created:
3 years, 6 months ago by Azure Wei Modified:
3 years, 5 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, sadrul, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nona+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, kalyank, James Su, davemoore+watch_chromium.org, sky Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionHide handwriting and voice buttons when keyboard is in restricted state.
When the virtual keyboard is in restricted state, some advanced features
like handwriting and voice are disabled. We should also hide the
handwriting and voice buttons on the opt-in IME menu.
Add InputMethodManager::SetImeMenuFeatureEnabled() method to enable or
disable of each feature.
ImeMenuTray use InputMethodManager::GetImeMenuFeatureEnabled() to
make decision whether to show each button when showing tray view.
BUG=710242
TEST=Unit test
Review-Url: https://codereview.chromium.org/2953033002
Cr-Commit-Position: refs/heads/master@{#483706}
Committed: https://chromium.googlesource.com/chromium/src/+/079691f9dc9e49f549fa38132e8c98b69537b2c7
Patch Set 1 #Patch Set 2 : Add InputMethodManager::FeaturesRestrictedState #
Total comments: 22
Patch Set 3 : addressed reviewer's comments #Patch Set 4 #
Total comments: 22
Patch Set 5 : Rename Set/GetImeMenuFeatureEnabled #
Total comments: 2
Patch Set 6 : Add FEATURE_ALL #Messages
Total messages: 47 (27 generated)
Description was changed from ========== Hide handwriting and voice buttons when keyboard is in restricted state BUG=710242 TEST=Unit test ========== to ========== Hide handwriting and voice buttons when keyboard is in restricted state. When the virtual keyboard is in restricted state, some advanced features like handwriting and voice are disabled. We should also hide the handwriting and voice buttons on the opt-in IME menu. Add a observer class InputMethodManager::KeyboardFeaturesObserver in InputMethodManager. ImeMenuTray listens on it to change the state of the buttons. BUG=710242 TEST=Unit test ==========
azurewei@chromium.org changed reviewers: + shuchen@chromium.org
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...
azurewei@chromium.org changed reviewers: + sky@chromium.org
Hi, please review this CL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Hide handwriting and voice buttons when keyboard is in restricted state. When the virtual keyboard is in restricted state, some advanced features like handwriting and voice are disabled. We should also hide the handwriting and voice buttons on the opt-in IME menu. Add a observer class InputMethodManager::KeyboardFeaturesObserver in InputMethodManager. ImeMenuTray listens on it to change the state of the buttons. BUG=710242 TEST=Unit test ========== to ========== Hide handwriting and voice buttons when keyboard is in restricted state. When the virtual keyboard is in restricted state, some advanced features like handwriting and voice are disabled. We should also hide the handwriting and voice buttons on the opt-in IME menu. Add InputMethodManager::SetFeaturesRestrictedState() method to set the restricted state of each feature. ImeMenuTray use InputMethodManager::GetFeaturesRestrictedState() to make decision whether to show each button when showing tray view. BUG=710242 TEST=Unit test ==========
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.
ping~
sky@chromium.org changed reviewers: + jamescook@chromium.org
+jamescook as he's been working on ime related conversion.
sky@chromium.org changed reviewers: + bshe@chromium.org
+bshe for extensions https://codereview.chromium.org/2953033002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2953033002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1352: if (restricted) { no {} https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... File ui/base/ime/chromeos/input_method_manager.h (right): https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:333: virtual void SetFeaturesRestrictedState(FeaturesRestrictedState feature, How come this function doesn't take a uint32_t that is the bitmask? Do need the functionality to toggle individual features?
On 2017/06/26 15:55:12, sky wrote: > +bshe for extensions > > https://codereview.chromium.org/2953033002/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): > > https://codereview.chromium.org/2953033002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1352: if > (restricted) { > no {} > > https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... > File ui/base/ime/chromeos/input_method_manager.h (right): > > https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... > ui/base/ime/chromeos/input_method_manager.h:333: virtual void > SetFeaturesRestrictedState(FeaturesRestrictedState feature, > How come this function doesn't take a uint32_t that is the bitmask? Do need the > functionality to toggle individual features? extensions/* lgtm
jamescook@chromium.org changed reviewers: - sky@chromium.org
sky, I will take over the review. azurewei, can you try a slightly different way of doing this? I'm refactoring the IME code to make it work with go/mustash (chrome --mash). In this mode ash runs in a separate process, so it cannot access the InputMethodManagerImpl in chrome browser. In particular, InputMethodManager::Get() will return null when called in src/ash, so the emoji/handwriting/etc. code won't work under mash. I have some notes on mustash and the system tray at go/mustash-tray-ime The way to make this work is to use mojo IPC interfaces to send information from chrome to ash. Mojo docs are in src/mojo/README.md if you need them, but you probably won't. Here's what I recommend: * Have InputMethodManagerImpl send the EHV information to ImeControllerClient, perhaps through an observer * Add a method like SetImeMenuEnabledFeatures(bool emoji, bool handwriting, bool voice) to ash/public/interfaces/ime_controller.mojom * Store the booleans in ash ImeController * You might need to call Shell::Get()->system_tray_notifier()->NotifyRefreshIME() if the enabled state can change while the menu is open * Use ash::Shell::Get()->ime_controller() in the tray menu code to read the values I'm sorry to ask you to change this -- there's no way to tell from the code that I am doing this. If the mojo approach causes problems or this is too hard please let me know -- you can land something like this CL and I can refactor it. https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:179: explicit ImeButtonsView(ImeMenuTray* ime_menu_tray, nit: no explicit https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:184: show_emoji_(show_emoji), Do you need member variables for these? How about passing them as parameters to the Init() method? https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:446: input_method_manager->GetFeaturesRestrictedState(); Instead of having a global IsEmojiHandwritingVoiceOnImeMenuEnabled() followed by "restricted features", how about just having 3 booleans like emoji-enabled, handwriting-enabled and voice-enabled? This could be something like GetImeMenuFeatures() that returns a bitmask or individual GetFoo() methods. https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.h:55: bool ShouldShowBottomButtons(bool& emoji, nit: out parameters should be bool* https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray_unittest.cc:314: chromeos::input_method::InputMethodManager::Get(); nit: chromeos::input_method:: not needed (you don't use it below) https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray_unittest.cc:339: chromeos::input_method::InputMethodManager::Get(); ditto https://codereview.chromium.org/2953033002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc (right): https://codereview.chromium.org/2953033002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1650: ASSERT_EQ(InputMethodManager::FeaturesRestrictedState::RESTRICTED_NONE, nit: EXPECT_EQ here and below (unless the test will crash when the assertion fails, in which case ASSERT_EQ is preferred) https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... File ui/base/ime/chromeos/input_method_manager.h (right): https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:68: enum FeaturesRestrictedState { If you keep this, prefer "enum class", like FeaturesRestricted::NONE.
On 2017/06/26 17:21:20, James Cook wrote: > sky, I will take over the review. > > azurewei, can you try a slightly different way of doing this? > > I'm refactoring the IME code to make it work with go/mustash (chrome --mash). In > this mode ash runs in a separate process, so it cannot access the > InputMethodManagerImpl in chrome browser. In particular, > InputMethodManager::Get() will return null when called in src/ash, so the > emoji/handwriting/etc. code won't work under mash. I have some notes on mustash > and the system tray at go/mustash-tray-ime > > The way to make this work is to use mojo IPC interfaces to send information from > chrome to ash. Mojo docs are in src/mojo/README.md if you need them, but you > probably won't. Here's what I recommend: > > * Have InputMethodManagerImpl send the EHV information to ImeControllerClient, > perhaps through an observer > * Add a method like SetImeMenuEnabledFeatures(bool emoji, bool handwriting, bool > voice) to ash/public/interfaces/ime_controller.mojom > * Store the booleans in ash ImeController > * You might need to call > Shell::Get()->system_tray_notifier()->NotifyRefreshIME() if the enabled state > can change while the menu is open > * Use ash::Shell::Get()->ime_controller() in the tray menu code to read the > values > > I'm sorry to ask you to change this -- there's no way to tell from the code that > I am doing this. If the mojo approach causes problems or this is too hard please > let me know -- you can land something like this CL and I can refactor it. Thanks for reviewing! I'm really unfamiliar with mojo. Also, I mainly work on chromeos stuff with 10% time for fixing serious bugs. So, would mind I only submitting this CL to fix the p0 bug? Thanks! > > https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... > File ash/system/ime_menu/ime_menu_tray.cc (right): > > https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... > ash/system/ime_menu/ime_menu_tray.cc:179: explicit ImeButtonsView(ImeMenuTray* > ime_menu_tray, > nit: no explicit > > https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... > ash/system/ime_menu/ime_menu_tray.cc:184: show_emoji_(show_emoji), > Do you need member variables for these? How about passing them as parameters to > the Init() method? > > https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... > ash/system/ime_menu/ime_menu_tray.cc:446: > input_method_manager->GetFeaturesRestrictedState(); > Instead of having a global IsEmojiHandwritingVoiceOnImeMenuEnabled() followed by > "restricted features", how about just having 3 booleans like emoji-enabled, > handwriting-enabled and voice-enabled? > > This could be something like GetImeMenuFeatures() that returns a bitmask or > individual GetFoo() methods. > > https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... > File ash/system/ime_menu/ime_menu_tray.h (right): > > https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... > ash/system/ime_menu/ime_menu_tray.h:55: bool ShouldShowBottomButtons(bool& > emoji, > nit: out parameters should be bool* > > https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... > File ash/system/ime_menu/ime_menu_tray_unittest.cc (right): > > https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... > ash/system/ime_menu/ime_menu_tray_unittest.cc:314: > chromeos::input_method::InputMethodManager::Get(); > nit: chromeos::input_method:: not needed (you don't use it below) > > https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... > ash/system/ime_menu/ime_menu_tray_unittest.cc:339: > chromeos::input_method::InputMethodManager::Get(); > ditto > > https://codereview.chromium.org/2953033002/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc > (right): > > https://codereview.chromium.org/2953033002/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1650: > ASSERT_EQ(InputMethodManager::FeaturesRestrictedState::RESTRICTED_NONE, > nit: EXPECT_EQ here and below (unless the test will crash when the assertion > fails, in which case ASSERT_EQ is preferred) > > https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... > File ui/base/ime/chromeos/input_method_manager.h (right): > > https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... > ui/base/ime/chromeos/input_method_manager.h:68: enum FeaturesRestrictedState { > If you keep this, prefer "enum class", like FeaturesRestricted::NONE.
https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:179: explicit ImeButtonsView(ImeMenuTray* ime_menu_tray, On 2017/06/26 17:21:19, James Cook wrote: > nit: no explicit Done. https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:184: show_emoji_(show_emoji), On 2017/06/26 17:21:19, James Cook wrote: > Do you need member variables for these? How about passing them as parameters to > the Init() method? These are unnecessary. Passed them with Init(). https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:446: input_method_manager->GetFeaturesRestrictedState(); On 2017/06/26 17:21:19, James Cook wrote: > Instead of having a global IsEmojiHandwritingVoiceOnImeMenuEnabled() followed by > "restricted features", how about just having 3 booleans like emoji-enabled, > handwriting-enabled and voice-enabled? > > This could be something like GetImeMenuFeatures() that returns a bitmask or > individual GetFoo() methods. Done. https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.h:55: bool ShouldShowBottomButtons(bool& emoji, On 2017/06/26 17:21:19, James Cook wrote: > nit: out parameters should be bool* Done. https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray_unittest.cc:314: chromeos::input_method::InputMethodManager::Get(); On 2017/06/26 17:21:19, James Cook wrote: > nit: chromeos::input_method:: not needed (you don't use it below) Done. https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray_unittest.cc:339: chromeos::input_method::InputMethodManager::Get(); On 2017/06/26 17:21:19, James Cook wrote: > ditto Done. https://codereview.chromium.org/2953033002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2953033002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1352: if (restricted) { On 2017/06/26 15:55:12, sky wrote: > no {} Done. https://codereview.chromium.org/2953033002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc (right): https://codereview.chromium.org/2953033002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1650: ASSERT_EQ(InputMethodManager::FeaturesRestrictedState::RESTRICTED_NONE, On 2017/06/26 17:21:19, James Cook wrote: > nit: EXPECT_EQ here and below (unless the test will crash when the assertion > fails, in which case ASSERT_EQ is preferred) Done. https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... File ui/base/ime/chromeos/input_method_manager.h (right): https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:68: enum FeaturesRestrictedState { On 2017/06/26 17:21:19, James Cook wrote: > If you keep this, prefer "enum class", like FeaturesRestricted::NONE. Using 'enum class', it cannot be operanded to binary expression with uint32_t https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:333: virtual void SetFeaturesRestrictedState(FeaturesRestrictedState feature, On 2017/06/26 15:55:12, sky wrote: > How come this function doesn't take a uint32_t that is the bitmask? Do need the > functionality to toggle individual features? I want to hide the state behind. I changed GetFeaturesRestricted() as GetFeaturesRestricted(FeatureRestricted), hope it will look better.
Not using mojo is fine for this CL, but you will need to learn at least a little bit about it soon. The ash system UI will be running in its own process and you'll need to use mojo IPC to talk to it from chrome. Mostly you'll end up copy/pasting function declarations in .mojom IDL files. https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2953033002/diff/20001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:446: input_method_manager->GetFeaturesRestrictedState(); On 2017/06/27 14:27:14, Azure Wei wrote: > On 2017/06/26 17:21:19, James Cook wrote: > > Instead of having a global IsEmojiHandwritingVoiceOnImeMenuEnabled() followed > by > > "restricted features", how about just having 3 booleans like emoji-enabled, > > handwriting-enabled and voice-enabled? > > > > This could be something like GetImeMenuFeatures() that returns a bitmask or > > individual GetFoo() methods. > > Done. This is not what I meant. See new comment on InputMethodManagerImpl. https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... File ui/base/ime/chromeos/input_method_manager.h (right): https://codereview.chromium.org/2953033002/diff/20001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:68: enum FeaturesRestrictedState { On 2017/06/27 14:27:15, Azure Wei wrote: > On 2017/06/26 17:21:19, James Cook wrote: > > If you keep this, prefer "enum class", like FeaturesRestricted::NONE. > > Using 'enum class', it cannot be operanded to binary expression with uint32_t You can do it with static_cast<>. But I agree it's a little ugly. Since you are not changing this to enum class, please add back a prefix to the values, like FEATURE_EMOJI. https://codereview.chromium.org/2953033002/diff/60001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2953033002/diff/60001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:360: if (show_bottom_buttons) nit: add {} https://codereview.chromium.org/2953033002/diff/60001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/2953033002/diff/60001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray_unittest.cc:327: InputMethodManager::FeaturesRestricted::NONE)); nit: No "::FeaturesRestricted::" for plain enums. https://codereview.chromium.org/2953033002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2953033002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1353: restricted ? FeaturesRestricted::NONE : (~FeaturesRestricted::NONE); nit: () not needed, here or below https://codereview.chromium.org/2953033002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc (right): https://codereview.chromium.org/2953033002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1652: // Sets emoji restricted and not restricted super-nit: Comments should end with "." here and below https://codereview.chromium.org/2953033002/diff/60001/extensions/browser/api/... File extensions/browser/api/virtual_keyboard/virtual_keyboard_api.cc (right): https://codereview.chromium.org/2953033002/diff/60001/extensions/browser/api/... extensions/browser/api/virtual_keyboard/virtual_keyboard_api.cc:34: return RespondNow( Won't this function early-exit if the features have different values? Then your code below won't run and you can't set the features individually. https://codereview.chromium.org/2953033002/diff/60001/extensions/browser/api/... extensions/browser/api/virtual_keyboard/virtual_keyboard_api.cc:46: params->restrictions.voice_input_enabled); Are you sure this is correct? You are restricting the voice feature when voice input is enabled. https://codereview.chromium.org/2953033002/diff/60001/ui/base/ime/chromeos/in... File ui/base/ime/chromeos/input_method_manager.h (right): https://codereview.chromium.org/2953033002/diff/60001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:70: EMOJI = 1 << 0, If you use enum instead of enum class these should all be FEATURE_EMOJI, FEATURE_HANDWRITING, etc. https://codereview.chromium.org/2953033002/diff/60001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:329: virtual bool IsEmojiHandwritingVoiceOnImeMenuEnabled() = 0; This function partially duplicates GetFeaturesRestricted(). I think it would be better to remove this function and use GetFeatureEnabled() or GetImeMenuFeatureEnabled(). https://codereview.chromium.org/2953033002/diff/60001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:333: virtual void SetFeaturesRestricted(FeaturesRestricted feature, Please use positive sense booleans, so SetFeatureEnabled() or SetImeMenuFeatureEnabled(). go/tott-387 https://codereview.chromium.org/2953033002/diff/60001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:337: virtual bool GetFeaturesRestricted(FeaturesRestricted feature) const = 0; GetFeatureEnabled() or GetImeMenuFeatureEnabled()
tbarzic@chromium.org changed reviewers: + tbarzic@chromium.org
https://codereview.chromium.org/2953033002/diff/60001/extensions/browser/api/... File extensions/browser/api/virtual_keyboard/virtual_keyboard_api.cc (right): https://codereview.chromium.org/2953033002/diff/60001/extensions/browser/api/... extensions/browser/api/virtual_keyboard/virtual_keyboard_api.cc:46: params->restrictions.voice_input_enabled); On 2017/06/27 16:31:11, James Cook wrote: > Are you sure this is correct? You are restricting the voice feature when voice > input is enabled. yeah, this looks wrong; feature should be restricted if it's not enabled.
https://codereview.chromium.org/2953033002/diff/60001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2953033002/diff/60001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray.cc:360: if (show_bottom_buttons) On 2017/06/27 16:31:11, James Cook wrote: > nit: add {} Done. https://codereview.chromium.org/2953033002/diff/60001/ash/system/ime_menu/ime... File ash/system/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/2953033002/diff/60001/ash/system/ime_menu/ime... ash/system/ime_menu/ime_menu_tray_unittest.cc:327: InputMethodManager::FeaturesRestricted::NONE)); On 2017/06/27 16:31:11, James Cook wrote: > nit: No "::FeaturesRestricted::" for plain enums. Done. https://codereview.chromium.org/2953033002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2953033002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:1353: restricted ? FeaturesRestricted::NONE : (~FeaturesRestricted::NONE); On 2017/06/27 16:31:11, James Cook wrote: > nit: () not needed, here or below Done. https://codereview.chromium.org/2953033002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc (right): https://codereview.chromium.org/2953033002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1652: // Sets emoji restricted and not restricted On 2017/06/27 16:31:11, James Cook wrote: > super-nit: Comments should end with "." here and below Done. https://codereview.chromium.org/2953033002/diff/60001/extensions/browser/api/... File extensions/browser/api/virtual_keyboard/virtual_keyboard_api.cc (right): https://codereview.chromium.org/2953033002/diff/60001/extensions/browser/api/... extensions/browser/api/virtual_keyboard/virtual_keyboard_api.cc:34: return RespondNow( On 2017/06/27 16:31:11, James Cook wrote: > Won't this function early-exit if the features have different values? Then your > code below won't run and you can't set the features individually. Yes, this will early-exit. The current API doesn't support different features with different values. We just do nothing with the 'wrong call'. But I think the API is going to support restricting features separately very soon, which will remove the Error('') line. https://codereview.chromium.org/2953033002/diff/60001/extensions/browser/api/... extensions/browser/api/virtual_keyboard/virtual_keyboard_api.cc:46: params->restrictions.voice_input_enabled); On 2017/06/27 17:51:19, tbarzic wrote: > On 2017/06/27 16:31:11, James Cook wrote: > > Are you sure this is correct? You are restricting the voice feature when voice > > input is enabled. > > yeah, this looks wrong; feature should be restricted if it's not enabled. You are right, this is wrong. Corrected. Thanks! https://codereview.chromium.org/2953033002/diff/60001/ui/base/ime/chromeos/in... File ui/base/ime/chromeos/input_method_manager.h (right): https://codereview.chromium.org/2953033002/diff/60001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:70: EMOJI = 1 << 0, On 2017/06/27 16:31:11, James Cook wrote: > If you use enum instead of enum class these should all be FEATURE_EMOJI, > FEATURE_HANDWRITING, etc. Done. https://codereview.chromium.org/2953033002/diff/60001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:329: virtual bool IsEmojiHandwritingVoiceOnImeMenuEnabled() = 0; On 2017/06/27 16:31:11, James Cook wrote: > This function partially duplicates GetFeaturesRestricted(). I think it would be > better to remove this function and use GetFeatureEnabled() or > GetImeMenuFeatureEnabled(). > This is a different condition. IsEmojiHandwritingVoiceOnImeMenuEnabled() is for getting the feature flag whether to show bottom (e/h/v) buttons if possible. If IsEmojiHandwritingVoiceOnImeMenuEnabled() is false, e/h/v buttons will never be shown on the opt-in ImeMenuTray even all features are 'not restricted'. https://codereview.chromium.org/2953033002/diff/60001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:333: virtual void SetFeaturesRestricted(FeaturesRestricted feature, On 2017/06/27 16:31:11, James Cook wrote: > Please use positive sense booleans, so SetFeatureEnabled() or > SetImeMenuFeatureEnabled(). go/tott-387 Done. https://codereview.chromium.org/2953033002/diff/60001/ui/base/ime/chromeos/in... ui/base/ime/chromeos/input_method_manager.h:337: virtual bool GetFeaturesRestricted(FeaturesRestricted feature) const = 0; On 2017/06/27 16:31:11, James Cook wrote: > GetFeatureEnabled() or GetImeMenuFeatureEnabled() Done.
LGTM https://codereview.chromium.org/2953033002/diff/60001/extensions/browser/api/... File extensions/browser/api/virtual_keyboard/virtual_keyboard_api.cc (right): https://codereview.chromium.org/2953033002/diff/60001/extensions/browser/api/... extensions/browser/api/virtual_keyboard/virtual_keyboard_api.cc:34: return RespondNow( On 2017/06/29 16:37:03, Azure Wei wrote: > On 2017/06/27 16:31:11, James Cook wrote: > > Won't this function early-exit if the features have different values? Then > your > > code below won't run and you can't set the features individually. > > Yes, this will early-exit. The current API doesn't support different features > with different values. We just do nothing with the 'wrong call'. > But I think the API is going to support restricting features separately very > soon, which will remove the Error('') line. OK. Do you know who is going to fix that? https://codereview.chromium.org/2953033002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2953033002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:863: features_enabled_state_(~0) { nit: add an enum value like FEATURE_ALL = ~0 and use it here
On 2017/06/29 16:48:29, James Cook wrote: > LGTM > > https://codereview.chromium.org/2953033002/diff/60001/extensions/browser/api/... > File extensions/browser/api/virtual_keyboard/virtual_keyboard_api.cc (right): > > https://codereview.chromium.org/2953033002/diff/60001/extensions/browser/api/... > extensions/browser/api/virtual_keyboard/virtual_keyboard_api.cc:34: return > RespondNow( > On 2017/06/29 16:37:03, Azure Wei wrote: > > On 2017/06/27 16:31:11, James Cook wrote: > > > Won't this function early-exit if the features have different values? Then > > your > > > code below won't run and you can't set the features individually. > > > > Yes, this will early-exit. The current API doesn't support different features > > with different values. We just do nothing with the 'wrong call'. > > But I think the API is going to support restricting features separately very > > soon, which will remove the Error('') line. > > OK. Do you know who is going to fix that? > > https://codereview.chromium.org/2953033002/diff/80001/chrome/browser/chromeos... > File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): > > https://codereview.chromium.org/2953033002/diff/80001/chrome/browser/chromeos... > chrome/browser/chromeos/input_method/input_method_manager_impl.cc:863: > features_enabled_state_(~0) { > nit: add an enum value like > > FEATURE_ALL = ~0 > > and use it here Also, please update CL description to refer to the new SetFeatureEnabled() method names.
Description was changed from ========== Hide handwriting and voice buttons when keyboard is in restricted state. When the virtual keyboard is in restricted state, some advanced features like handwriting and voice are disabled. We should also hide the handwriting and voice buttons on the opt-in IME menu. Add InputMethodManager::SetFeaturesRestrictedState() method to set the restricted state of each feature. ImeMenuTray use InputMethodManager::GetFeaturesRestrictedState() to make decision whether to show each button when showing tray view. BUG=710242 TEST=Unit test ========== to ========== Hide handwriting and voice buttons when keyboard is in restricted state. When the virtual keyboard is in restricted state, some advanced features like handwriting and voice are disabled. We should also hide the handwriting and voice buttons on the opt-in IME menu. Add InputMethodManager::SetImeMenuFeatureEnabled() method to enable or disable of each feature. ImeMenuTray use InputMethodManager::GetImeMenuFeatureEnabled() to make decision whether to show each button when showing tray view. BUG=710242 TEST=Unit test ==========
https://codereview.chromium.org/2953033002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right): https://codereview.chromium.org/2953033002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_manager_impl.cc:863: features_enabled_state_(~0) { On 2017/06/29 16:48:29, James Cook wrote: > nit: add an enum value like > > FEATURE_ALL = ~0 > > and use it here Done.
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.
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2953033002/#ps100001 (title: "Add FEATURE_ALL")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
Because it's late Friday night in China and this is fixing a P0 bug I'm going to put it back in the CQ.
The CQ bit was checked by jamescook@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498834839367780,
"parent_rev": "4bca026aca0b992f6cad31d794db2102ac7c353e", "commit_rev":
"079691f9dc9e49f549fa38132e8c98b69537b2c7"}
Message was sent while issue was closed.
Description was changed from ========== Hide handwriting and voice buttons when keyboard is in restricted state. When the virtual keyboard is in restricted state, some advanced features like handwriting and voice are disabled. We should also hide the handwriting and voice buttons on the opt-in IME menu. Add InputMethodManager::SetImeMenuFeatureEnabled() method to enable or disable of each feature. ImeMenuTray use InputMethodManager::GetImeMenuFeatureEnabled() to make decision whether to show each button when showing tray view. BUG=710242 TEST=Unit test ========== to ========== Hide handwriting and voice buttons when keyboard is in restricted state. When the virtual keyboard is in restricted state, some advanced features like handwriting and voice are disabled. We should also hide the handwriting and voice buttons on the opt-in IME menu. Add InputMethodManager::SetImeMenuFeatureEnabled() method to enable or disable of each feature. ImeMenuTray use InputMethodManager::GetImeMenuFeatureEnabled() to make decision whether to show each button when showing tray view. BUG=710242 TEST=Unit test Review-Url: https://codereview.chromium.org/2953033002 Cr-Commit-Position: refs/heads/master@{#483706} Committed: https://chromium.googlesource.com/chromium/src/+/079691f9dc9e49f549fa38132e8c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/079691f9dc9e49f549fa38132e8c... |
