Handle Caps Lock short cut (Shift+Search) in ash [part 2 of 2].

Handle Caps Lock short cut (Shift+Search) in ash [part 2 of 2].

Add caps_lock_handler.cc which implements ash::CapsLockDelegate.

Part 1 of 2: http://codereview.chromium.org/9242003
This CL depends on https://chromiumcodereview.appspot.com/9241002/ and http://codereview.chromium.org/9264004/.

NOTE: THIS CL WAS REPLACED BY http://codereview.chromium.org/9297027/ (to solve some merge issues without access to yusukes)

BUG=110129
TEST=ran browser_tests

[Yusuke Sato, 2012-01-18 10:09:20]

Daniel, Ben,
I'm moving hot key handling code in
chrome/browser/chromeos/system_key_event_listener.cc to Aura. Could you review
this?

[Daniel Erat, 2012-01-18 15:44:31]

I mostly just have style nits.

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/chro...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/chro...
chrome/browser/chromeos/system_key_event_listener.cc:344: // When Aura is in
use, the short cut is handled in ash/.
nit: either just say "Ash" instead of "ash/" or mention the specific class that
handles it

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/chro...
File chrome/browser/chromeos/system_key_event_listener_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/chro...
chrome/browser/chromeos/system_key_event_listener_browsertest.cc:31: // When
Aura is in use, the short cut is handled in ash/.
nit: same as last comment

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/ui/v...
File chrome/browser/ui/views/aura/caps_lock_handler.cc (right):

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/ui/v...
chrome/browser/ui/views/aura/caps_lock_handler.cc:19: is_running_on_chromeos_(
nit: the lines in the initializer list should be indented two more spaces

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/ui/v...
chrome/browser/ui/views/aura/caps_lock_handler.cc:29: =
chromeos::SystemKeyEventListener::GetInstance();
nit: equals sign should be at end of previous line

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/ui/v...
chrome/browser/ui/views/aura/caps_lock_handler.cc:30: // SystemKeyEventListener
is instantiated only when running on Chrome OS.
this comment doesn't exactly match the check.  should it instead be
"SystemKeyEventListener should be instantiated when we're running on Chrome
OS."?

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/ui/v...
chrome/browser/ui/views/aura/caps_lock_handler.cc:31:
DCHECK((!is_running_on_chromeos_) || system_event_listener);
nit: remove extra parentheses around !is_running_on_chromeos_

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/ui/v...
chrome/browser/ui/views/aura/caps_lock_handler.cc:40: =
chromeos::SystemKeyEventListener::GetInstance();
nit: move equals sign to previous line

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/ui/v...
File chrome/browser/ui/views/aura/caps_lock_handler.h (right):

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/ui/v...
chrome/browser/ui/views/aura/caps_lock_handler.h:17: class CapsLockHandler
nit: add a brief comment describing what this class does

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/ui/v...
chrome/browser/ui/views/aura/caps_lock_handler.h:37: void
set_is_running_on_chromeos(bool is_running_on_chromeos) {
nit: add "_for_test" at the end of this method's name and caps_lock_is_on()'s; i
believe that there's a presubmit check that will then make sure that these are
only called from tests.

you can also then just make these public and not need a derived class in the
test.

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/ui/v...
File chrome/browser/ui/views/aura/caps_lock_handler_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9225004/diff/10001/chrome/browser/ui/v...
chrome/browser/ui/views/aura/caps_lock_handler_browsertest.cc:66:
manager_->GetXKeyboard()->SetCapsLockEnabled(initial_caps_lock_state_);
i wish that this were mocked out.  as is, can we end up leaving the caps lock
state modified on the machine where the test is running?  what if the developer
is composing an email message while the test is running?

[Yusuke Sato, 2012-01-23 06:53:49]

Fixed. Please take another look.

(Currently the linux_chromeos_aura trybot is red since
http://codereview.chromium.org/9264004/ is not submitted yet. I'll rerun the bot
once the CL is committed.)

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener.cc:344: // When Aura is in
use, the short cut is handled in ash/.
On 2012/01/18 15:44:31, Daniel Erat wrote:
> nit: either just say "Ash" instead of "ash/" or mention the specific class
that
> handles it

Done.

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_key_event_listener_browsertest.cc (right):

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener_browsertest.cc:31: // When
Aura is in use, the short cut is handled in ash/.
On 2012/01/18 15:44:31, Daniel Erat wrote:
> nit: same as last comment

Done.

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/caps_lock_handler.cc (right):

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.cc:19: is_running_on_chromeos_(
On 2012/01/18 15:44:31, Daniel Erat wrote:
> nit: the lines in the initializer list should be indented two more spaces

Done.

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.cc:29: =
chromeos::SystemKeyEventListener::GetInstance();
On 2012/01/18 15:44:31, Daniel Erat wrote:
> nit: equals sign should be at end of previous line

Done.

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.cc:30: // SystemKeyEventListener
is instantiated only when running on Chrome OS.
On 2012/01/18 15:44:31, Daniel Erat wrote:
> this comment doesn't exactly match the check.  should it instead be
> "SystemKeyEventListener should be instantiated when we're running on Chrome
> OS."?

Done.

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.cc:31:
DCHECK((!is_running_on_chromeos_) || system_event_listener);
On 2012/01/18 15:44:31, Daniel Erat wrote:
> nit: remove extra parentheses around !is_running_on_chromeos_

Done.

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.cc:40: =
chromeos::SystemKeyEventListener::GetInstance();
On 2012/01/18 15:44:31, Daniel Erat wrote:
> nit: move equals sign to previous line

Done.

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/caps_lock_handler.h (right):

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.h:17: class CapsLockHandler
On 2012/01/18 15:44:31, Daniel Erat wrote:
> nit: add a brief comment describing what this class does

Done.

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.h:37: void
set_is_running_on_chromeos(bool is_running_on_chromeos) {
On 2012/01/18 15:44:31, Daniel Erat wrote:
> nit: add "_for_test" at the end of this method's name and caps_lock_is_on()'s;
i
> believe that there's a presubmit check that will then make sure that these are
> only called from tests.
> 
> you can also then just make these public and not need a derived class in the
> test.

Thanks. Removed the derived class as well.

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/caps_lock_handler_browsertest.cc (right):

http://codereview.chromium.org/9225004/diff/10001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler_browsertest.cc:66:
manager_->GetXKeyboard()->SetCapsLockEnabled(initial_caps_lock_state_);
On 2012/01/18 15:44:31, Daniel Erat wrote:
> i wish that this were mocked out.  as is, can we end up leaving the caps lock
> state modified on the machine where the test is running?  what if the
developer
> is composing an email message while the test is running?

Done.

[Daniel Erat, 2012-01-23 16:42:41]

As a larger question, I'm curious about whether this code even needs to be built
outside of Chrome OS.  Do you know whether caps-lock-toggling is planned on
other platforms?  It seems unlikely.

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener.cc:344: // When Aura is in
use, the short cut is handled in Ash.
nit: s/short cut/shortcut/

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/caps_lock_handler.cc (right):

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.cc:24:
chromeos::system::runtime_environment::IsRunningOnChromeOS()),
nit: indent this four spaces beyond the previous line instead of two

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.cc:26: {
nit: opening brace should be at the end of the previous line

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.cc:63: caps_lock_is_on_ =
enabled;
it seems weird that this variable is defined on non-chrome-os platforms but only
gets updated on chrome os.

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/caps_lock_handler.h (right):

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.h:23: // A class which toggles
Caps Lock state when the hot key for Caps Lock
nit: s/hot key/hotkey/

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.h:26: : public
ash::CapsLockDelegate
this probably should be on the previous line, like:

class CapsLockHandler : public ash::CapsLockDelegate
#if defined(OS_CHROMEOS)
                      , public
chromeos::SystemKeyEventListener::CapsLockObserver
#endif

otherwise, it looks like it's part of an initialization list.  i'm not sure how
to wrap the CapsLockObserver line, though.  maybe it's okay to leave it like
this.

[Yusuke Sato, 2012-01-24 09:03:19]

Fixed. Please take another look.

> As a larger question, I'm curious about whether this code even needs to be
built outside of Chrome OS.  Do you
> know whether caps-lock-toggling is planned on other platforms?

I made the new files in chrome/browser/ui/views/aura/ OS neutral primarily for
Ash on Windows.  If I remember correctly, last month Ben told me that Ash on
Windows should have as many features of Chromebook as possible (for example, Ash
on Windows should provide the same set of keyboard layouts and input method
editors as Chromebook). So I guess on Ash Shift+WinL should toggle Caps Lock
even on Windows.

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener.cc:344: // When Aura is in
use, the short cut is handled in Ash.
On 2012/01/23 16:42:43, Daniel Erat wrote:
> nit: s/short cut/shortcut/

Done.

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/caps_lock_handler.cc (right):

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.cc:24:
chromeos::system::runtime_environment::IsRunningOnChromeOS()),
On 2012/01/23 16:42:43, Daniel Erat wrote:
> nit: indent this four spaces beyond the previous line instead of two

Done.

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.cc:26: {
On 2012/01/23 16:42:43, Daniel Erat wrote:
> nit: opening brace should be at the end of the previous line

Done.

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.cc:63: caps_lock_is_on_ =
enabled;
On 2012/01/23 16:42:43, Daniel Erat wrote:
> it seems weird that this variable is defined on non-chrome-os platforms but
only
> gets updated on chrome os.

Done. Removed all member variables from !OS_CHROMEOS builds.

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
File chrome/browser/ui/views/aura/caps_lock_handler.h (right):

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.h:23: // A class which toggles
Caps Lock state when the hot key for Caps Lock
On 2012/01/23 16:42:43, Daniel Erat wrote:
> nit: s/hot key/hotkey/

Done.

http://codereview.chromium.org/9225004/diff/21001/chrome/browser/ui/views/aur...
chrome/browser/ui/views/aura/caps_lock_handler.h:26: : public
ash::CapsLockDelegate
On 2012/01/23 16:42:43, Daniel Erat wrote:
> this probably should be on the previous line, like:
> 
> class CapsLockHandler : public ash::CapsLockDelegate
> #if defined(OS_CHROMEOS)
>                       , public
> chromeos::SystemKeyEventListener::CapsLockObserver
> #endif
> 
> otherwise, it looks like it's part of an initialization list.  i'm not sure
how
> to wrap the CapsLockObserver line, though.  maybe it's okay to leave it like
> this.

Done.

[Daniel Erat, 2012-01-24 17:23:18]

Thanks for the explanation.  LGTM.

[commit-bot: I haz the power, 2012-01-24 22:59:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/9225004/35003

[commit-bot: I haz the power, 2012-01-24 22:59:27]

Presubmit check for 9225004-35003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/ui/views/aura/caps_lock_handler.cc,chrome/browser/ui/views/aura/caps_lock_handler.h,chrome/browser/ui/views/aura/caps_lock_handler_browsertest.cc,chrome/browser/ui/views/aura/screenshot_taker.h

Presubmit checks took 1.5s to calculate.

[Yusuke Sato, 2012-01-24 23:06:04]

Ben, could you do an OWNER review of this?

On 2012/01/24 22:59:27, I haz the power (commit-bot) wrote:
> Presubmit check for 9225004-35003 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for:
>
chrome/browser/ui/views/aura/caps_lock_handler.cc,chrome/browser/ui/views/aura/caps_lock_handler.h,chrome/browser/ui/views/aura/caps_lock_handler_browsertest.cc,chrome/browser/ui/views/aura/screenshot_taker.h
> 
> Presubmit checks took 1.5s to calculate.

[dmazzoni, 2012-01-27 19:55:40]

This patch didn't apply anymore and we want to land it today, so I created a new
change:

http://codereview.chromium.org/9297027

This change should be closed.

[Ben Goodger (Google), 2012-01-27 21:12:12]

lgtm

[Yusuke Sato, 2012-01-27 21:54:07]

Thanks Dominic for taking care of the change, but I also have a modified version
of the change locally (and I've just uploaded it). Since this CL already have an
approval from Ben, probably it's easier just to commit this CL.

I'll also land the a11y fix within today (Friday PST).

[Yusuke Sato, 2012-01-27 21:58:25]

On 2012/01/27 21:54:07, Yusuke Sato wrote:
> Thanks Dominic for taking care of the change, but I also have a modified
version
> of the change locally (and I've just uploaded it). Since this CL already have
an
> approval from Ben, probably it's easier just to commit this CL.
> 
> I'll also land the a11y fix within today (Friday PST).

(chatted with Dominic. submitting.)

[commit-bot: I haz the power, 2012-01-27 21:58:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yusukes@chromium.org/9225004/40001