Give platform apps control over launcher right-click context menu.

Give platform apps control over launcher right-click context menu.
This moves most of the extension context menu related code from RenderViewContextMenu to a seperate class, which is then used in two other places as well.

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

[Marijn Kruisselbrink, 2012-09-06 17:54:50]

A first attempt at making it possible for platform apps to add items to the
launcher right-click context menu. A big part of this patch is trying to factor
out the extension related context menu code from RenderViewContextMenu into a
separate class (although I'm not sure if the name of this class or its location
in the tree is best?).

To make this split out class usable in both the original RenderViewContextMenu
place and in the new locations for the launcher context menu, I needed some way
to specify which MenuItems to include in the context menu. Currently I've done
that by passing a Callback<bool(const MenuItem*)> to this new class, but I'm not
sure if maybe a pure virtual method and having multiple implementations of that
might have been a better way (or even a template parameter).

Another thing is that currently the context-menu onClick event requires a
page_url to be passed. Launcher context menus don't really have a page_url so
I'm now passing "about:blank" as page_url, but maybe it would be better to just
make the parameter optional (and document that it is not set for launcher
context menu items).

https://chromiumcodereview.appspot.com/10918103/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/context_menu_manager.h (right):

https://chromiumcodereview.appspot.com/10918103/diff/1/chrome/browser/extensi...
chrome/browser/extensions/context_menu_manager.h:44: static bool
ContextIsLauncher(const MenuItem* item);
This method should probably instead of being here just be implemented separately
by the two launcher context menu related code paths where it is currently used
(it is only used to pass as filter when creating this class).

[asargent_no_longer_on_chrome, 2012-09-10 20:25:19]

I like the overall approach - I think that it's really useful to pull out some
of this extensions-specific code from RenderViewContextMenu, which has gotten
pretty big and unwieldy.

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
File chrome/browser/extensions/context_menu_manager.h (right):

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
chrome/browser/extensions/context_menu_manager.h:21: class ContextMenuManager {
I keep trying to come up with a better name to suggest for this class, since we
already have the MenuManager class in extensions which serves a somewhat
different purpose. The MenuManager class is all about holding the logical model
of items that might apply in a concrete situation, and this class is a helper
for populating a context menu in a concrete situation. 

Some ideas I've come up with:

ContextMenuItems
ContextMenuDelegate
ContextMenuAppender
ContextMenuMatcher

I think I might like ContextMenuMatcher a little better than the others, but
none of them seem great. Got any better ideas?

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
chrome/browser/extensions/context_menu_manager.h:28: const
base::Callback<bool(const MenuItem*)>& filter);
It's probably worth a comment here that |filter| will be called on possibly
matching items, and its result used to make the final decision about inclusion.

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
chrome/browser/extensions/context_menu_manager.h:44: static bool
ContextIsLauncher(const MenuItem* item);
On 2012/09/06 17:54:50, Marijn Kruisselbrink wrote:
> This method should probably instead of being here just be implemented
separately
> by the two launcher context menu related code paths where it is currently used
> (it is only used to pass as filter when creating this class).

Yes, it might make more sense to implemented in an anonymous namespace just
above ExtensionAppItem::GetContextMenuModel. You might also consider renaming it
to "HasLauncherContext", since it looks like |item| could have multiple contexts
and this will return true if LAUNCHER is one of them.

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
chrome/browser/extensions/context_menu_manager.h:46: // These two methods should
only be used for tests
Can you put them in a protected: section and make the tests in question friend
classes?

Alternatively, sometimes we give methods a name like FooForTest to emphasize
that regular consumers should not use them (this is to prevent the situation of
a chrome developer copy/pasting from existing usage in a test somewhere and
never having come by the .h file to see a comment telling them they shouldn't be
using the method)

http://codereview.chromium.org/10918103/diff/1/chrome/browser/tab_contents/re...
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/10918103/diff/1/chrome/browser/tab_contents/re...
chrome/browser/tab_contents/render_view_context_menu.h:185:
extensions::ContextMenuManager menu_manager_;
nit: let's name this variable extension_items_ or something with "extension" in
it

http://codereview.chromium.org/10918103/diff/1/chrome/browser/ui/app_list/ext...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

