Implement context menu for page and browser actions

Add the right-click context menu for Browser actions and Page Actions.

BUG=29538
TEST=Right-click an extension icon and make sure all the options work for both
Page and Browser actions (Options should be grayed out when there is no Options
page specified in the manifest).

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

[Finnur, 2009-12-14 16:56:09]

Looks like I forgot to send it out... :/

[Erik does not do reviews, 2009-12-15 01:30:36]

http://codereview.chromium.org/486022/diff/1008/14
File chrome/browser/extensions/extension_action_context_menu.cc (right):

http://codereview.chromium.org/486022/diff/1008/14#newcode30
chrome/browser/extensions/extension_action_context_menu.cc:30: kDisable,
l10n_util::GetStringUTF16(IDS_EXTENSIONS_DISABLE));
I'm assuming that all of these strings already existed, right?

http://codereview.chromium.org/486022/diff/1008/14#newcode32
chrome/browser/extensions/extension_action_context_menu.cc:32: kConfigure,
l10n_util::GetStringUTF16(IDS_EXTENSIONS_OPTIONS));
I think options should be first, but I don't have anything scientific to go on
here (it seems more common in the steady state and it feels weird to have it
sandwiched between two destructive actions).  Is there any conventional wisdom
from the UI team we can use here?

http://codereview.chromium.org/486022/diff/1008/14#newcode55
chrome/browser/extensions/extension_action_context_menu.cc:55: Browser* browser
= BrowserList::GetLastActive();
If you're going to test this API in unit tests, this won't work (active will
return NULL).  It might be worth a comment even if you aren't testing it now
since this issue has come up a number of times in the past and is hard to track
down.

http://codereview.chromium.org/486022/diff/1008/14#newcode72
chrome/browser/extensions/extension_action_context_menu.cc:72:
CrxInstaller::DecodeInstallIcon(icon_path, &uninstall_icon);
This isn't necessary for uninstall since the icon has already been safely
converted.  You can just load the icon directly.

http://codereview.chromium.org/486022/diff/1008/15
File chrome/browser/extensions/extension_action_context_menu.h (right):

http://codereview.chromium.org/486022/diff/1008/15#newcode9
chrome/browser/extensions/extension_action_context_menu.h:9: #include
"views/controls/menu/menu_2.h"
doesn't that mean this is views specific?  (and that it needs an #if
defined(TOOLKIT_VIEWS))

http://codereview.chromium.org/486022/diff/1008/11
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/486022/diff/1008/11#newcode161
chrome/browser/views/browser_actions_container.cc:161: PopupDidShow();  // Maybe
we should rename this function?
Yes, please change these functions to something like "SetButtonPushed()", etc.

http://codereview.chromium.org/486022/diff/1008/11#newcode166
chrome/browser/views/browser_actions_container.cc:166: // Make the menu appear
below the button.
nit: newline above comment

http://codereview.chromium.org/486022/diff/1008/11#newcode167
chrome/browser/views/browser_actions_container.cc:167: gfx::Rect bounds =
GetBounds(APPLY_MIRRORING_TRANSFORMATION);
it looks like this mirroring aspect isn't used anywhere.  Any reason it's here?

http://codereview.chromium.org/486022/diff/1008/11#newcode168
chrome/browser/views/browser_actions_container.cc:168: point.Offset(0,
bounds.height());
in fact, why not just call height() directly on this view?

