Ignore volume key modifiers

Ignore volume key modifiers
The new way of handling volume keys using gdk_window_add_filter() (Issue 4098003) was allowing any modifier keys (Alt, Ctrl, Shift) as well, when only unmodified F8/F9/F10 keypresses should be responded to.

BUG=chromium-os:6074
TEST=Ctrl+, Alt+ or Shift+ F8/F9/F10 should not adjust volume anymore

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

[davejcool, 2010-11-13 01:57:00]

This one's pretty simple... Ctrl+, Alt+, and/or Shift+ volume key should not
adjust volume.

[Daniel Erat, 2010-11-13 03:16:10]

LGTM, although I'm not sure I understand how this fits in with issue
6074.  Are there plans to make the modified versions of these keys do
something else?  I guess it's consistent with F6 and F7, at least
(they don't appear to currently change the brightness on my L13 when I
have modifiers down).

On Fri, Nov 12, 2010 at 5:57 PM,  <davej@chromium.org> wrote:
> Reviewers: scherkus, glotov, Daniel Erat, tfarina,
>
> Message:
> This one's pretty simple... Ctrl+, Alt+, and/or Shift+ volume key should not
> adjust volume.
>
>
> Description:
> Ignore volume key modifiers
> The new way of handling volume keys using gdk_window_add_filter() (Issue
> 4098003) was allowing any modifier keys (Alt, Ctrl, Shift) as well, when
> only
> unmodified F8/F9/F10 keypresses should be responded to.
>
> BUG=chromium-os:6074
> TEST=Ctrl+, Alt+ or Shift+ F8/F9/F10 should not adjust volume anymore
>
>
> Please review this at http://codereview.chromium.org/4865001/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/chrome/browser/chromeos
>
> Affected files:
>  M chrome/browser/chromeos/system_key_event_listener.cc
>
>
> Index: chrome/browser/chromeos/system_key_event_listener.cc
> diff --git a/chrome/browser/chromeos/system_key_event_listener.cc
> b/chrome/browser/chromeos/system_key_event_listener.cc
> index
>
999f5bb55a6176e9a2ec5086fa8ec799079d7ed0..74af30822d2a310956a49919a4026994a4e75e26
> 100644
> --- a/chrome/browser/chromeos/system_key_event_listener.cc
> +++ b/chrome/browser/chromeos/system_key_event_listener.cc
> @@ -84,18 +84,21 @@ GdkFilterReturn
> SystemKeyEventListener::GdkEventFilter(GdkXEvent* gxevent,
>   if (xevent->type == KeyPress) {
>     int32 keycode = xevent->xkey.keycode;
>     if (keycode) {
> -      if ((keycode == listener->key_f8_) ||
> -          (keycode == listener->key_volume_mute_)) {
> -        listener->OnVolumeMute();
> -        return GDK_FILTER_REMOVE;
> -      } else if ((keycode == listener->key_f9_) ||
> -                  keycode == listener->key_volume_down_) {
> -        listener->OnVolumeDown();
> -        return GDK_FILTER_REMOVE;
> -      } else if ((keycode == listener->key_f10_) ||
> -                 (keycode == listener->key_volume_up_)) {
> -         listener->OnVolumeUp();
> -         return GDK_FILTER_REMOVE;
> +      // Only doing non-Alt/Shift/Ctrl modified keys
> +      if (!(xevent->xkey.state & (Mod1Mask | ShiftMask | ControlMask))) {
> +        if ((keycode == listener->key_f8_) ||
> +            (keycode == listener->key_volume_mute_)) {
> +          listener->OnVolumeMute();
> +          return GDK_FILTER_REMOVE;
> +        } else if ((keycode == listener->key_f9_) ||
> +                    keycode == listener->key_volume_down_) {
> +          listener->OnVolumeDown();
> +          return GDK_FILTER_REMOVE;
> +        } else if ((keycode == listener->key_f10_) ||
> +                   (keycode == listener->key_volume_up_)) {
> +          listener->OnVolumeUp();
> +          return GDK_FILTER_REMOVE;
> +        }
>       }
>     }
>   }
>
>
>

[davejcool, 2010-11-15 19:25:32]

Thanks.  Issue 6074 was getting brightness and volume keys working at the lock
screen.  Before this change, All key combinations for volume were consumed,
which was not consistent with F6/F7 behavior.  Plus, what if in the future an
extension or app might want to use one of these combinations?

[Daniel Erat, 2010-11-15 19:30:55]

On 2010/11/15 19:25:32, davejcool wrote:
> Thanks.  Issue 6074 was getting brightness and volume keys working at the lock
> screen.  Before this change, All key combinations for volume were consumed,
> which was not consistent with F6/F7 behavior.  Plus, what if in the future an
> extension or app might want to use one of these combinations?

Thanks for the explanation.  I'm skeptical that there will be any sites that
want to see Alt-F8 but not F8 by itself, though. :-P