Sound volume and bightness bubbles doesn't grab focus anymore.

Sound volume and bightness bubbles don't grab focus anymore.
Pressing the escape key to close such a bubble is handled on a higher level. So this key closes the bubble first and then closes other popups (such as a combo box popup).

BUG=chromium-os:16684
TEST=Open a page with popup element (translate bar popup element would suit). Open the popup. Press the "Volume up" button. Press Esc. Ensure the volume bubble has closed (not the popup). Open a search page. Put focus to the search box. Press the "Volume up" key. Ensure any keys except volume contol keys and Esc are handled by the search box.

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

[glotov, 2011-07-04 14:38:32]

http://codereview.chromium.org/7294024/diff/1/chrome/browser/chromeos/system_...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/7294024/diff/1/chrome/browser/chromeos/system_...
chrome/browser/chromeos/system_key_event_listener.cc:206: bool
SystemKeyEventListener::IsBubbleShown() const {
Shouldn't these methods be static?

[SeRya, 2011-07-05 08:48:02]

http://codereview.chromium.org/7294024/diff/1/chrome/browser/chromeos/system_...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/7294024/diff/1/chrome/browser/chromeos/system_...
chrome/browser/chromeos/system_key_event_listener.cc:206: bool
SystemKeyEventListener::IsBubbleShown() const {
On 2011/07/04 14:38:32, glotov wrote:
> Shouldn't these methods be static?

Done.

[glotov, 2011-07-05 11:10:27]

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/sett...
File chrome/browser/chromeos/setting_level_bubble.cc (right):

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/sett...
chrome/browser/chromeos/setting_level_bubble.cc:140: return bubble_;
Does bubble_->Close() zero bubble_ variable?
In other words, why, if the bubble is hidden, bubble_ is NULL?

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.cc:44: key_esc_ =
XKeysymToKeycode(GDK_DISPLAY(), XK_Escape);
I still cant understand: what is the connection between ESC handlers here and
accelerators in the Bubble class.

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system_key_event_listener.h (right):

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.h:84: static bool
IsBubbleShown();
Maybe a short comment would be good here: what bubble are you talking about and
why?

[SeRya, 2011-07-05 12:09:51]

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/sett...
File chrome/browser/chromeos/setting_level_bubble.cc (right):

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/sett...
chrome/browser/chromeos/setting_level_bubble.cc:140: return bubble_;
On 2011/07/05 11:10:27, glotov wrote:
> Does bubble_->Close() zero bubble_ variable?
> In other words, why, if the bubble is hidden, bubble_ is NULL?

Yes, SettingLevelBubble::BubbleClosing makes it NULL.

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.cc:44: key_esc_ =
XKeysymToKeycode(GDK_DISPLAY(), XK_Escape);
On 2011/07/05 11:10:27, glotov wrote:
> I still cant understand: what is the connection between ESC handlers here and
> accelerators in the Bubble class.

This handler replaces ESC accelerator. Reason for that is the following.
Accelerator relates to widget hierarchy and focus manager of the top level
widget. In contrast sound control buttons are handled globally not using visual
hierarchy. It looks looks logical for me to handle Esc in the same way.

It could be important in the following nonresistance. To use accelerator as
other bubbles does I need to choose a focus manager. It apparently should be the
focus manager that controls focus at time of opening the bubble. But if top
level widget has changed the bubble would stop handling this accelerator (but
continue to handle sound control keys).

Also it simplifies lifetime management of objects in two visual trees.

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system_key_event_listener.h (right):

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.h:84: static bool
IsBubbleShown();
On 2011/07/05 11:10:27, glotov wrote:
> Maybe a short comment would be good here: what bubble are you talking about
and
> why?

Done.

[glotov, 2011-07-05 13:23:46]

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.cc:44: key_esc_ =
XKeysymToKeycode(GDK_DISPLAY(), XK_Escape);
So, the accelerator is needed for the Light bubble only, right?

[SeRya, 2011-07-05 13:40:53]

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/7294024/diff/5001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.cc:44: key_esc_ =
XKeysymToKeycode(GDK_DISPLAY(), XK_Escape);
On 2011/07/05 13:23:47, glotov wrote:
> So, the accelerator is needed for the Light bubble only, right?

Both SettingLevelBubble's (VolumeBubble and BrightnessBubble) are handled here.
Other types of bubbles (I found PinnedContentsInfoBubble and FirstRunBubble)
should not be affected and still use accelerators.

[glotov, 2011-07-05 14:16:37]

LGTM with comment. And also please pass Trybots.

http://codereview.chromium.org/7294024/diff/9001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system_key_event_listener.h (right):

http://codereview.chromium.org/7294024/diff/9001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.h:84: // Tells if one of
system setting bubble (such as a sound volume bubble)
nit: we have only 2 bubbles (system setting, I mean). Moreover, your
implementation is tied to it. So why make the user think that it handles any
other bubbles?
The same below.

http://codereview.chromium.org/7294024/diff/9001/chrome/browser/ui/views/bubb...
File chrome/browser/ui/views/bubble/bubble.h (right):

http://codereview.chromium.org/7294024/diff/9001/chrome/browser/ui/views/bubb...
chrome/browser/ui/views/bubble/bubble.h:158: virtual void
RegisterEscapeAccelerator();
As discussed, please add a comment here that these acc-tors do not handle
Brightness and Volume bubbles, since they are handled on the
SystemKeyEventListener level.

[SeRya, 2011-07-05 15:35:19]

http://codereview.chromium.org/7294024/diff/9001/chrome/browser/chromeos/syst...
File chrome/browser/chromeos/system_key_event_listener.h (right):

http://codereview.chromium.org/7294024/diff/9001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.h:84: // Tells if one of
system setting bubble (such as a sound volume bubble)
On 2011/07/05 14:16:37, glotov wrote:
> nit: we have only 2 bubbles (system setting, I mean). Moreover, your
> implementation is tied to it. So why make the user think that it handles any
> other bubbles?
> The same below.

Done.

http://codereview.chromium.org/7294024/diff/9001/chrome/browser/ui/views/bubb...
File chrome/browser/ui/views/bubble/bubble.h (right):

http://codereview.chromium.org/7294024/diff/9001/chrome/browser/ui/views/bubb...
chrome/browser/ui/views/bubble/bubble.h:158: virtual void
RegisterEscapeAccelerator();
On 2011/07/05 14:16:37, glotov wrote:
> As discussed, please add a comment here that these acc-tors do not handle
> Brightness and Volume bubbles, since they are handled on the
> SystemKeyEventListener level.

Added comment to SettingLevelBubble::ShowBubble. In comment for ShowFocusless
mentioned that it doesn't set an accelerator.

[commit-bot: I haz the power, 2011-07-06 11:12:16]

Can't apply patch for file chrome/browser/chromeos/system_key_event_listener.h.
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/system_key_event_listener.h
Hunk #1 FAILED at 77.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/chromeos/system_key_event_listener.h.rej

[commit-bot: I haz the power, 2011-07-07 11:51:23]

Try job failure for 7294024-12004 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[Ben Goodger (Google), 2011-07-07 14:52:51]

LGTM for the bubble bits.

[Daniel Erat, 2011-07-12 15:04:58]

http://codereview.chromium.org/7294024/diff/12004/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_key_event_listener.h (right):

http://codereview.chromium.org/7294024/diff/12004/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener.h:94: static bool
IsBubbleShown();
This is already checked in, but for future changes, please make sure you're
following the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order