mac: Create a static mapping of accelerators in the main menu.

mac: Create a static mapping of accelerators in the main menu.

This change allows for two new functionalities:
1. Compare a mac accelerator with an extension accelerator.
2. Determine whether an accelerator is a standard main-menu accelerator without
a dynamic query of the main menu.

Accelerators generated from the extension manifest use cross-platform
key_codes/modifiers.  Previously, there was no canonical way to get an
accelerator from the mac main menu. There was a mapping of accelerators from
the toolbar menu, but those accelerators only included mac key_codes/modifiers,
and not cross-platform key_codes/modifiers.

This CL creates a static mapping of all accelerators used in the main menu and
toolbar menu. The mapping includes the command_id, the cross-platform key_code,
and the Cocoa modifiers. The cross-platform modifiers and the Cocoa
keyEquivalent are generated on demand.

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

[erikchen, 2014-02-11 05:09:21]

https://codereview.chromium.org/152643007/diff/50001/ui/events/keycodes/keybo...
File ui/events/keycodes/keyboard_code_conversion_mac.mm (left):

https://codereview.chromium.org/152643007/diff/50001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion_mac.mm:524: // Control characters.
This code is not used in the current code base (All calls to
MacKeyCodeForWindowsKeyCode use flags = 0). 

I don't understand lines 526-527, despite having looked at the CL in which this
code was introduced. https://codereview.chromium.org/2986004. This code causes
my newly written code to malfunction, so I removed it.

[Robert Sesek, 2014-02-11 16:19:48]

