Move functions for controlling Caps Lock to CapsLockDelegate from SystemTrayDelegate.

Move functions for controlling Caps Lock to CapsLockDelegate from SystemTrayDelegate.

- Add functions for controlling Caps Lock to CapsLockDelegate
- Move the ownership of CapsLockDelegate to Shell from AcceleratorController
- Add ShellDelegate::CreateCapsLockDelegate

BUG=144474
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153597

[mazda, 2012-08-25 00:43:43]

Please take a look at this change.

[tfarina, 2012-08-26 20:49:06]

https://chromiumcodereview.appspot.com/10878058/diff/7003/ash/caps_lock_deleg...
File ash/caps_lock_delegate.h (right):

https://chromiumcodereview.appspot.com/10878058/diff/7003/ash/caps_lock_deleg...
ash/caps_lock_delegate.h:18: virtual bool IsCapsLockEnabled() = 0;
can you make this const?

[Daniel Erat, 2012-08-26 22:32:46]

Thanks for cleaning this up!

https://chromiumcodereview.appspot.com/10878058/diff/7003/ash/caps_lock_deleg...
File ash/caps_lock_delegate_stub.cc (right):

https://chromiumcodereview.appspot.com/10878058/diff/7003/ash/caps_lock_deleg...
ash/caps_lock_delegate_stub.cc:10: : enabled_(false) {}
nit: indent this two more spaces

https://chromiumcodereview.appspot.com/10878058/diff/7003/ash/shell.cc
File ash/shell.cc (right):

https://chromiumcodereview.appspot.com/10878058/diff/7003/ash/shell.cc#newcode17
ash/shell.cc:17: #include "ash/focus_cycler.h"
nit: not your fault, but mind alphabetizing these?

https://chromiumcodereview.appspot.com/10878058/diff/7003/chrome/browser/ui/a...
File chrome/browser/ui/ash/caps_lock_handler.cc (right):

https://chromiumcodereview.appspot.com/10878058/diff/7003/chrome/browser/ui/a...
chrome/browser/ui/ash/caps_lock_handler.cc:44: return
xkeyboard_->CapsLockIsEnabled();
should you just be returning caps_lock_is_on_ here?

[mazda, 2012-08-27 16:06:40]

I addressed comments. Please take another look.

http://codereview.chromium.org/10878058/diff/7003/ash/caps_lock_delegate.h
File ash/caps_lock_delegate.h (right):

http://codereview.chromium.org/10878058/diff/7003/ash/caps_lock_delegate.h#ne...
ash/caps_lock_delegate.h:18: virtual bool IsCapsLockEnabled() = 0;
On 2012/08/26 20:49:07, tfarina wrote:
> can you make this const?

Done.

http://codereview.chromium.org/10878058/diff/7003/ash/caps_lock_delegate_stub.cc
File ash/caps_lock_delegate_stub.cc (right):

http://codereview.chromium.org/10878058/diff/7003/ash/caps_lock_delegate_stub...
ash/caps_lock_delegate_stub.cc:10: : enabled_(false) {}
On 2012/08/26 22:32:47, Daniel Erat wrote:
> nit: indent this two more spaces

Done.

http://codereview.chromium.org/10878058/diff/7003/ash/shell.cc
File ash/shell.cc (right):

http://codereview.chromium.org/10878058/diff/7003/ash/shell.cc#newcode17
ash/shell.cc:17: #include "ash/focus_cycler.h"
On 2012/08/26 22:32:47, Daniel Erat wrote:
> nit: not your fault, but mind alphabetizing these?

Done.

http://codereview.chromium.org/10878058/diff/7003/chrome/browser/ui/ash/caps_...
File chrome/browser/ui/ash/caps_lock_handler.cc (right):

http://codereview.chromium.org/10878058/diff/7003/chrome/browser/ui/ash/caps_...
chrome/browser/ui/ash/caps_lock_handler.cc:44: return
xkeyboard_->CapsLockIsEnabled();
On 2012/08/26 22:32:47, Daniel Erat wrote:
> should you just be returning caps_lock_is_on_ here?

It seems better. Thanks.

[Yusuke Sato, 2012-08-27 16:45:38]

http://codereview.chromium.org/10878058/diff/5028/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller_unittest.cc (right):

http://codereview.chromium.org/10878058/diff/5028/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller_unittest.cc:579: // CapsLock
What is this test for? Testing VKEY_[LR]?SHIFT for Caps Lock does not make sense
to me..

[mazda, 2012-08-27 16:58:58]