http://codereview.chromium.org/486022/diff/1008/12
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/486022/diff/1008/12#newcode1386
chrome/browser/views/location_bar_view.cc:1386: else if
(event.IsRightMouseButton()) {
nit: with one {, I think all of the conditionals need them

http://codereview.chromium.org/486022/diff/1008/12#newcode1390
chrome/browser/views/location_bar_view.cc:1390: // Make the menu appear below
the button.
nit: newline above comment

http://codereview.chromium.org/486022/diff/1008/12#newcode1392
chrome/browser/views/location_bar_view.cc:1392: point.Offset(0,
bounds.height());
just use height()

http://codereview.chromium.org/486022/diff/1008/13
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/486022/diff/1008/13#newcode623
chrome/chrome_browser.gypi:623:
'browser/extensions/extension_action_context_menu.cc',
This won't compile on mac or linux (gtk).

[Finnur, 2009-12-16 23:39:11]

Updated to address review comments.

http://codereview.chromium.org/486022/diff/1008/14
File chrome/browser/extensions/extension_action_context_menu.cc (right):

http://codereview.chromium.org/486022/diff/1008/14#newcode30
chrome/browser/extensions/extension_action_context_menu.cc:30: kDisable,
l10n_util::GetStringUTF16(IDS_EXTENSIONS_DISABLE));
Yes, these are the constants we use for the Extension Management UI.

[Erik does not do reviews, 2009-12-17 00:34:51]

http://codereview.chromium.org/486022/diff/6001/7009
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/486022/diff/6001/7009#newcode158
chrome/browser/extensions/crx_installer.cc:158:
extension_->GetIconPath(Extension::EXTENSION_ICON_LARGE).GetFilePath();
why figure out the path here?  do it in the Extension method instead?

http://codereview.chromium.org/486022/diff/6001/7014
File chrome/browser/extensions/extension_action_context_menu_model.cc (right):

http://codereview.chromium.org/486022/diff/6001/7014#newcode16
chrome/browser/extensions/extension_action_context_menu_model.cc:16: static
const int kName      = 1;
add a comment above this list.  since it's just an incrementing list of numbers,
why not an enum?

http://codereview.chromium.org/486022/diff/6001/7014#newcode38
chrome/browser/extensions/extension_action_context_menu_model.cc:38: if (locale
== "en-US" || locale == "en-GB")
are there other "en-"* locales?  (australia?  canada, etc.).  maybe do a prefix
match on "en-"?

http://codereview.chromium.org/486022/diff/6001/7014#newcode47
chrome/browser/extensions/extension_action_context_menu_model.cc:47: bool
ExtensionActionContextMenuModel::IsCommandIdChecked(int command_id) const {
checked?  is this a little checkbox next to the menu item?  If so, then this
should be false.

http://codereview.chromium.org/486022/diff/6001/7014#newcode75
chrome/browser/extensions/extension_action_context_menu_model.cc:75:
browser->OpenURL(extension_->options_url(), GURL(),
DCHECK that this is non-empty

http://codereview.chromium.org/486022/diff/6001/7015
File chrome/browser/extensions/extension_action_context_menu_model.h (right):

http://codereview.chromium.org/486022/diff/6001/7015#newcode9
chrome/browser/extensions/extension_action_context_menu_model.h:9: #include
"chrome/browser/extensions/extension_install_ui.h"
nit: you could forward declare these two classes rather than including.

http://codereview.chromium.org/486022/diff/6001/7008
File chrome/browser/extensions/extensions_ui.cc (right):

http://codereview.chromium.org/486022/diff/6001/7008#newcode410
chrome/browser/extensions/extensions_ui.cc:410:
CrxInstaller::DecodeInstallIcon(icon_path, Extension::EXTENSION_ICON_LARGE,
this can change to Extension::DecodeIcon

http://codereview.chromium.org/486022/diff/4007/5023
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/486022/diff/4007/5023#newcode182
chrome/common/extensions/extension.h:182: static void DecodeInstallIcon(const
FilePath& icon_path,
rename to DecodeIcon and remove the path (it can be computed)

[Finnur, 2009-12-17 01:11:48]

http://codereview.chromium.org/486022/diff/6001/7014
File chrome/browser/extensions/extension_action_context_menu_model.cc (right):

http://codereview.chromium.org/486022/diff/6001/7014#newcode38
chrome/browser/extensions/extension_action_context_menu_model.cc:38: if (locale
== "en-US" || locale == "en-GB")
There is only en-US and en-GB. But what you are suggesting is certainly more
generic...

[Erik does not do reviews, 2009-12-17 01:23:07]

LGTM

http://codereview.chromium.org/486022/diff/9012/8032
File chrome/browser/extensions/extension_action_context_menu_model.h (right):

http://codereview.chromium.org/486022/diff/9012/8032#newcode31
chrome/browser/extensions/extension_action_context_menu_model.h:31: enum
MenuEntries {
this should still be in the cc file, right?  (it's not used in the APIs)