http://codereview.chromium.org/10918103/diff/1/chrome/browser/ui/app_list/ext...
chrome/browser/ui/app_list/extension_app_item.cc:276: // TODO(mek): maybe make
page_url not required?
Yeah, I think it would be good to make page_url be optional

http://codereview.chromium.org/10918103/diff/1/chrome/browser/ui/ash/launcher...
File chrome/browser/ui/ash/launcher/launcher_context_menu.h (right):

http://codereview.chromium.org/10918103/diff/1/chrome/browser/ui/ash/launcher...
chrome/browser/ui/ash/launcher/launcher_context_menu.h:66:
scoped_ptr<extensions::ContextMenuManager> context_menu_manager_;
same naming nit here - might be better to have something extensions-specific

[Marijn Kruisselbrink, 2012-09-10 23:16:56]

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
File chrome/browser/extensions/context_menu_manager.h (right):

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
chrome/browser/extensions/context_menu_manager.h:21: class ContextMenuManager {
On 2012/09/10 20:25:20, Antony Sargent wrote:
> I keep trying to come up with a better name to suggest for this class, since
we
> already have the MenuManager class in extensions which serves a somewhat
> different purpose. The MenuManager class is all about holding the logical
model
> of items that might apply in a concrete situation, and this class is a helper
> for populating a context menu in a concrete situation. 
> 
> Some ideas I've come up with:
> 
> ContextMenuItems
> ContextMenuDelegate
> ContextMenuAppender
> ContextMenuMatcher
> 
> I think I might like ContextMenuMatcher a little better than the others, but
> none of them seem great. Got any better ideas?
I agree that the name of the class sucks. Unfortunately I don't have better
ideas (otherwise I would have used a different name to begin with :) ). Not sure
I like Matcher better... maybe ContextMenuHandler? Although that's also pretty
vague in what it does...
Oh, and is browser/extensions/ the right location for this class, considering
that it is used i code in browser/tab_contents, browser/ui/app_list and
browser/ui/ash/launcher?

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
chrome/browser/extensions/context_menu_manager.h:28: const
base::Callback<bool(const MenuItem*)>& filter);
On 2012/09/10 20:25:20, Antony Sargent wrote:
> It's probably worth a comment here that |filter| will be called on possibly
> matching items, and its result used to make the final decision about
inclusion. 

Done.

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
chrome/browser/extensions/context_menu_manager.h:44: static bool
ContextIsLauncher(const MenuItem* item);
On 2012/09/10 20:25:20, Antony Sargent wrote:
> On 2012/09/06 17:54:50, Marijn Kruisselbrink wrote:
> > This method should probably instead of being here just be implemented
> separately
> > by the two launcher context menu related code paths where it is currently
used
> > (it is only used to pass as filter when creating this class).
> 
> Yes, it might make more sense to implemented in an anonymous namespace just
> above ExtensionAppItem::GetContextMenuModel. You might also consider renaming
it
> to "HasLauncherContext", since it looks like |item| could have multiple
contexts
> and this will return true if LAUNCHER is one of them. 

