Use observer for an X message pump to intercept keyevents for volume control.

Use observer for an X message pump to intercept keyevents for volume control.

BUG=none
TEST=Pressed F8/F9/F10 keys on chromeos builds with and without touchui on the desktop and saw the volume-control bubble show up.

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

[sadrul, 2011-05-11 20:53:50]

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

http://codereview.chromium.org/7011005/diff/1001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.h:67: OVERRIDE
This is kind of ugly.

[sadrul, 2011-05-16 18:40:44]

+derat

[Daniel Erat, 2011-05-16 18:46:36]

Please describe what you did to test the change in the TEST= section of your
description, e.g. "manual: made sure that volume keys work in both old and new
code paths".

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

http://codereview.chromium.org/7011005/diff/1001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.cc:53:
MessageLoopForUI::current()->AddObserver(this);
do you need to remove the observer in Stop()?

http://codereview.chromium.org/7011005/diff/1001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.cc:162: if ((keycode ==
key_f8_) ||
This was broken before this change, but I figure it's worth pointing out: X's
keycode-to-keysym mapping can be changed at runtime; a MappingNotify event is
sent to all clients in this case.  The right thing to do then is to ungrab any
keycodes that are no longer used and grab new ones.  It's probably not a concern
for these particular keys, since it's unlikely that the volume or function keys
would be remapped.

[sadrul, 2011-05-16 19:58:05]

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

http://codereview.chromium.org/7011005/diff/1001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.cc:53:
MessageLoopForUI::current()->AddObserver(this);
On 2011/05/16 18:46:36, Daniel Erat wrote:
> do you need to remove the observer in Stop()?

Aha, yes! Done.

http://codereview.chromium.org/7011005/diff/1001/chrome/browser/chromeos/syst...
chrome/browser/chromeos/system_key_event_listener.cc:162: if ((keycode ==
key_f8_) ||
On 2011/05/16 18:46:36, Daniel Erat wrote:
> This was broken before this change, but I figure it's worth pointing out: X's
> keycode-to-keysym mapping can be changed at runtime; a MappingNotify event is
> sent to all clients in this case.  The right thing to do then is to ungrab any
> keycodes that are no longer used and grab new ones.  It's probably not a
concern
> for these particular keys, since it's unlikely that the volume or function
keys
> would be remapped.

Should this change be in a separate CL?

[Daniel Erat, 2011-05-16 21:47:20]

LGTM

On 2011/05/16 19:58:05, sadrul wrote:
> On 2011/05/16 18:46:36, Daniel Erat wrote:
> > This was broken before this change, but I figure it's worth pointing out:
X's
> > keycode-to-keysym mapping can be changed at runtime; a MappingNotify event
is
> > sent to all clients in this case.  The right thing to do then is to ungrab
any
> > keycodes that are no longer used and grab new ones.  It's probably not a
> concern
> > for these particular keys, since it's unlikely that the volume or function
> keys
> > would be remapped.
> 
> Should this change be in a separate CL?

Yeah, it shouldn't happen in this one -- maybe just add a TODO now and/or file a
bug?