Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(107)

Issue 34783006: Add --keyboard-usability-test flag and always show keyboard unless manually hide (Closed)

Created:
7 years, 2 months ago by bshe
Modified:
7 years, 1 month ago
Reviewers:
James Cook, sadrul, kevers
CC:
chromium-reviews
Visibility:
Public.

Description

Add --keyboard-usability-test flag and always show keyboard unless manually hide This flag is for testing virtual keyboard usability. It is intentionally excluded from about:flag page because this flag is only intended to be used for our tests. If the flag is set, virtual keyboard should always show unless user specifically click hide keyboard key. BUG=310327 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230928

Patch Set 1 #

Total comments: 7

Patch Set 2 : review #

Total comments: 1

Patch Set 3 : reviews #

Patch Set 4 : rebase(no need to review diff) #

Total comments: 4

Patch Set 5 : #

Total comments: 2

Patch Set 6 : Reviews #

Total comments: 2

Patch Set 7 : #

Patch Set 8 : fix tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -8 lines) Patch
M ash/root_window_controller_unittest.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M ash/shell.cc View 1 2 3 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc View 1 2 1 chunk +7 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc View 1 2 3 4 5 6 2 chunks +9 lines, -2 lines 0 comments Download
M ui/keyboard/keyboard_controller.cc View 1 2 3 4 3 chunks +5 lines, -1 line 0 comments Download
M ui/keyboard/keyboard_controller_unittest.cc View 1 2 3 4 5 3 chunks +37 lines, -0 lines 0 comments Download
M ui/keyboard/keyboard_switches.h View 1 chunk +3 lines, -0 lines 0 comments Download
M ui/keyboard/keyboard_switches.cc View 1 chunk +1 line, -0 lines 0 comments Download
M ui/keyboard/keyboard_util.cc View 1 2 3 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 17 (0 generated)
bshe
Hey Sadrul. Could you take a look at this CL? Thanks! Biao
7 years, 2 months ago (2013-10-23 01:18:22 UTC) #1
kevers
Drive by review. https://codereview.chromium.org/34783006/diff/1/ui/keyboard/keyboard_controller.cc File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/34783006/diff/1/ui/keyboard/keyboard_controller.cc#newcode156 ui/keyboard/keyboard_controller.cc:156: if (CommandLine::ForCurrentProcess()->HasSwitch( I think this belongs ...
7 years, 2 months ago (2013-10-23 11:37:39 UTC) #2
sadrul
https://codereview.chromium.org/34783006/diff/1/ui/keyboard/keyboard_controller.cc File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/34783006/diff/1/ui/keyboard/keyboard_controller.cc#newcode156 ui/keyboard/keyboard_controller.cc:156: if (CommandLine::ForCurrentProcess()->HasSwitch( On 2013/10/23 11:37:39, kevers wrote: > I ...
7 years, 2 months ago (2013-10-23 14:01:49 UTC) #3
bshe
Could you please take another look? https://codereview.chromium.org/34783006/diff/1/ui/keyboard/keyboard_controller.cc File ui/keyboard/keyboard_controller.cc (right): https://codereview.chromium.org/34783006/diff/1/ui/keyboard/keyboard_controller.cc#newcode156 ui/keyboard/keyboard_controller.cc:156: if (CommandLine::ForCurrentProcess()->HasSwitch( I ...
7 years, 2 months ago (2013-10-23 17:08:43 UTC) #4
sadrul
Also, please add a test for this behaviour. https://codereview.chromium.org/34783006/diff/1/ui/keyboard/keyboard_switches.h File ui/keyboard/keyboard_switches.h (right): https://codereview.chromium.org/34783006/diff/1/ui/keyboard/keyboard_switches.h#newcode17 ui/keyboard/keyboard_switches.h:17: KEYBOARD_EXPORT ...
7 years, 2 months ago (2013-10-23 17:45:05 UTC) #5
bshe
Added unit test and delayed ActivateKeyboard to PostProfileInit. It looks like the reason we need ...
7 years, 2 months ago (2013-10-24 00:23:36 UTC) #6
sadrul
https://codereview.chromium.org/34783006/diff/280001/ui/keyboard/keyboard_controller.h File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/34783006/diff/280001/ui/keyboard/keyboard_controller.h#newcode103 ui/keyboard/keyboard_controller.h:103: You shouldn't need this. See comment in test. https://codereview.chromium.org/34783006/diff/280001/ui/keyboard/keyboard_controller_unittest.cc ...
7 years, 2 months ago (2013-10-24 01:21:01 UTC) #7
bshe
Done. Thanks for review. https://codereview.chromium.org/34783006/diff/280001/ui/keyboard/keyboard_controller.h File ui/keyboard/keyboard_controller.h (right): https://codereview.chromium.org/34783006/diff/280001/ui/keyboard/keyboard_controller.h#newcode103 ui/keyboard/keyboard_controller.h:103: On 2013/10/24 01:21:01, sadrul wrote: ...
7 years, 2 months ago (2013-10-24 03:35:36 UTC) #8
sadrul
LGTM https://codereview.chromium.org/34783006/diff/330001/ui/keyboard/keyboard_controller_unittest.cc File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/34783006/diff/330001/ui/keyboard/keyboard_controller_unittest.cc#newcode359 ui/keyboard/keyboard_controller_unittest.cc:359: new KeyboardContainerObserver(keyboard_container)); I don't think you need this.
7 years, 2 months ago (2013-10-24 06:09:56 UTC) #9
bshe
Thanks! +jamescook for ash related OWNER: ash/shell.cc chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc https://codereview.chromium.org/34783006/diff/330001/ui/keyboard/keyboard_controller_unittest.cc File ui/keyboard/keyboard_controller_unittest.cc (right): https://codereview.chromium.org/34783006/diff/330001/ui/keyboard/keyboard_controller_unittest.cc#newcode359 ui/keyboard/keyboard_controller_unittest.cc:359: ...
7 years, 2 months ago (2013-10-24 13:25:20 UTC) #10
bshe
dooh. really +jamescook ash/shell.cc chrome/browser/ui/ash/ash_keyboard_controller_proxy.cc chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc
7 years, 2 months ago (2013-10-24 13:26:12 UTC) #11
James Cook
*/ash/* lgtm https://codereview.chromium.org/34783006/diff/380001/chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right): https://codereview.chromium.org/34783006/diff/380001/chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc#newcode94 chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:94: if (ash::Shell::HasInstance()) { nit: consider early return
7 years, 2 months ago (2013-10-24 15:25:22 UTC) #12
bshe
Done. Thanks! https://codereview.chromium.org/34783006/diff/380001/chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc File chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc (right): https://codereview.chromium.org/34783006/diff/380001/chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc#newcode94 chrome/browser/ui/views/ash/chrome_browser_main_extra_parts_ash.cc:94: if (ash::Shell::HasInstance()) { On 2013/10/24 15:25:23, James ...
7 years, 2 months ago (2013-10-24 15:39:19 UTC) #13
kevers
lgtm
7 years, 2 months ago (2013-10-24 17:50:48 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bshe@chromium.org/34783006/470001
7 years, 2 months ago (2013-10-24 17:53:23 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bshe@chromium.org/34783006/310010
7 years, 1 month ago (2013-10-24 23:05:09 UTC) #16
commit-bot: I haz the power
7 years, 1 month ago (2013-10-25 03:37:56 UTC) #17
Message was sent while issue was closed.
Change committed as 230928

Powered by Google App Engine
This is Rietveld 408576698