[Mac] Implements context menus for Page Actions. Introduces a reusable subcla...

[Mac] Implements context menus for Page and Browser Actions. Introduces a reusable subclass of NSMenu that is used by both.

BUG=30655
TEST=Right click on any Page action or Browser Action, observe a context menu appears.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=36021

[Mark Mentovai, 2010-01-07 22:20:16]

LG, coupla no-big-deal comments, coupla questions.

http://codereview.chromium.org/525098/diff/3001/3006
File chrome/browser/cocoa/autocomplete_text_field_editor.mm (right):

http://codereview.chromium.org/525098/diff/3001/3006#newcode109
chrome/browser/cocoa/autocomplete_text_field_editor.mm:109:
GetExtensionsService()->GetExtensionById(
Maybe you want to save the extensions service part outside of the loop so you
don't need to make the long ugly call on each pass?

[self isFlipped] (line 107) isn't going to change during the lifetime of the
loop either, maybe you want to save that off up front too?

http://codereview.chromium.org/525098/diff/3001/3004
File chrome/browser/cocoa/extensions/extension_action_context_menu.mm (right):

http://codereview.chromium.org/525098/diff/3001/3004#newcode21
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:21:
~ExtensionUIDelegate() { extension_ = NULL; }
You don't need this.

http://codereview.chromium.org/525098/diff/3001/3004#newcode42
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:42: // Callback
for the context menu items.
No indent here, you're not in an {} and we don't indent these.

http://codereview.chromium.org/525098/diff/3001/3004#newcode50
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:50: enum {
This should be static or in an anonymous namespace.

http://codereview.chromium.org/525098/diff/3001/3004#newcode51
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:51:
KExtensionContextName = 0,
Capital K -> lowercase.

http://codereview.chromium.org/525098/diff/3001/3004#newcode86
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:86: [itemObj
setTarget:nil];
Does being target-less disable it?

Do you want to disable it?

http://codereview.chromium.org/525098/diff/3001/3004#newcode127
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:127:
Extension::DecodeIcon(extension_, Extension::EXTENSION_ICON_LARGE,
Does this do disk I/O to grab the icon?  If so, get it off the UI thread.

[Bons, 2010-01-08 20:57:05]

http://codereview.chromium.org/525098/diff/3001/3006
File chrome/browser/cocoa/autocomplete_text_field_editor.mm (right):

http://codereview.chromium.org/525098/diff/3001/3006#newcode109
chrome/browser/cocoa/autocomplete_text_field_editor.mm:109:
GetExtensionsService()->GetExtensionById(
On 2010/01/07 22:20:16, Mark Mentovai wrote:
> Maybe you want to save the extensions service part outside of the loop so you
> don't need to make the long ugly call on each pass?
> 
> [self isFlipped] (line 107) isn't going to change during the lifetime of the
> loop either, maybe you want to save that off up front too?

Done.

http://codereview.chromium.org/525098/diff/3001/3004
File chrome/browser/cocoa/extensions/extension_action_context_menu.mm (right):

http://codereview.chromium.org/525098/diff/3001/3004#newcode21
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:21:
~ExtensionUIDelegate() { extension_ = NULL; }
On 2010/01/07 22:20:16, Mark Mentovai wrote:
> You don't need this.

Done.

http://codereview.chromium.org/525098/diff/3001/3004#newcode42
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:42: // Callback
for the context menu items.
On 2010/01/07 22:20:16, Mark Mentovai wrote:
> No indent here, you're not in an {} and we don't indent these.

Done.

http://codereview.chromium.org/525098/diff/3001/3004#newcode50
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:50: enum {
On 2010/01/07 22:20:16, Mark Mentovai wrote:
> This should be static or in an anonymous namespace.

Done.

http://codereview.chromium.org/525098/diff/3001/3004#newcode51
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:51:
KExtensionContextName = 0,
On 2010/01/07 22:20:16, Mark Mentovai wrote:
> Capital K -> lowercase.

Done.

http://codereview.chromium.org/525098/diff/3001/3004#newcode86
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:86: [itemObj
setTarget:nil];
On 2010/01/07 22:20:16, Mark Mentovai wrote:
> Does being target-less disable it?
> 
> Do you want to disable it?
Yes. setDisabled doesn't seem to work as intended. Weirdness indeed.

http://codereview.chromium.org/525098/diff/3001/3004#newcode127
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:127:
Extension::DecodeIcon(extension_, Extension::EXTENSION_ICON_LARGE,
On 2010/01/07 22:20:16, Mark Mentovai wrote:
> Does this do disk I/O to grab the icon?  If so, get it off the UI thread.

Done.

[Pam (message me for reviews), 2010-01-08 21:05:02]

What he said, plus some tiny nits.

- Pam

http://codereview.chromium.org/525098/diff/3001/3002
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/525098/diff/3001/3002#newcode117
chrome/browser/cocoa/autocomplete_text_field.mm:117: const NSRect hintIconFrame
= [cell securityImageFrameForFrame:[self bounds]];
Clean up this [self bounds] too?

http://codereview.chromium.org/525098/diff/3001/3003
File chrome/browser/cocoa/extensions/extension_action_context_menu.h (right):

http://codereview.chromium.org/525098/diff/3001/3003#newcode1
chrome/browser/cocoa/extensions/extension_action_context_menu.h:1: // Copyright
(c) 2009 The Chromium Authors. All rights reserved.
2010

http://codereview.chromium.org/525098/diff/3001/3004
File chrome/browser/cocoa/extensions/extension_action_context_menu.mm (right):

http://codereview.chromium.org/525098/diff/3001/3004#newcode1
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:1: // Copyright
(c) 2009 The Chromium Authors. All rights reserved.
2010

[Bons, 2010-01-08 21:38:09]

http://codereview.chromium.org/525098/diff/3001/3002
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/525098/diff/3001/3002#newcode117
chrome/browser/cocoa/autocomplete_text_field.mm:117: const NSRect hintIconFrame
= [cell securityImageFrameForFrame:[self bounds]];
On 2010/01/08 21:05:02, Pam wrote:
> Clean up this [self bounds] too?

Done.

http://codereview.chromium.org/525098/diff/3001/3003
File chrome/browser/cocoa/extensions/extension_action_context_menu.h (right):

http://codereview.chromium.org/525098/diff/3001/3003#newcode1
chrome/browser/cocoa/extensions/extension_action_context_menu.h:1: // Copyright
(c) 2009 The Chromium Authors. All rights reserved.
On 2010/01/08 21:05:02, Pam wrote:
> 2010

Done.

http://codereview.chromium.org/525098/diff/3001/3004
File chrome/browser/cocoa/extensions/extension_action_context_menu.mm (right):

http://codereview.chromium.org/525098/diff/3001/3004#newcode1
chrome/browser/cocoa/extensions/extension_action_context_menu.mm:1: // Copyright
(c) 2009 The Chromium Authors. All rights reserved.
On 2010/01/08 21:05:02, Pam wrote:
> 2010

Done.

[Andrew Bonventre, 2010-01-11 23:32:55]

Added browser action support as well...

[Pam (message me for reviews), 2010-01-12 01:06:07]

OK.

- Pam

http://codereview.chromium.org/525098/diff/12001/13008
File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/525098/diff/12001/13008#newcode184
chrome/browser/cocoa/extensions/browser_actions_controller.mm:184: // NSButton
between alloc/init and setCell:.
This comment is now harder to follow, visually if not semantically. It would be
clearer if the two new lines [cell ...] were moved down.

[Mark Mentovai, 2010-01-12 01:41:29]

Check unit_tests crash on try run:

[35493:267:0111/154547:79820979219076:FATAL:/b/slave/mac/build/src/chrome/browser/cocoa/autocomplete_text_field.mm(142)]
Check failed: editor != nil.

[Mark Mentovai, 2010-01-12 01:54:10]

mmmhmm

lgtm with these changes

http://codereview.chromium.org/525098/diff/12001/13001
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/525098/diff/12001/13001#newcode130
chrome/browser/cocoa/autocomplete_text_field.mm:130: if ([theEvent
modifierFlags] & NSControlKeyMask == 0) {
Check your logic here.  This ought to be:

      if (([theEvent modifierFlags] & NSControlKeyMask) == 0) {

As currently written, it's equivalent to:

      if ([theEvent modifierFlags] & (NSControlKeyMask == 0)) {

so this will never fire.  Make sure that this works properly.

http://codereview.chromium.org/525098/diff/12001/13002
File chrome/browser/cocoa/extensions/extension_action_context_menu.h (right):

http://codereview.chromium.org/525098/diff/12001/13002#newcode16
chrome/browser/cocoa/extensions/extension_action_context_menu.h:16: // if a user
{right|CTRL}-clicks the view of the given extension.
Nit: CTRL -> lowercase
Nit: CTRL -> control

I think you can just say right-click.  Control-click is a well-known way to
achieve right-click on the Mac.  'Course, if you think it's clearer to spell out
"control" here too, that's OK.

[Andrew Bonventre, 2010-01-12 17:24:23]

Going to make sure the try bots do OK then submit.

http://codereview.chromium.org/525098/diff/12001/13001
File chrome/browser/cocoa/autocomplete_text_field.mm (right):

http://codereview.chromium.org/525098/diff/12001/13001#newcode130
chrome/browser/cocoa/autocomplete_text_field.mm:130: if ([theEvent
modifierFlags] & NSControlKeyMask == 0) {
On 2010/01/12 01:54:10, Mark Mentovai wrote:
> Check your logic here.  This ought to be:
> 
>       if (([theEvent modifierFlags] & NSControlKeyMask) == 0) {
> 
> As currently written, it's equivalent to:
> 
>       if ([theEvent modifierFlags] & (NSControlKeyMask == 0)) {
> 
> so this will never fire.  Make sure that this works properly.

Done.

http://codereview.chromium.org/525098/diff/12001/13008
File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/525098/diff/12001/13008#newcode184
chrome/browser/cocoa/extensions/browser_actions_controller.mm:184: // NSButton
between alloc/init and setCell:.
On 2010/01/12 01:06:07, Pam wrote:
> This comment is now harder to follow, visually if not semantically. It would
be
> clearer if the two new lines [cell ...] were moved down.

Done.

http://codereview.chromium.org/525098/diff/12001/13002
File chrome/browser/cocoa/extensions/extension_action_context_menu.h (right):

http://codereview.chromium.org/525098/diff/12001/13002#newcode16
chrome/browser/cocoa/extensions/extension_action_context_menu.h:16: // if a user
{right|CTRL}-clicks the view of the given extension.
On 2010/01/12 01:54:10, Mark Mentovai wrote:
> Nit: CTRL -> lowercase
> Nit: CTRL -> control
> 
> I think you can just say right-click.  Control-click is a well-known way to
> achieve right-click on the Mac.  'Course, if you think it's clearer to spell
out
> "control" here too, that's OK.

Done.