Implement the final UI for the CAPS LOCK indicator (part 3 of 3)

Implement the final UI for the CAPS LOCK indicator (part 3 of 3)

Show the bubble UI (http://crosbug.com/11690#c18) when both Shift keys are pressed.

BUG=chromium-os:11690
TEST=ran browser_tests


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

[Zachary Kuznia, 2011-10-18 08:09:10]

http://codereview.chromium.org/8330016/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/caps_lock_menu_button.cc (right):

http://codereview.chromium.org/8330016/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/caps_lock_menu_button.cc:130: if
(!event.IsLeftMouseButton())
Is there a reason the right mouse button shouldn't close the bubble, too?

http://codereview.chromium.org/8330016/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/caps_lock_menu_button.cc:237: if (enabled &&
IsMenuShown())
Are multiple statements here expected to be true?  If not, I think it would be
more clear if these were else if blocks.

http://codereview.chromium.org/8330016/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/caps_lock_menu_button.cc:312: LOG(ERROR) << "The
bubble UI is already created";
Should this be NOTREACHED()?

http://codereview.chromium.org/8330016/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/caps_lock_menu_button.cc:330:
bubble_timer_.Stop(); // no-op if it's not running.
nit: Two spaces between ; and //

[Yusuke Sato, 2011-10-18 08:42:46]

http://codereview.chromium.org/8330016/diff/1/chrome/browser/chromeos/status/...
File chrome/browser/chromeos/status/caps_lock_menu_button.cc (right):

http://codereview.chromium.org/8330016/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/caps_lock_menu_button.cc:130: if
(!event.IsLeftMouseButton())
I chose the same behavior as PowerButton.

On 2011/10/18 08:09:10, Zachary Kuznia wrote:
> Is there a reason the right mouse button shouldn't close the bubble, too?

http://codereview.chromium.org/8330016/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/caps_lock_menu_button.cc:237: if (enabled &&
IsMenuShown())
On 2011/10/18 08:09:10, Zachary Kuznia wrote:
> Are multiple statements here expected to be true?  If not, I think it would be
> more clear if these were else if blocks.

Done.

http://codereview.chromium.org/8330016/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/caps_lock_menu_button.cc:312: LOG(ERROR) << "The
bubble UI is already created";
On 2011/10/18 08:09:10, Zachary Kuznia wrote:
> Should this be NOTREACHED()?

Done.

http://codereview.chromium.org/8330016/diff/1/chrome/browser/chromeos/status/...
chrome/browser/chromeos/status/caps_lock_menu_button.cc:330:
bubble_timer_.Stop(); // no-op if it's not running.
On 2011/10/18 08:09:10, Zachary Kuznia wrote:
> nit: Two spaces between ; and //

Done.

[Zachary Kuznia, 2011-10-19 09:23:34]

lgtm

[commit-bot: I haz the power, 2011-10-19 13:02:16]

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

[commit-bot: I haz the power, 2011-10-19 14:13:59]

Change committed as 106280