Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
442333002:
Fixed odd behavior for keyboards using the AL_CONSUMER_CONTROL_CONFIG (Closed)
DescriptionFixed odd behavior for keyboards using the AL_CONSUMER_CONTROL_CONFIG
This usage was mapped to XF86XK_Tools and remapped to VKEY_F13 on
chrome, which is used for LOCK (logout) causing confusion for some
users. This usage is supported on Windows and Android to launch the
default media player, therefore it is now mapped to
VKEY_MEDIA_LAUNCH_MEDIA_SELECT.
R=garykac@chromium.org, wez@chromium.org, adlr@chromium.org
BUG=398345
Committed: https://crrev.com/6b7305748f00a08fc37b6fb86eba4e333c321afd
Cr-Commit-Position: refs/heads/master@{#295584}
Patch Set 1 #
Total comments: 6
Patch Set 2 : edited commit following Wez comments #
Total comments: 3
Patch Set 3 : update commented text upon request from wez #
Total comments: 6
Patch Set 4 : corrected comment to crbug.com/yyy #Messages
Total messages: 25 (3 generated)
As per discussion initiated beginning of may with andrew.
On 2014/08/06 13:16:15, mmeisser wrote: > As per discussion initiated beginning of may with andrew. Note: mapping to VKEY_MEDIA_LAUNCH_MEDIA_SELECT is a placeholder for now. The idea it that this will be functional as soon as will be implemented a logic allowing VKEYs to launch generic apps.
https://codereview.chromium.org/442333002/diff/1/ui/events/keycodes/keyboard_... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/442333002/diff/1/ui/events/keycodes/keyboard_... ui/events/keycodes/keyboard_code_conversion_x.cc:763: // When evdev is in use, /usr/share/X11/xkb/symbols/inet maps F13-18 keys nit: Update this comment. https://codereview.chromium.org/442333002/diff/1/ui/events/keycodes/keyboard_... ui/events/keycodes/keyboard_code_conversion_x.cc:779: // this was mapped to VKEY_F13 (see above), but this was causing odd nit: Capitalization. https://codereview.chromium.org/442333002/diff/1/ui/events/keycodes/keyboard_... ui/events/keycodes/keyboard_code_conversion_x.cc:782: // (see crbug=398345) nit: Rather than describe the bug, describe the logic behind mapping "Tools" to "Select Media", i.e. why this mapping? https://codereview.chromium.org/442333002/diff/1/ui/events/keycodes/keyboard_... ui/events/keycodes/keyboard_code_conversion_x.cc:783: case XF86XK_Tools: Can you move this to the appropriate order in this switch, i.e. near to whatever the closest other XK values handled are?
https://codereview.chromium.org/442333002/diff/1/ui/events/keycodes/keyboard_... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/442333002/diff/1/ui/events/keycodes/keyboard_... ui/events/keycodes/keyboard_code_conversion_x.cc:782: // (see crbug=398345) On 2014/08/06 21:17:53, Wez wrote: > nit: Rather than describe the bug, describe the logic behind mapping "Tools" to > "Select Media", i.e. why this mapping? I will describe the mapping. This seems odd, but already the XK86XK_Tools is a weird naming for AL_CONSUMER_CONTROL_CONFIG (0x0C 0x0183) in the first place. The second weird thing is that this is used in Windows and Android in order to launch a media player app. Multiple keyboards in the field (not only logitech's) are using this usage on a hotkey with a music icon. https://codereview.chromium.org/442333002/diff/1/ui/events/keycodes/keyboard_... ui/events/keycodes/keyboard_code_conversion_x.cc:783: case XF86XK_Tools: On 2014/08/06 21:17:53, Wez wrote: > Can you move this to the appropriate order in this switch, i.e. near to whatever > the closest other XK values handled are? The actual list of XK mappings are not really ordered in terms of values, rather on meaning/features. But I guess putting this closer to launchA & co makes sense since those are the closer values and do belong to the same feature-set (ie launch an app).
We could also propose to the HID HUT, to actually rename this usage that had been "unfortunately" used as launch media player in multiple OSes in field. It is also very possible that the original intent of the usage was never actually used (at least we know it was not used with the original intent in Windows and Android).
Wez, shall we proceed with this one, since the AL_SLEEP will not be mapped to VKEY_F13 anymore (as it makes more sense to map it to VK_SLEEP), so it does make less sense to merge this CL to the other one (AL_SLEEP), right ? In fact this one is more of a side effect we would want to avoid now, until the "launch music app" can be integrated. Do you agree ?
https://codereview.chromium.org/442333002/diff/20001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/442333002/diff/20001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:818: // media player app in most common OS (see crbug=398345) Suggest "is mapped from" -> "is generated by" Suggest "HID usage ... ( ... usage 0x0183)" -> "HID Usage AL_... (Usage 0x0183, Page 0x0C)" Suggest ".. and most commonly launches the OS default media player. See crbug.com/398345." https://codereview.chromium.org/442333002/diff/20001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:819: // map it now to VKEY_MEDIA_LAUNCH_MEDIA_SELECT which can be used to What VKEY does that USB usage generate under Windows, for a normal Windows app, or in Chrome or IE? https://codereview.chromium.org/442333002/diff/20001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:820: // later launch a media player app. Remove this?
> https://codereview.chromium.org/442333002/diff/20001/ui/events/keycodes/keybo... > ui/events/keycodes/keyboard_code_conversion_x.cc:819: // map it now to > VKEY_MEDIA_LAUNCH_MEDIA_SELECT which can be used to > What VKEY does that USB usage generate under Windows, for a normal Windows app, > or in Chrome or IE? The behaviour I observed under Windows is that this usage is directly consumed by the OS and is launching the default media player app (ex: WMP or VLC or else) > https://codereview.chromium.org/442333002/diff/20001/ui/events/keycodes/keybo... > ui/events/keycodes/keyboard_code_conversion_x.cc:820: // later launch a media > player app. > Remove this? I'm not sure you meant "remove the mapping" but I would rather keep this since it is generic enough and we could be able to use this mapping (VKEY_MEDIA_LAUNCH_MEDIA_SELECT) in the future by adding the corresponding accelerator when available.
On 29 August 2014 07:59, <mmeisser@logitech.com> wrote: > > https://codereview.chromium.org/442333002/diff/20001/ui/ > events/keycodes/keyboard_code_conversion_x.cc#newcode819 > >> ui/events/keycodes/keyboard_code_conversion_x.cc:819: // map it now to >> VKEY_MEDIA_LAUNCH_MEDIA_SELECT which can be used to >> What VKEY does that USB usage generate under Windows, for a normal Windows >> > app, > >> or in Chrome or IE? >> > > The behaviour I observed under Windows is that this usage is directly > consumed > by the > OS and is launching the default media player app (ex: WMP or VLC or else) I mean what VKEY does Windows generate internally (whether or not it the shell them swallows it). You can determine that by e.g. running an app that installs a low-level key event hook, which will receive the key event before it is processed as a hot-key. The point of doing that is to match the coding that Windows uses, so that we don't later discover that Windows uses VKEY_MEDIA_LAUNCH_MEDIA_SELECT for some other purpose, for example. > https://codereview.chromium.org/442333002/diff/20001/ui/ > events/keycodes/keyboard_code_conversion_x.cc#newcode820 > >> ui/events/keycodes/keyboard_code_conversion_x.cc:820: // later launch a >> media >> player app. >> Remove this? >> > > I'm not sure you meant "remove the mapping" but I would rather keep this > since > it > is generic enough and we could be able to use this mapping > (VKEY_MEDIA_LAUNCH_MEDIA_SELECT) > in the future by adding the corresponding accelerator when available. > No, I just meant removing the end of the comment about "later having this launch the media player", since this code is shared between Chrome on the desktop, and ChromeOS/Ash, and under the former it's not Chrome's responsibility to implement shell hot-keys. > > > https://codereview.chromium.org/442333002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/29 18:48:42, Wez wrote: > > I mean what VKEY does Windows generate internally (whether or not it the > shell them swallows it). You can determine that by e.g. running an app that > installs a low-level key event hook, which will receive the key event > before it is processed as a hot-key. The point of doing that is to match > the coding that Windows uses, so that we don't later discover that Windows > uses VKEY_MEDIA_LAUNCH_MEDIA_SELECT for some other purpose, for example. Wez, I checked using this (link: http://www.mediaevent.de/javascript/Extras-Javascript-Keycodes.html) to see what the Canary browser is getting on Windows when we hit the AL_CONSUMER_CONTROL_CONFIG key on our keyboards. The browser gets AL_CONSUMER_CONTROL_CONFIG -> 181(dec)/0xB5 while Windows launches independently the Media player. Therefore I think it is safe to use this mapping. would you agree ? as well as (other examples): AL_LAUNCH_EMAIL -> 180(dec)/0xB4 (windows launches the email client) AL_CALCULATOR -> 183(dec)/0xB7 (windows launches the calculator) > No, I just meant removing the end of the comment about "later having this > launch the media player", since this code is shared between Chrome on the > desktop, and ChromeOS/Ash, and under the former it's not Chrome's > responsibility to implement shell hot-keys. Understood.I removed that from the comment and left the mapping. Is that ok for you ?
On 2014/09/01 16:00:22, mmeisser wrote: > On 2014/08/29 18:48:42, Wez wrote: > > > > I mean what VKEY does Windows generate internally (whether or not it the > > shell them swallows it). You can determine that by e.g. running an app that > > installs a low-level key event hook, which will receive the key event > > before it is processed as a hot-key. The point of doing that is to match > > the coding that Windows uses, so that we don't later discover that Windows > > uses VKEY_MEDIA_LAUNCH_MEDIA_SELECT for some other purpose, for example. > > Wez, I checked using this (link: > http://www.mediaevent.de/javascript/Extras-Javascript-Keycodes.html) to see what > the Canary browser is getting on Windows when we hit the > AL_CONSUMER_CONTROL_CONFIG key on our keyboards. The browser gets > AL_CONSUMER_CONTROL_CONFIG -> 181(dec)/0xB5 while Windows launches > independently the Media player. OK, and VK_LAUNCH_MEDIA_SELECT == 0xB5; SGTM. > Therefore I think it is safe to use this mapping. would you agree ? > > > as well as (other examples): > AL_LAUNCH_EMAIL -> 180(dec)/0xB4 (windows launches the email client) VK_LAUNCH_MAIL == 0xB4 > AL_CALCULATOR -> 183(dec)/0xB7 (windows launches the calculator) VK_LAUNCH_APP2 == 0xB7 > > No, I just meant removing the end of the comment about "later having this > > launch the media player", since this code is shared between Chrome on the > > desktop, and ChromeOS/Ash, and under the former it's not Chrome's > > responsibility to implement shell hot-keys. > > Understood.I removed that from the comment and left the mapping. > > Is that ok for you ?
https://codereview.chromium.org/442333002/diff/40001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/442333002/diff/40001/AUTHORS#newcode264 AUTHORS:264: Mathieu Meisser <mmeisser@logitech.com> You'll need to accept the CLA - see http://www.chromium.org/developers/contributing-code/external-contributor-che... https://codereview.chromium.org/442333002/diff/40001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/442333002/diff/40001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:818: // media player (see crbug/398345). s/crbug/crbug.com
see my answers on Wez comments https://codereview.chromium.org/442333002/diff/40001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/442333002/diff/40001/AUTHORS#newcode264 AUTHORS:264: Mathieu Meisser <mmeisser@logitech.com> On 2014/09/02 17:27:18, Wez wrote: > You'll need to accept the CLA - see > http://www.chromium.org/developers/contributing-code/external-contributor-che... Yep. ongoing https://codereview.chromium.org/442333002/diff/40001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/442333002/diff/40001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:818: // media player (see crbug/398345). On 2014/09/02 17:27:18, Wez wrote: > http://s/crbug/crbug.com Should I change something here ?
https://codereview.chromium.org/442333002/diff/40001/AUTHORS File AUTHORS (right): https://codereview.chromium.org/442333002/diff/40001/AUTHORS#newcode264 AUTHORS:264: Mathieu Meisser <mmeisser@logitech.com> On 2014/09/09 14:20:52, mmeisser wrote: > On 2014/09/02 17:27:18, Wez wrote: > > You'll need to accept the CLA - see > > > http://www.chromium.org/developers/contributing-code/external-contributor-che... > > Yep. ongoing If Logitech accept the CLA as a corporate contributor then there should instead be an entry lower down like: Logitech <*@logitech.com> https://codereview.chromium.org/442333002/diff/40001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/442333002/diff/40001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:818: // media player (see crbug/398345). On 2014/09/09 14:20:52, mmeisser wrote: > On 2014/09/02 17:27:18, Wez wrote: > > http://s/crbug/crbug.com > > Should I change something here ? Yes, please update the comment to say "(see crbug.com/398345)".
> If Logitech accept the CLA as a corporate contributor then there should instead > be an entry lower down like: Logitech <mailto:*@logitech.com> Ok, or we can keep individual contributors listed in the AUTHORS file, right ? (me still waiting for legal on our side...) > Yes, please update the comment to say "(see crbug.com/398345)". Yes, I understood it now. I had it changed in the last patchset.
On 2014/09/10 08:28:32, mmeisser wrote: > > If Logitech accept the CLA as a corporate contributor then there should > instead > > be an entry lower down like: Logitech <mailto:*@logitech.com> > > Ok, or we can keep individual contributors listed in the AUTHORS file, right ? > (me still waiting for legal on our side...) No, if Logitech will be corporate signing then there should be a single entry for Logitech rather than one for each individual employee.
The CQ bit was checked by wez@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/442333002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by wez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/442333002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 0c304fe4e7d7ec9f40ef3fec559ac4b252d05504
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6b7305748f00a08fc37b6fb86eba4e333c321afd Cr-Commit-Position: refs/heads/master@{#295584} |