Mostly style stuff. Nice test coverage!

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/accelerators_cocoa.h (right):

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.h:8: #import <Cocoa/Cocoa.h>
nit: blank line after, to separate C and C++ includes

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.h:42: const ui::Accelerator*
GetAcceleratorForHotKey(
Can this method be const, too?

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.h:43: NSString* keyEquivalent,
NSUInteger modifiers);
naming: key_equivalent (and in the comment).

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.h:63: AcceleratorVector
acceleratorVector_;
naming: accelerator_vector_

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/accelerators_cocoa.mm (right):

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:35: ui::KeyboardCode keyXP;  //
The key used for cross-platform compatibility.
"XP" is not a common term in Chromium, except if you're talking about a version
of Windows. I'd stick with the terms "key_code" for the cross-platform version
and "key_equivalent" for the Cocoa version.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:37: // Accelerators used in the
toolbar menu.
nit: the indent on this was correct before (only two spaces)

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:104: }  // Anonymous namespace.
Revert this to what it was. "// namespace" is the correct terminating comment
for an anonymous namespace.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:107: int
xpModifiersFromCocoaModifiers(NSUInteger modifiers) {
This should go in the namespae above, should be named with a capital letter, and
shouldn't use "xp" in the name.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:123: int xpModifiers =
xpModifiersFromCocoaModifiers(entry.modifiers);
naming: here and throughout xp -> something else

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:123: int xpModifiers =
xpModifiersFromCocoaModifiers(entry.modifiers);
I'm surprised we don't already have this function written somewhere.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:127: unichar
characterIgnoringModifiers;
naming: here and throughout, in C++ or C functions and classes, the rule is to
use under_scores for variable names. Only in ObjC classes do you use camelCase.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:176: for
(AcceleratorVector::iterator it = acceleratorVector_.begin();
const_iterator?

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:177: it !=
acceleratorVector_.end();
nit: align with 'A'

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:181: static_cast<const
ui::PlatformAcceleratorCocoa*>(
nit: continuations are indented 4 spaces

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm (right):

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
Happy new year x2

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:15: typedef
InProcessBrowserTest AcceleratorsCocoaBrowsertest;
Browsertest -> BrowserTest

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:20: void
addAcceleratorItemsToArray(NSMenu* menu, NSMutableArray* array) {
naming: AddAccelerator

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:24:
addAcceleratorItemsToArray(submenu, array);
nit: over-indented

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:33: NSMenuItem*
menuContainsAccelerator(NSMenu* menu,
naming: MenuContains

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:48: ((modifiers &
(~NSShiftKeyMask)) == item.keyEquivalentModifierMask);
You use [square brackets] above for keyEquivalent but dot.notation here. Be
consistent.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:56: }  // Anonymous
namespace.
"// namespace"

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:91: NSString*
cocoaKeyEquivalent = platform_accelerator->characters();
naming: under_scores

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:135: 
nit: extra blank line

https://codereview.chromium.org/152643007/diff/50001/ui/events/keycodes/keybo...
File ui/events/keycodes/keyboard_code_conversion_mac.h (right):

https://codereview.chromium.org/152643007/diff/50001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion_mac.h:19: // and corresponding
unicode character. |flags| is the cocoa modifiers mask
nit: Cocoa is a proper noun

[erikchen, 2014-02-11 19:42:26]

Thank you for the fast response time, and thorough review!

PTAL

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/accelerators_cocoa.h (right):

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.h:8: #import <Cocoa/Cocoa.h>
On 2014/02/11 16:19:48, rsesek wrote:
> nit: blank line after, to separate C and C++ includes

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.h:42: const ui::Accelerator*
GetAcceleratorForHotKey(
On 2014/02/11 16:19:48, rsesek wrote:
> Can this method be const, too?

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.h:43: NSString* keyEquivalent,
NSUInteger modifiers);
On 2014/02/11 16:19:48, rsesek wrote:
> naming: key_equivalent (and in the comment).

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.h:63: AcceleratorVector
acceleratorVector_;
On 2014/02/11 16:19:48, rsesek wrote:
> naming: accelerator_vector_

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/accelerators_cocoa.mm (right):

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:35: ui::KeyboardCode keyXP;  //
The key used for cross-platform compatibility.
On 2014/02/11 16:19:48, rsesek wrote:
> "XP" is not a common term in Chromium, except if you're talking about a
version
> of Windows. I'd stick with the terms "key_code" for the cross-platform version
> and "key_equivalent" for the Cocoa version.

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:37: // Accelerators used in the
toolbar menu.
This was the output of clang-format. I'm not sure why there are 6 spaces.
Reverted to 2.
On 2014/02/11 16:19:48, rsesek wrote:
> nit: the indent on this was correct before (only two spaces)

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:104: }  // Anonymous namespace.
On 2014/02/11 16:19:48, rsesek wrote:
> Revert this to what it was. "// namespace" is the correct terminating comment
> for an anonymous namespace.

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:107: int
xpModifiersFromCocoaModifiers(NSUInteger modifiers) {
On 2014/02/11 16:19:48, rsesek wrote:
> This should go in the namespae above, should be named with a capital letter,
and
> shouldn't use "xp" in the name.

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:123: int xpModifiers =
xpModifiersFromCocoaModifiers(entry.modifiers);
I've change xp -> x, as a shorthand for cross-platform. Let me know if you
prefer another naming.

On 2014/02/11 16:19:48, rsesek wrote:
> naming: here and throughout xp -> something else

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:123: int xpModifiers =
xpModifiersFromCocoaModifiers(entry.modifiers);
We have similar functions, but they all require NSEvents, and add additional
flags based on the NSEvent.

On 2014/02/11 16:19:48, rsesek wrote:
> I'm surprised we don't already have this function written somewhere.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:127: unichar
characterIgnoringModifiers;
ah. got it.

On 2014/02/11 16:19:48, rsesek wrote:
> naming: here and throughout, in C++ or C functions and classes, the rule is to
> use under_scores for variable names. Only in ObjC classes do you use
camelCase.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:176: for
(AcceleratorVector::iterator it = acceleratorVector_.begin();
On 2014/02/11 16:19:48, rsesek wrote:
> const_iterator?

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:177: it !=
acceleratorVector_.end();
On 2014/02/11 16:19:48, rsesek wrote:
> nit: align with 'A'

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:181: static_cast<const
ui::PlatformAcceleratorCocoa*>(
On 2014/02/11 16:19:48, rsesek wrote:
> nit: continuations are indented 4 spaces

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm (right):

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2014/02/11 16:19:48, rsesek wrote:
> Happy new year x2

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:15: typedef
InProcessBrowserTest AcceleratorsCocoaBrowsertest;
I copied the casing from another browser_test in the same folder. I've made the
change you've suggested.

On 2014/02/11 16:19:48, rsesek wrote:
> Browsertest -> BrowserTest

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:20: void
addAcceleratorItemsToArray(NSMenu* menu, NSMutableArray* array) {
On 2014/02/11 16:19:48, rsesek wrote:
> naming: AddAccelerator

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:24:
addAcceleratorItemsToArray(submenu, array);
On 2014/02/11 16:19:48, rsesek wrote:
> nit: over-indented

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:33: NSMenuItem*
menuContainsAccelerator(NSMenu* menu,
On 2014/02/11 16:19:48, rsesek wrote:
> naming: MenuContains

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:48: ((modifiers &
(~NSShiftKeyMask)) == item.keyEquivalentModifierMask);
changed keyEquivalent, keyEquivalentModifierMask, and submenu to use dot
notation, since the interfaces match those of properties. I've kept itemArray as
a method.

On 2014/02/11 16:19:48, rsesek wrote:
> You use [square brackets] above for keyEquivalent but dot.notation here. Be
> consistent.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:56: }  // Anonymous
namespace.
On 2014/02/11 16:19:48, rsesek wrote:
> "// namespace"

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:91: NSString*
cocoaKeyEquivalent = platform_accelerator->characters();
On 2014/02/11 16:19:48, rsesek wrote:
> naming: under_scores

Done.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:135: 
On 2014/02/11 16:19:48, rsesek wrote:
> nit: extra blank line

Done.

https://codereview.chromium.org/152643007/diff/50001/ui/events/keycodes/keybo...
File ui/events/keycodes/keyboard_code_conversion_mac.h (right):

https://codereview.chromium.org/152643007/diff/50001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion_mac.h:19: // and corresponding
unicode character. |flags| is the cocoa modifiers mask
On 2014/02/11 16:19:48, rsesek wrote:
> nit: Cocoa is a proper noun

Done.

[Robert Sesek, 2014-02-11 22:18:16]

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/accelerators_cocoa.mm (right):

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:123: int xpModifiers =
xpModifiersFromCocoaModifiers(entry.modifiers);
On 2014/02/11 19:42:27, erikchen1 wrote:
> We have similar functions, but they all require NSEvents, and add additional
> flags based on the NSEvent.
> 
> On 2014/02/11 16:19:48, rsesek wrote:
> > I'm surprised we don't already have this function written somewhere.
> 

OK. It may be handy to refactor out the flag-conversion logic into a function
that can be used here along with the other places (if possible, I haven't
checked).

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:123: int xpModifiers =
xpModifiersFromCocoaModifiers(entry.modifiers);
On 2014/02/11 19:42:27, erikchen1 wrote:
> I've change xp -> x, as a shorthand for cross-platform. Let me know if you
> prefer another naming.
> 
> On 2014/02/11 16:19:48, rsesek wrote:
> > naming: here and throughout xp -> something else
> 

What about ui_modifiers vs cocoa_modifiers? Those are the names of the two
libraries between which you are converting.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm (right):

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:20: void
addAcceleratorItemsToArray(NSMenu* menu, NSMutableArray* array) {
On 2014/02/11 19:42:27, erikchen1 wrote:
> On 2014/02/11 16:19:48, rsesek wrote:
> > naming: AddAccelerator
> 
> Done.

Argh, sorry I should have been more clear. I was just pointing out
capitalization. The name AddAcceleratorItemsToArray is good, same with below.

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/accelerators_cocoa.mm (right):

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:101: // Compute the cross-platform
modifier from the cocoa modifier.
nit: capitalize Cocoa

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:132: 
nit: extra blank line

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:137: for (size_t i = 0; i <
arraysize(kAcceleratorList); ++i) {
These two loops are awfully similar. Can they be condensed with a helper
function?

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:177: const
ui::PlatformAcceleratorCocoa* platform_accelerator =
This block is also a bit similar to the one above.

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm (right):

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:66: int command_id =
item.tag;
-tag returns NSInteger

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:93: // cocoa
accelerator map.
nit: Cocoa

[erikchen, 2014-02-11 23:16:28]

PTAL

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/accelerators_cocoa.mm (right):

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:123: int xpModifiers =
xpModifiersFromCocoaModifiers(entry.modifiers);
refactored completed.
On 2014/02/11 22:18:16, rsesek wrote:
> On 2014/02/11 19:42:27, erikchen1 wrote:
> > We have similar functions, but they all require NSEvents, and add additional
> > flags based on the NSEvent.
> > 
> > On 2014/02/11 16:19:48, rsesek wrote:
> > > I'm surprised we don't already have this function written somewhere.
> > 
> 
> OK. It may be handy to refactor out the flag-conversion logic into a function
> that can be used here along with the other places (if possible, I haven't
> checked).

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:123: int xpModifiers =
xpModifiersFromCocoaModifiers(entry.modifiers);
I found ui_modifiers to be confusing as well. I've decided to go with the full
name "cross_platform_modifiers"

On 2014/02/11 22:18:16, rsesek wrote:
> On 2014/02/11 19:42:27, erikchen1 wrote:
> > I've change xp -> x, as a shorthand for cross-platform. Let me know if you
> > prefer another naming.
> > 
> > On 2014/02/11 16:19:48, rsesek wrote:
> > > naming: here and throughout xp -> something else
> > 
> 
> What about ui_modifiers vs cocoa_modifiers? Those are the names of the two
> libraries between which you are converting.

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm (right):

https://codereview.chromium.org/152643007/diff/50001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:20: void
addAcceleratorItemsToArray(NSMenu* menu, NSMutableArray* array) {
On 2014/02/11 22:18:16, rsesek wrote:
> On 2014/02/11 19:42:27, erikchen1 wrote:
> > On 2014/02/11 16:19:48, rsesek wrote:
> > > naming: AddAccelerator
> > 
> > Done.
> 
> Argh, sorry I should have been more clear. I was just pointing out
> capitalization. The name AddAcceleratorItemsToArray is good, same with below.

Done.

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/accelerators_cocoa.mm (right):

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:101: // Compute the cross-platform
modifier from the cocoa modifier.
On 2014/02/11 22:18:16, rsesek wrote:
> nit: capitalize Cocoa

Done.

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:132: 
On 2014/02/11 22:18:16, rsesek wrote:
> nit: extra blank line

Done.

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:137: for (size_t i = 0; i <
arraysize(kAcceleratorList); ++i) {
On 2014/02/11 22:18:16, rsesek wrote:
> These two loops are awfully similar. Can they be condensed with a helper
> function?

Done.

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:177: const
ui::PlatformAcceleratorCocoa* platform_accelerator =
Almost all of the duplicated code lies in the invocation of
MacKeyCodeForWindowsKeyCode, which requires 2 local vars, and has a long
signature. Unfortunately, the response from the method is handled differently
(DCHECK vs graceful failure). While it is possible to refactor this code, I have
deemed the result to be equally complex to the current code.

On 2014/02/11 22:18:16, rsesek wrote:
> This block is also a bit similar to the one above.

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm (right):

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:66: int command_id =
item.tag;
On 2014/02/11 22:18:16, rsesek wrote:
> -tag returns NSInteger

Done.

https://codereview.chromium.org/152643007/diff/370001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa_browsertest.mm:93: // cocoa
accelerator map.
On 2014/02/11 22:18:16, rsesek wrote:
> nit: Cocoa

Done.

[Robert Sesek, 2014-02-11 23:21:24]

LGTM

https://codereview.chromium.org/152643007/diff/500001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/accelerators_cocoa.h (right):

https://codereview.chromium.org/152643007/diff/500001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa.h:44: NSString* key_equivalent,
NSUInteger modifiers) const;
nit: indent 2 more spaces

https://codereview.chromium.org/152643007/diff/500001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/accelerators_cocoa.mm (right):

https://codereview.chromium.org/152643007/diff/500001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:38: } kAcceleratorMap[] = {
Note: I didn't actually go through and check that every single one of these is
correct.

[erikchen, 2014-02-11 23:34:07]

rsesek: Can I get a lgtm on "ui/*" ? 
thakis is the only Cocoa owner, and he's OOO.

https://codereview.chromium.org/152643007/diff/500001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/accelerators_cocoa.h (right):

https://codereview.chromium.org/152643007/diff/500001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa.h:44: NSString* key_equivalent,
NSUInteger modifiers) const;
went with the output of clang-format.

On 2014/02/11 23:21:25, rsesek wrote:
> nit: indent 2 more spaces

https://codereview.chromium.org/152643007/diff/500001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/accelerators_cocoa.mm (right):

https://codereview.chromium.org/152643007/diff/500001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/accelerators_cocoa.mm:38: } kAcceleratorMap[] = {
Modulo the accelerators in kAcceleratorList, the unit tests show that there is a
bijection between these accelerators and the ones found in the main menu, and
that the modifiers/key_code/command_id are correct.

On 2014/02/11 23:21:25, rsesek wrote:
> Note: I didn't actually go through and check that every single one of these is
> correct.

[Robert Sesek, 2014-02-12 01:25:02]

slgtm for everything, and OWNERS approval for everything that I'm capable. But I
don't top-level own ui/, so you'll need sky@ or ben@.

[erikchen, 2014-02-12 01:59:35]

sky@, looking for a rubber stamp of
"ui/base/accelerators/platform_accelerator_cocoa.h"
thakis is OOO, and it's just a header file documentation change.

garykac@, looking for an owner review of
"ui/events/keycodes/keyboard_code_conversion_mac.mm"
I've copied my comment from patch set 1 for your convenience:

"This code is not used in the current code base (All calls to
MacKeyCodeForWindowsKeyCode use flags = 0). 

I don't understand lines 526-527, despite having looked at the CL in which this
code was introduced. https://codereview.chromium.org/2986004. This code causes
my newly written code to malfunction, so I removed it."

[sky, 2014-02-12 14:13:09]

ui/base/accelerators/platform_accelerator_cocoa.h LGTM

[garykac, 2014-02-12 18:14:59]

LGTM for keyboard_code_conversion_mac.mm

[erikchen, 2014-02-12 18:15:20]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-02-12 18:15:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erikchen@chromium.org/152643007/550001

[commit-bot: I haz the power, 2014-02-12 21:18:08]

Change committed as 250793