Disabled context menu for external component extension icon on MAC (to be consistent with Windows).

Disabled context menu for external component extension icon on MAC (to be consistent with Windows).

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

[groby-ooo-7-16, 2014-03-25 20:10:10]

https://codereview.chromium.org/197333005/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

https://codereview.chromium.org/197333005/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:152:
contextMenuController_.reset([[ExtensionActionContextMenuController alloc]
I believe that can move into the if() too, no?

https://codereview.chromium.org/197333005/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:161: }
Do we need to reset the menu to nil in an else branch?

[yefimt, 2014-03-25 21:12:17]

https://codereview.chromium.org/197333005/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

https://codereview.chromium.org/197333005/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:152:
contextMenuController_.reset([[ExtensionActionContextMenuController alloc]
not sure, in menuNeedsUpdate code expects it exists, at same time it probably
never be called in no menu. I don't have MAC to verify :(

On 2014/03/25 20:10:10, groby wrote:
> I believe that can move into the if() too, no?

https://codereview.chromium.org/197333005/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:161: }
I don't think it is necessary. It is the only place menu is set, so not setting
it will provide default behavior - no menu.

On 2014/03/25 20:10:10, groby wrote:
> Do we need to reset the menu to nil in an else branch?

[groby-ooo-7-16, 2014-03-25 21:15:53]

https://codereview.chromium.org/197333005/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

https://codereview.chromium.org/197333005/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:152:
contextMenuController_.reset([[ExtensionActionContextMenuController alloc]
On 2014/03/25 21:12:17, yefimt wrote:
> not sure, in menuNeedsUpdate code expects it exists, at same time it probably
> never be called in no menu. I don't have MAC to verify :(
> 
> On 2014/03/25 20:10:10, groby wrote:
> > I believe that can move into the if() too, no?
> 

menuNeedsUpdate is the menu's delegate. If menu is indeed set nowhere else, as
you indicate below, then yes, please do move this into the if statement.

[yefimt, 2014-03-25 21:19:24]

https://codereview.chromium.org/197333005/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

https://codereview.chromium.org/197333005/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:152:
contextMenuController_.reset([[ExtensionActionContextMenuController alloc]
On 2014/03/25 21:15:53, groby wrote:
> On 2014/03/25 21:12:17, yefimt wrote:
> > not sure, in menuNeedsUpdate code expects it exists, at same time it
probably
> > never be called in no menu. I don't have MAC to verify :(
> > 
> > On 2014/03/25 20:10:10, groby wrote:
> > > I believe that can move into the if() too, no?
> > 
> 
> menuNeedsUpdate is the menu's delegate. If menu is indeed set nowhere else, as
> you indicate below, then yes, please do move this into the if statement.

Done.

[groby-ooo-7-16, 2014-03-25 21:48:56]

lgtm

[commit-bot: I haz the power, 2014-03-25 21:49:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/197333005/70001

[commit-bot: I haz the power, 2014-03-26 02:01:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-26 02:01:07]

Retried try job too often on win_rel for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[yefimt, 2014-03-26 16:54:21]

The CQ bit was checked by yefim@chromium.org

[commit-bot: I haz the power, 2014-03-26 16:54:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/197333005/70001

[commit-bot: I haz the power, 2014-03-26 18:01:56]

Change committed as 259625

[yefimt, 2014-04-03 16:59:26]

A revert of this CL has been created in
https://codereview.chromium.org/213873003/ by yefim@chromium.org.

The reason for reverting is: This change was supposed to be applied to page
actions not browser actions..