Handle volume keys within Chrome

Volume Up/Down/Mute are handled within Chrome itself, without depending on Window Manager.  The SystemKeyEventListener keyboard filter is instantiated in screen_locker as well as browser_init, so these keys will be handled at the screen lock and when logged in.

The on-screen UI is shown when logged in, but currently is not being allowed to show at screen lock.  (A screen lock can be forced with /usr/bin/powerd_lock_screen).

After this patch, keyboard handling within Chrome supersedes the key handling in window_manager.cc, and so that functionality can safely be removed (http://codereview.chromium.org/4159004) and chromeos_wm_ipc_enums.h (http://codereview.chromium.org/4172004).  Related bug is http://crosbug.com/6074 .

BUG=chromium-os:6074
TEST=Volume Up/Down and Mute keys should work when screen is locked and when logged in normally.

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

[davejcool, 2010-10-27 01:52:14]

Hi,

I was looking at fixing this since currently if the audio was playing when you
shut the lid, when you open the lid there would be no way to turn down the sound
without logging in first.  There is no on-screen UI when adjusting the volume at
the lock screen though, but perhaps this would be a simple patch for Denis?

[Daniel Erat, 2010-10-27 06:27:08]

Thanks for doing this!

I think that the volume bubble not showing up when the screen is locked is a
result of the window manager only compositing windows with the screen locker
type (to guard against bugs where Chrome may accidentally map other windows). 
This is something that I can fix if something's added so I can identify the
volume bubble windows.  I'll file a bug against myself and give this some more
thought.

http://codereview.chromium.org/4098003/diff/10002/20003
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/4098003/diff/10002/20003#newcode25
chrome/browser/chromeos/system_key_event_listener.cc:25: instance =
Singleton<SystemKeyEventListener>::get();
what's the reason for this change?  is it just a performance thing?  do you need
to worry about thread-safety now?

http://codereview.chromium.org/4098003/diff/10002/20003#newcode48
chrome/browser/chromeos/system_key_event_listener.cc:48: void
SystemKeyEventListener::WillProcessEvent(GdkEvent* event) {
mind moving this and DidProcessEvent() so the definitions in this file match the
order in the header?

http://codereview.chromium.org/4098003/diff/10002/20004
File chrome/browser/chromeos/system_key_event_listener.h (right):

http://codereview.chromium.org/4098003/diff/10002/20004#newcode26
chrome/browser/chromeos/system_key_event_listener.h:26:
WmMessageListener::Observer {
looking at other chrome code, i think that this line should be indented more so
it lines up with the "public" on the previous line.  you should also repeat
"public" on this line unless you're really intending to switch to private
inheritance (in which case you should probably explicitly add "private")

[davejcool, 2010-10-27 18:52:55]

Did some formatting... and re-ordering to match header better.

http://codereview.chromium.org/4098003/diff/10002/20003
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/4098003/diff/10002/20003#newcode25
chrome/browser/chromeos/system_key_event_listener.cc:25: instance =
Singleton<SystemKeyEventListener>::get();
On 2010/10/27 06:27:08, Daniel Erat wrote:
> what's the reason for this change?  is it just a performance thing?  do you
need
> to worry about thread-safety now?

I've seen it done this way in a few other similar cases (WmIpc,
WmMessageListener, WmOverviewController) and in one I can't find right now, the
comment was that by saving the pointer in a static variable, Valgrind would
complain less on exit.

[Daniel Erat, 2010-10-27 19:00:55]

Thanks, LGTM.

http://codereview.chromium.org/4098003/diff/27001/28001
File chrome/browser/browser_init.cc (right):

http://codereview.chromium.org/4098003/diff/27001/28001#newcode406
chrome/browser/browser_init.cc:406: // messages regardless of focus
nit: add period at end of sentence

[scherkus (not reviewing), 2010-10-27 22:59:18]

LGTM

http://codereview.chromium.org/4098003/diff/31001/2004
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/4098003/diff/31001/2004#newcode84
chrome/browser/chromeos/system_key_event_listener.cc:84: // TODO(davej): This
fucntion will be removed once this message is removed
nit: fucntion -> function

[davejcool, 2010-10-30 00:25:59]

A keyboard filter is now inserted to grab the volume keys, instead of relying on
messages getting passed down through keyboard observers.  This simplifies things
overall, and can respond to volume keys even during full screen video, or when
in a shell.

[tfarina, 2010-10-30 00:31:27]

LGTM with the following changes.

http://codereview.chromium.org/4098003/diff/34001/35003
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/4098003/diff/34001/35003#newcode79
chrome/browser/chromeos/system_key_event_listener.cc:79: uint32 NumLockMask =
Mod2Mask;
nit: num_lock_mask

http://codereview.chromium.org/4098003/diff/34001/35003#newcode80
chrome/browser/chromeos/system_key_event_listener.cc:80: uint32 CapsLockMask =
LockMask;
nit: caps_lock_mask

http://codereview.chromium.org/4098003/diff/34001/35004
File chrome/browser/chromeos/system_key_event_listener.h (right):

http://codereview.chromium.org/4098003/diff/34001/35004#newcode33
chrome/browser/chromeos/system_key_event_listener.h:33: static GdkFilterReturn
gdk_event_filter(GdkXEvent* gxevent,
It seems you copied this from power_manager. But please, write this using
CamelCase for function names. Also I think you can write one argument per line
aligned with GdkXEvent

[Daniel Erat, 2010-10-30 16:13:50]

LGTM with Thiago's suggestions.

http://codereview.chromium.org/4098003/diff/34001/35003
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/4098003/diff/34001/35003#newcode65
chrome/browser/chromeos/system_key_event_listener.cc:65: } else if ((keycode ==
listener->key_f9_) ||
fix indenting here; I usually see people do something like:

} else if ((keycode == listener->key_fn_) ||
           (listener->key_volume_down_ &&
            keycode == listener->key_volume_down_)) {

[davejcool, 2010-11-01 20:22:29]

Thanks for looking this over.  I've updated the formatting, and think it's ready
to check in now.
-David.

http://codereview.chromium.org/4098003/diff/40001/41003
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/4098003/diff/40001/41003#newcode61
chrome/browser/chromeos/system_key_event_listener.cc:61: int32 keycode =
xevent->xkey.keycode;
I not only fixed formatting, but simplified by pulling out 'if (keycode)', so
rest is only checked if keycode != 0 (and thus if (listener->key_volume_X_ !=
0)).

[tfarina, 2010-11-01 20:29:21]

Some more nit below.

http://codereview.chromium.org/4098003/diff/40001/41003
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/4098003/diff/40001/41003#newcode7
chrome/browser/chromeos/system_key_event_listener.cc:7: #include
<X11/XF86keysym.h>
nit: sort alphabetical.

http://codereview.chromium.org/4098003/diff/40001/41003#newcode23
chrome/browser/chromeos/system_key_event_listener.cc:23: static
SystemKeyEventListener* instance = NULL;
nit: Is this change really needed?

http://codereview.chromium.org/4098003/diff/40001/41003#newcode40
chrome/browser/chromeos/system_key_event_listener.cc:40: if (key_volume_down_)
nit: else if? Same below

http://codereview.chromium.org/4098003/diff/40001/41003#newcode56
chrome/browser/chromeos/system_key_event_listener.cc:56: GdkEvent* gevent,
gpointer data) {
nit: could you write one argument per line aligned with GdkXEvent, if not
possible indent 4 spaces.

http://codereview.chromium.org/4098003/diff/40001/41003#newcode90
chrome/browser/chromeos/system_key_event_listener.cc:90: XGrabKey(GDK_DISPLAY(),
key, mask | caps_lock_mask | num_lock_mask, root, True,
nit: > 80 columns.

http://codereview.chromium.org/4098003/diff/40001/41004
File chrome/browser/chromeos/system_key_event_listener.h (right):

http://codereview.chromium.org/4098003/diff/40001/41004#newcode33
chrome/browser/chromeos/system_key_event_listener.h:33: static GdkFilterReturn
GdkEventFilter(GdkXEvent* gxevent,
Can you document this?

[davejcool, 2010-11-01 21:46:21]

Here's more nit fixes and responses, thanks again.
-David.

http://codereview.chromium.org/4098003/diff/40001/41003
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/4098003/diff/40001/41003#newcode23
chrome/browser/chromeos/system_key_event_listener.cc:23: static
SystemKeyEventListener* instance = NULL;
On 2010/11/01 20:29:21, tfarina wrote:
> nit: Is this change really needed?

I've seen it done this way in a few other similar cases (WmIpc,
WmMessageListener, WmOverviewController) and in browser_init.cc::429, there's a
comment about placing a Singleton into a global so it isn't reported as a leak.

If this truly isn't necessary, I can leave this bit alone,  but I wonder then
why is it being done this way elsewhere?

http://codereview.chromium.org/4098003/diff/40001/41003#newcode40
chrome/browser/chromeos/system_key_event_listener.cc:40: if (key_volume_down_)
On 2010/11/01 20:29:21, tfarina wrote:
> nit: else if? Same below

In this case, we want to call GrabKey() on each of these if they key codes are
found.

[tfarina, 2010-11-01 22:55:21]

My nits LGTM.

http://codereview.chromium.org/4098003/diff/40001/41003
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/4098003/diff/40001/41003#newcode23
chrome/browser/chromeos/system_key_event_listener.cc:23: static
SystemKeyEventListener* instance = NULL;
On 2010/11/01 21:46:21, davejcool wrote:
> On 2010/11/01 20:29:21, tfarina wrote:
> > nit: Is this change really needed?
> 
> I've seen it done this way in a few other similar cases (WmIpc,
> WmMessageListener, WmOverviewController) and in browser_init.cc::429, there's
a
> comment about placing a Singleton into a global so it isn't reported as a
leak.
> 
But I think here, the case is not the same as in there.

> If this truly isn't necessary, I can leave this bit alone,  but I wonder then
> why is it being done this way elsewhere?
> 
OK, leave it as is. But I don't see much the point of it. It is almost doing the
same thing of a non-thread-safe implementation of Singleton. It creates a
instance of |instance| and whenever its called again, the same object is
returned. I might be wrong here though. Greg Spencer might know better, since he
wrote the WmOverviewControlller in
http://codereview.chromium.org/661237/diff/289002/300001

[davejcool, 2010-11-02 00:50:30]

OK, I'm leaving ::instance() as it was after all.  So I'll check this in soon.

http://codereview.chromium.org/4098003/diff/40001/41003
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/4098003/diff/40001/41003#newcode23
chrome/browser/chromeos/system_key_event_listener.cc:23: static
SystemKeyEventListener* instance = NULL;
Greg said he didn't really need to do it in his case either, so I'm backing out
and leaving this as it was before.