Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2599743003:
Disable emoji, handwriting and voice for password input client. (Closed)
DescriptionDisable emoji, handwriting and voice for password input client.
Make ImeMenuTray observes InputMethod to get the current input type. If
the current input client is password, do not show emoji, handwriting and
voice buttons.
We don't need to care about the input type changed after IME menu shown,
since it will hide the bubble view.
BUG=676568
TEST=Verified on local build.
Review-Url: https://codereview.chromium.org/2599743003
Cr-Commit-Position: refs/heads/master@{#442478}
Committed: https://chromium.googlesource.com/chromium/src/+/1c4c4e8afd6f1259d52e1ccb1aff046204421fdc
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add unittest. #Patch Set 3 #Patch Set 4 : Use IMEBridge to get current input context. #
Total comments: 1
Patch Set 5 : Update to use chromeos::input_method::MockInputMethodManager #Patch Set 6 : sync #
Messages
Total messages: 44 (27 generated)
Description was changed from ========== Disable emoji&hwt&voice for pwd input client. BUG=676568 TEST=Verified on local build. ========== to ========== Disable emoji&hwt&voice for pwd input client. Make ImeMenuTray observes InputMethod to get the current input type. If the current input client is password, do not show emoji, handwriting and voice buttons. We don't need to care about the input type changed after IME menu shown, since it will hide the bubble view. BUG=676568 TEST=Verified on local build. ==========
Description was changed from ========== Disable emoji&hwt&voice for pwd input client. Make ImeMenuTray observes InputMethod to get the current input type. If the current input client is password, do not show emoji, handwriting and voice buttons. We don't need to care about the input type changed after IME menu shown, since it will hide the bubble view. BUG=676568 TEST=Verified on local build. ========== to ========== Disable emoji&hwt&voice for pwd input client. Make ImeMenuTray observes InputMethod to get the current input type. If the current input client is password, do not show emoji, handwriting and voice buttons. We don't need to care about the input type changed after IME menu shown, since it will hide the bubble view. BUG=676568 TEST=Verified on local build. ==========
azurewei@chromium.org changed reviewers: + shuchen@chromium.org, stevenjb@chromium.org
Description was changed from ========== Disable emoji&hwt&voice for pwd input client. Make ImeMenuTray observes InputMethod to get the current input type. If the current input client is password, do not show emoji, handwriting and voice buttons. We don't need to care about the input type changed after IME menu shown, since it will hide the bubble view. BUG=676568 TEST=Verified on local build. ========== to ========== Disable emoji&hwt&voice for pwd input client. Make ImeMenuTray observes InputMethod to get the current input type. If the current input client is password, do not show emoji, handwriting and voice buttons. We don't need to care about the input type changed after IME menu shown, since it will hide the bubble view. BUG=676568 TEST=Verified on local build. ==========
Please review this CL. Thanks!
On 2016/12/22 11:28:15, Azure Wei wrote: > Please review this CL. Thanks! Please clarify the title/description, it is unclear what 'hwt' refers to and please add spaces around '&'.
This should have a test. https://codereview.chromium.org/2599743003/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2599743003/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:377: input_method->AddObserver(this); RemoveObserver https://codereview.chromium.org/2599743003/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/2599743003/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:101: void OnShowImeIfNeeded() override {} Avoid inlining virtual implementations in the header, move them to the C++ file, even though they are empty.
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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Disable emoji&hwt&voice for pwd input client. Make ImeMenuTray observes InputMethod to get the current input type. If the current input client is password, do not show emoji, handwriting and voice buttons. We don't need to care about the input type changed after IME menu shown, since it will hide the bubble view. BUG=676568 TEST=Verified on local build. ========== to ========== Disable emoji, handwriting and voice for password input client. Make ImeMenuTray observes InputMethod to get the current input type. If the current input client is password, do not show emoji, handwriting and voice buttons. We don't need to care about the input type changed after IME menu shown, since it will hide the bubble view. BUG=676568 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.
Addressed comments and added unit test ImeMenuTrayTest.ShowEmojiHandwritingVoiceButtons has been added. PTAL. Thanks! https://codereview.chromium.org/2599743003/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/ime_menu/ime_menu_tray.cc (right): https://codereview.chromium.org/2599743003/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_menu_tray.cc:377: input_method->AddObserver(this); On 2016/12/22 19:19:23, stevenjb wrote: > RemoveObserver Done. https://codereview.chromium.org/2599743003/diff/1/ash/common/system/chromeos/... File ash/common/system/chromeos/ime_menu/ime_menu_tray.h (right): https://codereview.chromium.org/2599743003/diff/1/ash/common/system/chromeos/... ash/common/system/chromeos/ime_menu/ime_menu_tray.h:101: void OnShowImeIfNeeded() override {} On 2016/12/22 19:19:23, stevenjb wrote: > Avoid inlining virtual implementations in the header, move them to the C++ file, > even though they are empty. Done.
Please expose an API to allow the ime extension notify the menu to update whether to show the Emoji/Hwt icons.
On 2016/12/23 03:42:14, Shu Chen wrote: > Please expose an API to allow the ime extension notify the menu to update > whether to show the Emoji/Hwt icons. Per offline discussion, decide to go with the current logic that make OS decides whether to show buttons instead of adding new API.
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.
Instead of listening on InputMethodObserver, this cl has been updated with get current input context type from IMEBridge directly. PTAL. Thanks!
Thanks for adding the test. Just one suggestion. Moving mock_input_method_manager.cc should be done in a separate CL. https://codereview.chromium.org/2599743003/diff/60001/ash/common/system/chrom... File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): https://codereview.chromium.org/2599743003/diff/60001/ash/common/system/chrom... ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:96: : public chromeos::input_method::InputMethodManager { We already have mock_input_method_manager.cc which can and should be modified to live in ui/base/ime/chromeos next to input_method_manager.h. Then we can just override IsEmojiHandwritingVoiceOnImeMenuEnabled.
On 2016/12/27 17:41:31, stevenjb wrote: > Thanks for adding the test. Just one suggestion. Moving > mock_input_method_manager.cc should be done in a separate CL. > > https://codereview.chromium.org/2599743003/diff/60001/ash/common/system/chrom... > File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): > > https://codereview.chromium.org/2599743003/diff/60001/ash/common/system/chrom... > ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:96: : public > chromeos::input_method::InputMethodManager { > We already have mock_input_method_manager.cc which can and should be modified to > live in ui/base/ime/chromeos next to input_method_manager.h. Then we can just > override IsEmojiHandwritingVoiceOnImeMenuEnabled. Sent CL: https://codereview.chromium.org/2605843002/ to add MockInputMethodManager under ui/base/ime/chromeos/. Thanks!
lgtm
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.
On 2016/12/27 17:41:31, stevenjb wrote: > Thanks for adding the test. Just one suggestion. Moving > mock_input_method_manager.cc should be done in a separate CL. > > https://codereview.chromium.org/2599743003/diff/60001/ash/common/system/chrom... > File ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc (right): > > https://codereview.chromium.org/2599743003/diff/60001/ash/common/system/chrom... > ash/common/system/chromeos/ime_menu/ime_menu_tray_unittest.cc:96: : public > chromeos::input_method::InputMethodManager { > We already have mock_input_method_manager.cc which can and should be modified to > live in ui/base/ime/chromeos next to input_method_manager.h. Then we can just > override IsEmojiHandwritingVoiceOnImeMenuEnabled. This CL has been updated. Please take another look. Thanks!
ping~
lgtm
The CQ bit was checked by azurewei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org Link to the patchset: https://codereview.chromium.org/2599743003/#ps100001 (title: "sync")
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by azurewei@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": 1484016381097680,
"parent_rev": "f4462a35264c65c6e554d22b00bb76c423c2e703", "commit_rev":
"1c4c4e8afd6f1259d52e1ccb1aff046204421fdc"}
Message was sent while issue was closed.
Description was changed from ========== Disable emoji, handwriting and voice for password input client. Make ImeMenuTray observes InputMethod to get the current input type. If the current input client is password, do not show emoji, handwriting and voice buttons. We don't need to care about the input type changed after IME menu shown, since it will hide the bubble view. BUG=676568 TEST=Verified on local build. ========== to ========== Disable emoji, handwriting and voice for password input client. Make ImeMenuTray observes InputMethod to get the current input type. If the current input client is password, do not show emoji, handwriting and voice buttons. We don't need to care about the input type changed after IME menu shown, since it will hide the bubble view. BUG=676568 TEST=Verified on local build. Review-Url: https://codereview.chromium.org/2599743003 Cr-Commit-Position: refs/heads/master@{#442478} Committed: https://chromium.googlesource.com/chromium/src/+/1c4c4e8afd6f1259d52e1ccb1aff... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/1c4c4e8afd6f1259d52e1ccb1aff... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
