Add keyboard overlay help for the Search key as a Function key extended keyboard shortcuts.

Add keyboard overlay help for the Search key as a Function key extended keyboard shortcuts.

When the "Search key acts as a Function key" option is enabled, the keyboard
overlay help screen (accessed with Control-Alt-/) will include "Search" in its
list of modifiers to press to see shortcuts. And when Search is pressed, the
appropriate keys are highlighted with shortcuts that exist for it.

The shortcuts are: Search-Backspace, Search-Up, Search-Down, Search-1, Search-2,
etc, from https://codereview.chromium.org/11421055/

R=yusukes@chromium.org
BUG=162268
Depends on: https://codereview.chromium.org/11421055/
Depends on: https://codereview.chromium.org/11417144/


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170596

[Yusuke Sato, 2012-11-26 06:11:43]

Replacing myself with mazda@ (the author of the help overlay code) since I'm not
familiar with the code.

[danakj, 2012-11-27 23:26:54]

@mazda: Ping for review?

[mazda, 2012-11-28 05:25:17]

Sorry for the delay.

https://codereview.chromium.org/11415124/diff/5001/chrome/app/chromeos_string...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/11415124/diff/5001/chrome/app/chromeos_string...
chrome/app/chromeos_strings.grdp:2780: <message name="IDS_KEYBOARD_OVERLAY_F1"
desc="The text in the keyboard overlay to explain the shortcut.">
Could you move these strings above the line of <!-- BEGIN GENERATED KEYBOARD
OVERLAY STRINGS -->?
The strings below this marker comment are generated by a script.

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/keyboard_overlay.js (right):

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay.js:534: var
searchKeyIsModifier =
Please move this just before line 540.

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay.js:540: if
(searchKeyIsModifier)
if (searchKeyIsModifier) {
  ...
} else {
  ...
}

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/keyboard_overlay_data.js (right):

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay_data.js:15876:
'noSearchModifierShortcuts' : {
Could you keep this file intact as this fille is generated by a script?

For now, I propose the following way.
- Move these two maps to another file
- When search modifier is enabled, remove noSearchModifierShortcuts from the
original data and add searchModifierShortcuts in keyboard_overlay.js.

Please add TODO(mazda) somewhere to improve this.

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc (right):

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc:125: {
"keyboardOverlayF1", IDS_KEYBOARD_OVERLAY_F1 },
Please move these above "keyboardOverlayActivateLastLauncherItem" as this array
is generated by a script.

[danakj, 2012-11-28 21:11:23]

Thanks! PTAL

https://codereview.chromium.org/11415124/diff/5001/chrome/app/chromeos_string...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/11415124/diff/5001/chrome/app/chromeos_string...
chrome/app/chromeos_strings.grdp:2780: <message name="IDS_KEYBOARD_OVERLAY_F1"
desc="The text in the keyboard overlay to explain the shortcut.">
On 2012/11/28 05:25:18, mazda wrote:
> Could you move these strings above the line of <!-- BEGIN GENERATED KEYBOARD
> OVERLAY STRINGS -->?
> The strings below this marker comment are generated by a script.

Done.

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/keyboard_overlay.js (right):

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay.js:534: var
searchKeyIsModifier =
On 2012/11/28 05:25:18, mazda wrote:
> Please move this just before line 540.

Done.

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay.js:540: if
(searchKeyIsModifier)
On 2012/11/28 05:25:18, mazda wrote:
> if (searchKeyIsModifier) {
>   ...
> } else {
>   ...
> }

Done.

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/keyboard_overlay_data.js (right):

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay_data.js:15876:
'noSearchModifierShortcuts' : {
On 2012/11/28 05:25:18, mazda wrote:
> Could you keep this file intact as this fille is generated by a script?
> 
> For now, I propose the following way.
> - Move these two maps to another file
> - When search modifier is enabled, remove noSearchModifierShortcuts from the
> original data and add searchModifierShortcuts in keyboard_overlay.js.
> 
> Please add TODO(mazda) somewhere to improve this.

Done.

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc (right):

https://codereview.chromium.org/11415124/diff/5001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/keyboard_overlay_ui.cc:125: {
"keyboardOverlayF1", IDS_KEYBOARD_OVERLAY_F1 },
On 2012/11/28 05:25:18, mazda wrote:
> Please move these above "keyboardOverlayActivateLastLauncherItem" as this
array
> is generated by a script.

Done.

[mazda, 2012-11-28 21:23:09]

https://codereview.chromium.org/11415124/diff/9001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/keyboard_overlay.js (right):

https://codereview.chromium.org/11415124/diff/9001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay.js:165: return data;
I think it's better to cache the result because getShortcutData is used in
getKeyLabel.

[danakj, 2012-11-28 21:30:38]

PTAL

https://codereview.chromium.org/11415124/diff/9001/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/keyboard_overlay.js (right):

https://codereview.chromium.org/11415124/diff/9001/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay.js:165: return data;
On 2012/11/28 21:23:09, mazda wrote:
> I think it's better to cache the result because getShortcutData is used in
> getKeyLabel.

Ah, okay. Done!

[danakj, 2012-11-28 21:32:31]

Could you also recommend an appropriate OWNER for reviewing this once you're
happy with it? Thanks!

[mazda, 2012-11-28 22:08:25]

https://codereview.chromium.org/11415124/diff/6008/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/keyboard_overlay.js (right):

https://codereview.chromium.org/11415124/diff/6008/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay.js:132: var shortcutDataCache
= keyboardOverlayData['shortcut'];
var needs to be deleted.

[danakj, 2012-11-28 22:21:00]

Done.

https://codereview.chromium.org/11415124/diff/6008/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/keyboard_overlay.js (right):

https://codereview.chromium.org/11415124/diff/6008/chrome/browser/resources/c...
chrome/browser/resources/chromeos/keyboard_overlay.js:132: var shortcutDataCache
= keyboardOverlayData['shortcut'];
On 2012/11/28 22:08:25, mazda wrote:
> var needs to be deleted.

Oh! Thanks.

[mazda, 2012-11-28 22:24:28]

lgtm

+xiyuan for chromeos OWNERS review

[xiyuan, 2012-11-28 22:39:23]

rubber stamp LGTM

[commit-bot: I haz the power, 2012-11-30 20:38:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11415124/4005

[commit-bot: I haz the power, 2012-11-30 21:31:27]

Retried try job too often on win_aura for step(s) content_browsertests

[commit-bot: I haz the power, 2012-11-30 21:50:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11415124/4005

[commit-bot: I haz the power, 2012-11-30 23:37:18]

Change committed as 170596