http://codereview.chromium.org/10878058/diff/5028/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller_unittest.cc (right):

http://codereview.chromium.org/10878058/diff/5028/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller_unittest.cc:579: // CapsLock
On 2012/08/27 16:45:39, Yusuke Sato wrote:
> What is this test for? Testing VKEY_[LR]?SHIFT for Caps Lock does not make
sense
> to me..

This block is testing DISABLE_CAPSLOCK and TOGGLE_CAPSLOCK actions.
Shift release was changed to disable Caps Lock. (http://crbug.com/138575).

I separated the brock into two blocks for clarity. Does this look better?

[Yusuke Sato, 2012-08-27 17:21:28]

ah ok, lgtm

On 2012/08/27 16:58:58, mazda wrote:
>
http://codereview.chromium.org/10878058/diff/5028/ash/accelerators/accelerato...
> File ash/accelerators/accelerator_controller_unittest.cc (right):
> 
>
http://codereview.chromium.org/10878058/diff/5028/ash/accelerators/accelerato...
> ash/accelerators/accelerator_controller_unittest.cc:579: // CapsLock
> On 2012/08/27 16:45:39, Yusuke Sato wrote:
> > What is this test for? Testing VKEY_[LR]?SHIFT for Caps Lock does not make
> sense
> > to me..
> 
> This block is testing DISABLE_CAPSLOCK and TOGGLE_CAPSLOCK actions.
> Shift release was changed to disable Caps Lock. (http://crbug.com/138575).
> 
> I separated the brock into two blocks for clarity. Does this look better?

[Daniel Erat, 2012-08-27 17:26:57]

http://codereview.chromium.org/10878058/diff/11005/ash/caps_lock_delegate.h
File ash/caps_lock_delegate.h (right):

http://codereview.chromium.org/10878058/diff/11005/ash/caps_lock_delegate.h#n...
ash/caps_lock_delegate.h:17: // Returns true if the caps lock is enabled.
nit: s/the caps lock/caps lock/

http://codereview.chromium.org/10878058/diff/11005/ash/caps_lock_delegate.h#n...
ash/caps_lock_delegate.h:21: virtual void SetCapsLockEnabled(bool enabled) = 0;
document that this method and ToggleCapsLock() toggle the state asynchronously,
and calls to IsCapsLockEnabled() just after these may return the old state?  or
is it safe to update CapsLockHandler::caps_lock_is_on_ from these methods so
that the new state is shown immediately?

[mazda, 2012-08-27 21:07:49]

http://codereview.chromium.org/10878058/diff/11005/ash/caps_lock_delegate.h
File ash/caps_lock_delegate.h (right):

http://codereview.chromium.org/10878058/diff/11005/ash/caps_lock_delegate.h#n...
ash/caps_lock_delegate.h:17: // Returns true if the caps lock is enabled.
On 2012/08/27 17:26:57, Daniel Erat wrote:
> nit: s/the caps lock/caps lock/

Done.

http://codereview.chromium.org/10878058/diff/11005/ash/caps_lock_delegate.h#n...
ash/caps_lock_delegate.h:21: virtual void SetCapsLockEnabled(bool enabled) = 0;
On 2012/08/27 17:26:57, Daniel Erat wrote:
> document that this method and ToggleCapsLock() toggle the state
asynchronously,
> and calls to IsCapsLockEnabled() just after these may return the old state? 
or
> is it safe to update CapsLockHandler::caps_lock_is_on_ from these methods so
> that the new state is shown immediately?

Currently pressing caps lock key is handled separately in SystemKeyEventListener
without calling these functions, so I think it's better not to set
caps_lock_is_on_ in the place other than OnCapsLockChagned to avoid the state
gets inconsistent state.
I added the note on the asynchronous state change.

[Daniel Erat, 2012-08-27 21:11:32]

lgtm

Thanks!

[commit-bot: I haz the power, 2012-08-27 21:15:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/10878058/7006

[commit-bot: I haz the power, 2012-08-27 21:16:09]

Failed to apply patch for chrome/browser/ui/ash/chrome_shell_delegate.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/ash/chrome_shell_delegate.cc
Hunk #2 FAILED at 41.
Hunk #3 succeeded at 323 (offset 1 line).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/ui/ash/chrome_shell_delegate.cc.rej

[commit-bot: I haz the power, 2012-08-27 21:40:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mazda@chromium.org/10878058/4028

[commit-bot: I haz the power, 2012-08-28 01:00:00]

Change committed as 153597