Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(54)

Issue 11417144: Use rewriting to make ChromeOS keyboard F<number> keys produce extended keycodes. (Closed)

Created:
8 years, 1 month ago by danakj
Modified:
8 years ago
CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, derat+watch_chromium.org, ben+watch_chromium.org, tfarina, mazda+watch_chromium.org, piman, backer, WRONG-USE-chromium
Visibility:
Public.

Description

Use rewriting to make ChromeOS keyboard F<number> keys produce extended keycodes. This makes the ChromeOS keyboard F<number> keys produce keycodes such as BROWSER_BACK, VOLUME_UP, POWER, etc. We can then remove special-cases for ChromeOS accelerator bindings, and allow the same bindings to work on all platforms including ChromeOS. The ChromeOS keyboard, and external keyboards with these extra keys will also rely on and fire the same bindings for these keys, unifying the code path. The other advantage of this, is that now ChromeOS does not need to bind F<number> keys in a non-standard way. So, an external keyboard plugged into a Chromebook can still use the F<number> keys in the same way as they would when it was plugged into their desktop. This behaviour isn't yet possible, as the event rewritter is not aware if the event came from the ChromeOS keyboard or an external keyboard, but is a 1-line change once this information is known. Lastly, when the Search key acts as Function key option is enabled from https://codereview.chromium.org/11421055/ then Search-1 through Search-0 produce the keycode F1 through F10, and Search-- and Search-= produce F11 and F12. This allows applications to easily rely on these keys and consume them the same way on desktop as on a Chromebook. R=yusukes@chromium.org BUG=162268 TEST=unit_tests:EventRewriter.TestRewriteFunctionKeys Depends on: https://codereview.chromium.org/11421055/ Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170415

Patch Set 1 #

Total comments: 8

Patch Set 2 : Address Feedback #

Patch Set 3 : Rebase #

Total comments: 9

Patch Set 4 : NitFixin #

Patch Set 5 : UMA stuff #

Patch Set 6 : Detect Chromebook keyboard #

Total comments: 1

Patch Set 7 : chromeboxkeyboard #

Total comments: 10

Patch Set 8 : reviewed #

Patch Set 9 : nit #

Patch Set 10 : static const #

Total comments: 2

Patch Set 11 : Helper method do keysym mapping #

Total comments: 9

Patch Set 12 : for landing #

Patch Set 13 : forlanding2 #