Done.

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
chrome/browser/extensions/context_menu_manager.h:46: // These two methods should
only be used for tests
On 2012/09/10 20:25:20, Antony Sargent wrote:
> Can you put them in a protected: section and make the tests in question friend
> classes?
Unfortunately the test code that uses these methods is (currently) in a class in
an anonymous namespace, not in the test class itself. Which makes adding a
friend declaration a bit harder (it's what I tried initially). I'm not sure what
the best way to go here is. The easiest would be of course to go with the
FooForTest naming and not change anything else, alternatively I guess I can have
this class in the anonymous namespace call a method in
ExtensionContextMenuBrowserTest which then will call these methods making it
possible to add that class as a friend, or move the entire implementation of
these methods that are currently in <anonymous>::TestRenderViewContextMenu to
ExtensionContextMenuBrowserTest.
I think I'm leaning towards that last option, since it seems the cleanest.

http://codereview.chromium.org/10918103/diff/1/chrome/browser/tab_contents/re...
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/10918103/diff/1/chrome/browser/tab_contents/re...
chrome/browser/tab_contents/render_view_context_menu.h:185:
extensions::ContextMenuManager menu_manager_;
On 2012/09/10 20:25:20, Antony Sargent wrote:
> nit: let's name this variable extension_items_ or something with "extension"
in
> it

Done.

http://codereview.chromium.org/10918103/diff/1/chrome/browser/ui/app_list/ext...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

http://codereview.chromium.org/10918103/diff/1/chrome/browser/ui/app_list/ext...
chrome/browser/ui/app_list/extension_app_item.cc:276: // TODO(mek): maybe make
page_url not required?
On 2012/09/10 20:25:20, Antony Sargent wrote:
> Yeah, I think it would be good to make page_url be optional

Done.

http://codereview.chromium.org/10918103/diff/1/chrome/browser/ui/ash/launcher...
File chrome/browser/ui/ash/launcher/launcher_context_menu.h (right):

http://codereview.chromium.org/10918103/diff/1/chrome/browser/ui/ash/launcher...
chrome/browser/ui/ash/launcher/launcher_context_menu.h:66:
scoped_ptr<extensions::ContextMenuManager> context_menu_manager_;
On 2012/09/10 20:25:20, Antony Sargent wrote:
> same naming nit here - might be better to have something extensions-specific

Done.

[Mihai Parparita -not on Chrome, 2012-09-16 06:47:41]

Unfortunately I won't be able to review this before I leave for vacation, so I'm
taking myself off the reviewers list.

[asargent_no_longer_on_chrome, 2012-09-21 19:44:22]

LGTM

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
File chrome/browser/extensions/context_menu_manager.h (right):

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
chrome/browser/extensions/context_menu_manager.h:21: class ContextMenuManager {
On 2012/09/10 23:16:57, Marijn Kruisselbrink wrote:
> On 2012/09/10 20:25:20, Antony Sargent wrote:
> > I keep trying to come up with a better name to suggest for this class, since
> we
> > already have the MenuManager class in extensions which serves a somewhat
> > different purpose. The MenuManager class is all about holding the logical
> model
> > of items that might apply in a concrete situation, and this class is a
helper
> > for populating a context menu in a concrete situation. 
> > 
> > Some ideas I've come up with:
> > 
> > ContextMenuItems
> > ContextMenuDelegate
> > ContextMenuAppender
> > ContextMenuMatcher
> > 
> > I think I might like ContextMenuMatcher a little better than the others, but
> > none of them seem great. Got any better ideas?
> I agree that the name of the class sucks. Unfortunately I don't have better
> ideas (otherwise I would have used a different name to begin with :) ). Not
sure
> I like Matcher better... maybe ContextMenuHandler? Although that's also pretty
> vague in what it does...
> Oh, and is browser/extensions/ the right location for this class, considering
> that it is used i code in browser/tab_contents, browser/ui/app_list and
> browser/ui/ash/launcher?

I think I have a slight preference for ContextMenuMatcher, but
ContextMenuHandler seems ok to me too. Pick whichever one you like better.

Also, I think it's ok to leave this code defined here in
chrome/browser/extensions/. It seems pretty focused on extension specific stuff.

http://codereview.chromium.org/10918103/diff/1/chrome/browser/extensions/cont...
chrome/browser/extensions/context_menu_manager.h:46: // These two methods should
only be used for tests
On 2012/09/10 23:16:57, Marijn Kruisselbrink wrote:
> On 2012/09/10 20:25:20, Antony Sargent wrote:
> > Can you put them in a protected: section and make the tests in question
friend
> > classes?
> Unfortunately the test code that uses these methods is (currently) in a class
in
> an anonymous namespace, not in the test class itself. Which makes adding a
> friend declaration a bit harder (it's what I tried initially). I'm not sure
what
> the best way to go here is. The easiest would be of course to go with the
> FooForTest naming and not change anything else, alternatively I guess I can
have
> this class in the anonymous namespace call a method in
> ExtensionContextMenuBrowserTest which then will call these methods making it
> possible to add that class as a friend, or move the entire implementation of
> these methods that are currently in <anonymous>::TestRenderViewContextMenu to
> ExtensionContextMenuBrowserTest.
> I think I'm leaning towards that last option, since it seems the cleanest.
Ok - the last option sounds fine to me.

[Marijn Kruisselbrink, 2012-09-21 22:27:17]

xiyuan: can you review the chrome/browser/ui/app_list/ part?
sky: can you review the chrome/browser/ui/ash/launcher/ part?
brettw: you might have an opinion on the
chrome/browser/tab_contents/render_view_context_menu part?

[xiyuan, 2012-09-21 22:43:19]

chrome/browser/ui/app_list/ LGTM

[sky, 2012-09-21 23:30:01]

http://codereview.chromium.org/10918103/diff/15001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right):

http://codereview.chromium.org/10918103/diff/15001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:32: extension_items_(new
extensions::ContextMenuMatcher(
Having code specific to the item type isn't going to scale well. Could we
offload this to LauncherItemController so that the app specific one can do what
it needs?

[Marijn Kruisselbrink, 2012-09-22 01:01:18]

http://codereview.chromium.org/10918103/diff/15001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right):

http://codereview.chromium.org/10918103/diff/15001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/launcher_context_menu.cc:32: extension_items_(new
extensions::ContextMenuMatcher(
On 2012/09/21 23:30:01, sky wrote:
> Having code specific to the item type isn't going to scale well. Could we
> offload this to LauncherItemController so that the app specific one can do
what
> it needs?
A potential problem I see with that is that it seems like a
LauncherItemController instance is only created the first time an app is
launched. If that is actually a problem I don't know; at the moment the context
menu of an app is already completely different based on it being pinned or
unpinned even if it is running (pinning it changes the item_.type away from
TYPE_PLATFORM_APP), so the menu items wouldn't show up in pinned state anyway at
the moment...

[sky, 2012-09-24 13:53:48]

On Fri, Sep 21, 2012 at 6:01 PM,  <mek@chromium.org> wrote:
>
>
http://codereview.chromium.org/10918103/diff/15001/chrome/browser/ui/ash/laun...
> File chrome/browser/ui/ash/launcher/launcher_context_menu.cc (right):
>
>
http://codereview.chromium.org/10918103/diff/15001/chrome/browser/ui/ash/laun...
> chrome/browser/ui/ash/launcher/launcher_context_menu.cc:32:
> extension_items_(new extensions::ContextMenuMatcher(
> On 2012/09/21 23:30:01, sky wrote:
>>
>> Having code specific to the item type isn't going to scale well. Could
>
> we
>>
>> offload this to LauncherItemController so that the app specific one
>
> can do what
>>
>> it needs?
>
> A potential problem I see with that is that it seems like a
> LauncherItemController instance is only created the first time an app is
> launched. If that is actually a problem I don't know; at the moment the
> context menu of an app is already completely different based on it being
> pinned or unpinned even if it is running (pinning it changes the
> item_.type away from TYPE_PLATFORM_APP), so the menu items wouldn't show
> up in pinned state anyway at the moment...

Two ways to address that. The shortcut would could call into a static
on the other, or we could nuke the shortcut one and always use the
other.

  -Scott

[Marijn Kruisselbrink, 2012-09-24 18:23:06]

On 2012/09/24 13:53:48, sky wrote:
> On Fri, Sep 21, 2012 at 6:01 PM,  <mailto:mek@chromium.org> wrote:
> > On 2012/09/21 23:30:01, sky wrote:

> >> Having code specific to the item type isn't going to scale well. Could we
> >> offload this to LauncherItemController so that the app specific one can do
what
> >> it needs?
> >
> > A potential problem I see with that is that it seems like a
> > LauncherItemController instance is only created the first time an app is
> > launched.
Actually, there is a much bigger problem with trying to move this code to 
LauncherItemController. These context menus can potentially contain submenus,
and the MenuModel for a submenu has to be owned by the same object as whoever
owns the menu it is in. Since there is no way to know who will end up being the
owner of the LauncherContextMenu (it is whoever ends up calling
LauncherDelegate::CreateContextMenu), the only way to fulfill this requirement
is to make the LauncherContextMenu itself owner of these menu models, which is
exactly what I'm doing now by having the extensions::ContextMenuMatcher a part
of the LauncherContextMenu.

I did now change it to not depend on item_.type values anymore, but instead just
add these context menu items whenever the item has an app_id associated with it
(which also nicely solves the problem of these items not showing up in pinned
items).


Marijn

[sky, 2012-09-24 21:05:35]

I am OK with LauncherContextMenu taking additional menus, but I don't want it to
have extension specific code. What I want to see is:
ChromeLauncherController::CreateContextMenu
  calls through to LauncherItemController
extension specific LauncherItemController supplies additional args or whatever
so that it can create the context menu.

[Marijn Kruisselbrink, 2012-09-24 23:17:44]

On 2012/09/24 21:05:35, sky wrote:
> I am OK with LauncherContextMenu taking additional menus, but I don't want it
to
> have extension specific code. What I want to see is:
> ChromeLauncherController::CreateContextMenu
>   calls through to LauncherItemController
> extension specific LauncherItemController supplies additional args or whatever
> so that it can create the context menu.
The problem isn't so much creating the context menu, but it is keeping
the right data around for the right amount of time so the menu items can
actually be called and we don't leak any memory.


The end result I think we're trying to accomplish is for
ChromeLauncherController::CreateContextMenu to return a ui::MenuModel*
(or some other object if we're willing to change the
ash::LauncherDelegate API) that represents a popup menu that besides the
current items also contains possibly a number of menu items and submenus
defined by the app represented by the LauncherItem.

To do this I see two possibilities, either this ui::MenuModel is a
LauncherContextMenu instance, or it is an instance of a to-be-written
ui::MenuModel deriving class that wraps a LauncherContextMenu.

If we go for the first option, where we directly return a
LauncherContextMenu instance, this class will have to get some extra
data members, most importantly a ScopedVector<ui::SimpleMenuModel> in
order to fullfill the SimpleMenuModel::AddSubMenu requirements that
state that any MenuModel you add as a SubMenu needs to have the same
lifetime as the SimpleMenuModel you add it to (so with the assumption
that we don't change the ash::LauncherDelegate API and return a
LauncherContextMenu instance directly, the only way to have lifetime of
these submenus managed correctly is to have LauncherContextMenu take
care of this).
The ScopedVector<> would take care of the lifetime of the menu-items and
submenus an app might add, but isn't enough to make sure these items can
actually be invoked. To do that we'll need to somehow be able to map a
[MenuModel* model, int command_id] pair to a extensions::MenuItem::Id.
I'm assuming that we don't want to keep around mappings for items in
menu models that no longer exist/are deleted, so this comes down to
another piece of data where the lifetime should be linked to that of the
MenuModel* returned by CreateContextMenu, which would mean another
member in LauncherContextMenu (or one object containing both pieces of
data like I've done now).

So since that solution is apparently something that is not desired,
there is the option of having CreateContextMenu return a new
ui::MenuModel deriving class (ChromeLauncherContextMenu? not sure I can
think of any meaningfull name to describe what this class does and that
distinguishes it from the existing LauncherContextMenu class). This new
class would have a LauncherContextMenu member as well as an
extensions::ContextMenuMatcher member (or at least the two pieces of
data whose lifetime needs to be linked to that of the ui::MenuModel),
implement the MenuModel interface forwarding most methods to the
LauncherContextMenu instance but treating some index values as refering
to the app specific menu items (probably the least fragile way to do
this would be to always have these appear at the end of the menu).

To me the second solution seems even worse than the first one, but maybe
I'm missing some thing and there is another solution that does solve
these problems?

[sky, 2012-09-25 00:00:33]

You convinced me what you have is best. Thanks for trying. LGTM

[commit-bot: I haz the power, 2012-09-25 20:18:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10918103/22004

[commit-bot: I haz the power, 2012-09-25 20:19:03]

Failed to apply patch for
chrome/browser/extensions/api/context_menu/context_menu_api.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/api/context_menu/context_menu_api.cc
Hunk #1 FAILED at 75.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/api/context_menu/context_menu_api.cc.rej

[commit-bot: I haz the power, 2012-09-25 20:49:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mek@chromium.org/10918103/28001

[commit-bot: I haz the power, 2012-09-25 23:51:19]

Change committed as 158691