I think that shortcut was added, and an entry for it in the overlay was overlooked. Probably many other accelerators get added the same way, so +1 on establishing a process.

Idea: how about a command-line flag or internal url (like chrome://accelerators) that dumps all currently registered accelerators from ash, browser & elsewhere in a format that's easy to parse and compare against the keyboard overlay data file?

That would make it easy to detect mismatches.

Making sure the overlay gets updated should be part of UI review for any feature (as well as making sure any newly added combo will not cause conflicts). I've asked UI review folks to be more careful about this in the future.

Could we possibly add a test to enforce adding things to the overlay?  We can poke our review processes, but humans still make mistakes and miss things. Tests are more reliable.

+1 for an audit (though adding the test would accomplish the same thing)

I think this is a good task for Tao. I explained a bit about the history and the need for this, and briefly showed him parts of the code.

Tao, as per our discussion, please take a look at this. Please come to me whenever you have any question.

Uploaded a cl to add test to check future added cl is Search based:
https://codereview.chromium.org/2802583002/

For another test, Ensure that all accelerators are included in keyboard overlay, my proposal is the following. We might have some challenges to map the VKEY to characters.

1. Parse the keyboard_overlay_data.js to get the "shortcut" dictionary.
2. For each accelerator keycode (VKEY), map it to the character used in keyboard_overlay_data["shortcut"]. For example: VKEY_A to "a".
3. Map the modifier to "CTRL", "SHIFT", "ALT", "SEARCH".
4. Construct the string in the "shortcut": e.g. 'b<>ALT<>SHIFT', and check it exists in the "shortcut" dictionary.

+kpschoedel@ pointed out some corner cases for non-US keyboards and possible method to map (2) for US keyboard.

**Or we can check if the key (e.g. b<>ALT<>SHIFT') in "shortcut" can be easily resembled and mapped to the accelerator. Will implement what is easier.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/77744550580e6f83fa1ada19e8f96722b911640f

commit 77744550580e6f83fa1ada19e8f96722b911640f
Author: wutao <wutao@chromium.org>
Date: Fri Apr 07 04:55:37 2017

Add test to check search key based accelerators.

For new added accelerators, we want to ensure it is search key based by
adding the test.

BUG= 706018 , 706462 
TEST=AcceleratorTableTest.CheckSearchBasedAccelerators

Review-Url: https://codereview.chromium.org/2802583002
Cr-Commit-Position: refs/heads/master@{#462753}

[modify] https://crrev.com/77744550580e6f83fa1ada19e8f96722b911640f/ash/common/accelerators/accelerator_table_unittest.cc

There are several mappings scattered in .cc and .js files.
To parse the .js file and get the JSON dictionary seems work and not reliable.

I might write a browser test for "Ensure that all accelerators are included in keyboard overlay".

I fixed the following accelerators to make them have keyboard layout:
{true, ui::VKEY_MEDIA_LAUNCH_APP2, ui::EF_SHIFT_DOWN, TOGGLE_FULLSCREEN},
{true, ui::VKEY_P, ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN, SHOW_STYLUS_TOOLS}
{true, ui::VKEY_BROWSER_REFRESH, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN, ROTATE_WINDOW},
{true, ui::VKEY_I, ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN, TOUCH_HUD_MODE_CHANGE},

Add the following special mappings to keyboard_layout.js to make the test pass:
  152: 'power',
  166: 'back',
  167: 'forward',
  168: 'reload',
  173: 'mute',
  174: 'vol. down',
  175: 'vol. up',
  183: 'full screen',
  182: 'switch window',
  216: 'bright down',
  217: 'bright up',
  218: 'bright down',
  232: 'bright up',

The following VKEY no mapping, we might just skip the test when the modifier is "ui::EF_NONE"? Or just white list the followings?
No mapping:
 VKEY_KANA = 0x15, 21
 VKEY_CONVERT = 0x1C, 28
 VKEY_NONCONVERT = 0x1D, 29
 VKEY_WLAN = 0x97, 151
 VKEY_BROWSER_SEARCH = 0xAA, 170
 VKEY_DBE_SBCSCHAR = 0xF3,  // JIS DomKey::HANKAKU, 243
 VKEY_DBE_DBCSCHAR = 0xF4,  // JIS DomKey::ZENKAKU, 244
 VKEY_F13 = 0x7C, 124
 VKEY_RSHIFT = 0xA1, 161
 VKEY_PRINT = 0x2A, 42
 VKEY_HELP = 0x2F, 47
 VKEY_F14 = 0x7D, 7D
 VKEY_MEDIA_NEXT_TRACK = 0xB0, 176
 VKEY_MEDIA_PREV_TRACK = 0xB1, 177
 VKEY_MEDIA_PLAY_PAUSE = 0xB3, 179

Thanks!

I initially filed this due to the Alt+Shift+k accelerator not being in the overlay. I see that it's now been moved to kDeprecatedAccelerators in ash/common/accelerators/accelerator_table.cc, so I'm guessing that not including it in the overlay is intentional, right?

Regarding those other VKEYs, is there any way to just exclude them automatically since they aren't depicted on the overlay?

And just to point it out, we're probably always showing a Power key in the top right even though some devices (e.g. kevin and caroline) have Lock keys there instead.

#11, IIRC, from the cl when Alt+Shift+k was added, I do not see it was added to the keyboard overlay.

I can write another test to check the kDeprecatedAccelerators table, to make sure it should not exist in the keyboard overlay.

Currently this keyboard overlay entry is maintained manually and some new layout texts need to manually added at several places.

We might want to file another bug for automation of adding or deleting keyboard overlay?

Fixed one more:
{true, ui::VKEY_ESCAPE, ui::EF_SHIFT_DOWN | ui::EF_COMMAND_DOWN, UNPIN},


Not sure how to display this one: search<>ALT
{false, ui::VKEY_LWIN, ui::EF_ALT_DOWN, TOGGLE_CAPS_LOCK},

Uploaded a cl to add test to check all current accelerators having keyboard overlay.
https://codereview.chromium.org/2814003002

If reviewers are agree with this approach, I will add another test to check all deprecated accelerators should not have keyboard overlay.

Uploaded another cl of test to check the kDeprecatedAccelerators table, to make sure it should not exist in the keyboard overlay.
https://codereview.chromium.org/2815213003/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0bd2c53f52b94f64ac1521e3bdea8da54c870b4

commit a0bd2c53f52b94f64ac1521e3bdea8da54c870b4
Author: wutao <wutao@chromium.org>
Date: Thu Apr 13 17:36:39 2017

Add test to check deprecated accelerator do not have keyboard overlay.

When deprecate and replace an Accelerator, one step is to remove
it from the keyboard overlay. Writing a test to ensure it.

BUG= 706462 
R=xiyuan@chromium.org
TEST=KeyboardOverlayUIBrowserTest.*

Review-Url: https://codereview.chromium.org/2815213003
Cr-Commit-Position: refs/heads/master@{#464456}

[modify] https://crrev.com/a0bd2c53f52b94f64ac1521e3bdea8da54c870b4/chrome/browser/ui/webui/chromeos/keyboard_overlay_ui_browsertest.cc

I landed two tests. And will mark this as fixed.

1. Add test to check all current accelerators having keyboard overlay.
https://codereview.chromium.org/2814003002
2. Add test to check the deprecated accelerators do not have keyboard overlay.
https://codereview.chromium.org/2815213003/