Patch Set 14 : forlanding3 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1174 lines, -670 lines) Patch
M ash/accelerators/accelerator_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -3 lines 0 comments Download
M ash/accelerators/accelerator_controller_unittest.cc View 1 2 3 4 5 6 7 7 chunks +69 lines, -139 lines 0 comments Download
M ash/accelerators/accelerator_table.cc View 1 2 3 6 chunks +20 lines, -28 lines 0 comments Download
M ash/system/chromeos/keyboard_brightness_controller.cc View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/ash/brightness_controller_chromeos.cc View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/ash/chrome_shell_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/ash/event_rewriter.h View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +62 lines, -27 lines 0 comments Download
M chrome/browser/ui/ash/event_rewriter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 9 chunks +322 lines, -163 lines 0 comments Download
M chrome/browser/ui/ash/event_rewriter_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 6 chunks +667 lines, -280 lines 0 comments Download
M chrome/browser/ui/ash/volume_controller_chromeos.cc View 1 2 3 4 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/accelerator_table.cc View 1 chunk +14 lines, -18 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_view.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/ui/webui/options/chromeos/keyboard_handler.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 27 (0 generated)
danakj
8 years, 1 month ago (2012-11-23 04:52:00 UTC) #1
Yusuke Sato
https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_rewriter.cc File chrome/browser/ui/ash/event_rewriter.cc (right): https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_rewriter.cc#newcode764 chrome/browser/ui/ash/event_rewriter.cc:764: // On a ChromeOS keyboard, F<number> keys have special ...
8 years ago (2012-11-26 07:28:19 UTC) #2
danakj
https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_rewriter.cc File chrome/browser/ui/ash/event_rewriter.cc (right): https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_rewriter.cc#newcode764 chrome/browser/ui/ash/event_rewriter.cc:764: // On a ChromeOS keyboard, F<number> keys have special ...
8 years ago (2012-11-26 17:50:38 UTC) #3
Yusuke Sato
https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_rewriter.cc File chrome/browser/ui/ash/event_rewriter.cc (right): https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_rewriter.cc#newcode764 chrome/browser/ui/ash/event_rewriter.cc:764: // On a ChromeOS keyboard, F<number> keys have special ...
8 years ago (2012-11-26 18:59:59 UTC) #4
danakj
https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_rewriter.cc File chrome/browser/ui/ash/event_rewriter.cc (right): https://codereview.chromium.org/11417144/diff/1/chrome/browser/ui/ash/event_rewriter.cc#newcode764 chrome/browser/ui/ash/event_rewriter.cc:764: // On a ChromeOS keyboard, F<number> keys have special ...
8 years ago (2012-11-26 19:07:16 UTC) #5
danakj
I left the unit test for now, and am trying it out with a trybot. ...
8 years ago (2012-11-26 21:08:38 UTC) #6
Yusuke Sato
LGTM with some nits (but please wait for a comment from blumberg@). https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerator_table.cc File ash/accelerators/accelerator_table.cc ...
8 years ago (2012-11-27 01:56:53 UTC) #7
danakj
https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerator_table.cc File ash/accelerators/accelerator_table.cc (right): https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerator_table.cc#newcode1 ash/accelerators/accelerator_table.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights ...
8 years ago (2012-11-27 03:14:25 UTC) #8
Yusuke Sato
https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerator_table.cc File ash/accelerators/accelerator_table.cc (right): https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerator_table.cc#newcode1 ash/accelerators/accelerator_table.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights ...
8 years ago (2012-11-27 18:19:50 UTC) #9
Yusuke Sato
On 2012/11/27 18:19:50, Yusuke Sato wrote: > https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerator_table.cc > File ash/accelerators/accelerator_table.cc (right): > > https://codereview.chromium.org/11417144/diff/1008/ash/accelerators/accelerator_table.cc#newcode1 ...
8 years ago (2012-11-27 19:38:16 UTC) #10
Yusuke Sato
patch set 5 lgtm Can you ping blumberg@ and explain him about the old and ...
8 years ago (2012-11-27 22:16:58 UTC) #11
danakj
Hi Yusuke, PTAL. I've found a way to determine if the keyboard is the chromebook ...
8 years ago (2012-11-28 20:14:52 UTC) #12
Yusuke Sato
https://codereview.chromium.org/11417144/diff/9003/chrome/browser/ui/ash/event_rewriter.cc File chrome/browser/ui/ash/event_rewriter.cc (right): https://codereview.chromium.org/11417144/diff/9003/chrome/browser/ui/ash/event_rewriter.cc#newcode200 chrome/browser/ui/ash/event_rewriter.cc:200: if (LowerCaseEqualsASCII(device_name, "at translated set 2 keyboard")) can you ...
8 years ago (2012-11-28 20:39:17 UTC) #13
danakj
Done! PTAL
8 years ago (2012-11-28 21:15:11 UTC) #14
Yusuke Sato
On 2012/11/28 21:15:11, danakj wrote: > Done! PTAL lgtm, thanks.
8 years ago (2012-11-28 21:21:39 UTC) #15
danakj
+sky for OWNERS Sky, this is a followup to the last CL adding Search-bindings. This ...
8 years ago (2012-11-28 21:24:00 UTC) #16
danakj
+derat for event_rewriter
8 years ago (2012-11-28 21:24:46 UTC) #17
sky
LGTM
8 years ago (2012-11-28 22:15:35 UTC) #18
Daniel Erat
https://codereview.chromium.org/11417144/diff/10002/ash/accelerators/accelerator_table.cc File ash/accelerators/accelerator_table.cc (right): https://codereview.chromium.org/11417144/diff/10002/ash/accelerators/accelerator_table.cc#newcode47 ash/accelerators/accelerator_table.cc:47: { true, ui::VKEY_MEDIA_LAUNCH_APP2, ui::EF_CONTROL_DOWN, CYCLE_DISPLAY_MODE }, is there a ...
8 years ago (2012-11-28 22:23:03 UTC) #19
danakj
PTAL. @yusukes: I have guarded all the "Search key as modifier" places also on --has-chromeos-keyboard, ...
8 years ago (2012-11-29 01:50:54 UTC) #20
Daniel Erat
https://codereview.chromium.org/11417144/diff/16001/chrome/browser/ui/ash/event_rewriter.cc File chrome/browser/ui/ash/event_rewriter.cc (right): https://codereview.chromium.org/11417144/diff/16001/chrome/browser/ui/ash/event_rewriter.cc#newcode865 chrome/browser/ui/ash/event_rewriter.cc:865: { NativeKeySymToNativeKeycode(XK_1), 0, Mod4Mask, the keysym-to-keycode mappings can change ...
8 years ago (2012-11-29 04:12:27 UTC) #21
danakj
https://codereview.chromium.org/11417144/diff/16001/chrome/browser/ui/ash/event_rewriter.cc File chrome/browser/ui/ash/event_rewriter.cc (right): https://codereview.chromium.org/11417144/diff/16001/chrome/browser/ui/ash/event_rewriter.cc#newcode865 chrome/browser/ui/ash/event_rewriter.cc:865: { NativeKeySymToNativeKeycode(XK_1), 0, Mod4Mask, On 2012/11/29 04:12:27, Daniel Erat ...
8 years ago (2012-11-29 04:34:14 UTC) #22
Daniel Erat
LGTM w/comments https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/event_rewriter.cc File chrome/browser/ui/ash/event_rewriter.cc (right): https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/event_rewriter.cc#newcode286 chrome/browser/ui/ash/event_rewriter.cc:286: base::hash_map<KeySym, KeyCode>::iterator it; nit: delete this; it ...
8 years ago (2012-11-29 04:54:23 UTC) #23
danakj
Thanks! https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/event_rewriter.cc File chrome/browser/ui/ash/event_rewriter.cc (right): https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/event_rewriter.cc#newcode286 chrome/browser/ui/ash/event_rewriter.cc:286: base::hash_map<KeySym, KeyCode>::iterator it; On 2012/11/29 04:54:23, Daniel Erat ...
8 years ago (2012-11-30 03:07:40 UTC) #24
danakj
https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/event_rewriter.h File chrome/browser/ui/ash/event_rewriter.h (right): https://codereview.chromium.org/11417144/diff/11015/chrome/browser/ui/ash/event_rewriter.h#newcode105 chrome/browser/ui/ash/event_rewriter.h:105: typedef unsigned long KeySym; On 2012/11/30 03:07:40, danakj wrote: ...
8 years ago (2012-11-30 03:49:12 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danakj@chromium.org/11417144/4056
8 years ago (2012-11-30 04:07:36 UTC) #26
commit-bot: I haz the power
8 years ago (2012-11-30 07:27:54 UTC) #27
Message was sent while issue was closed.
Change committed as 170415

Powered by Google App Engine
This is Rietveld 408576698