Hide default context menu for platform apps.

Hide default context menu for platform apps.

Platform apps can provide their own context menu by overriding
the contextmenu event. If they don't override this event, the
default Chrome menu should not be shown.

BUG=106687
TEST=Manually tested, and new browser test cases added.

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

[benwells, 2011-12-07 10:35:48]

Will look at adding tests for this tomorrow, any comments on the approach in the
meantime appreciated. In particular, is this the right method to use? Is it the
best host observer to use?

[Mihai Parparita -not on Chrome, 2011-12-08 03:34:52]

+Anthony since he knows context menus better than me

http://codereview.chromium.org/8834008/diff/1/chrome/browser/renderer_host/ch...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/8834008/diff/1/chrome/browser/renderer_host/ch...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:69: if
(message.type() == ViewHostMsg_ContextMenu::ID) {
Blocking this at the IPC level doesn't seem quite right. I think there are two
ways we can approach context menus in platform apps:

1. Remove context menu support entirely, since it's up to apps to decide how
they want to handle right-clicks. In that case, we would ideally want to prevent
the IPC being sent in the first place. This might require making WebKit changes
(perhaps in
https://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/W...
to allow the ContextMenuClient (of which Chrome has an implementation) to say
that that context menus shouldn't be shown at all).

2. Allow platform apps to customize the content of context menus (they're not
shown by default, but they can request items to be shown in it). In that case
we'd want to modify the behavior of RenderViewContextMenu::InitMenu
(https://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/tab_contents...),
so that for platform apps none of the other context menu items are added, but
the ones specified by the app are added.

[benwells, 2011-12-08 03:47:11]

On 2011/12/08 03:34:52, Mihai Parparita wrote:
> +Anthony since he knows context menus better than me
> 
>
http://codereview.chromium.org/8834008/diff/1/chrome/browser/renderer_host/ch...
> File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):
> 
>
http://codereview.chromium.org/8834008/diff/1/chrome/browser/renderer_host/ch...
> chrome/browser/renderer_host/chrome_render_view_host_observer.cc:69: if
> (message.type() == ViewHostMsg_ContextMenu::ID) {
> Blocking this at the IPC level doesn't seem quite right. I think there are two
> ways we can approach context menus in platform apps:
> 
> 1. Remove context menu support entirely, since it's up to apps to decide how
> they want to handle right-clicks. In that case, we would ideally want to
prevent
> the IPC being sent in the first place. This might require making WebKit
changes
> (perhaps in
>
https://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/W...
> to allow the ContextMenuClient (of which Chrome has an implementation) to say
> that that context menus shouldn't be shown at all).
> 
> 2. Allow platform apps to customize the content of context menus (they're not
> shown by default, but they can request items to be shown in it). In that case
> we'd want to modify the behavior of RenderViewContextMenu::InitMenu
>
(https://www.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/tab_contents...),
> so that for platform apps none of the other context menu items are added, but
> the ones specified by the app are added.

I had just reached a similar conclusion after discovering chrome.contextMenus.
Approach 2 would also let us put 'Inspect element' in there if you're in
developer mode if we wanted.

If apps want to completely take over the menu they can do so with the
contextmenu event.

Anthony - any opinions on the best appraoch?

[asargent_no_longer_on_chrome, 2011-12-08 18:50:46]

I like approach 2 - let platform apps control what goes into the "native" menu. 


There are 2 shortcomings with our current extension API for context menus that
have limited its usefulness from a developer perspective:

-Lack of an "onBeforeShow" event where developers can control what goes in the
menu at the time the click happens. Instead, we require them to register a set
of conditions up front and the browser decides which of their registered items
are applicable. We did it this way partly to avoid jankiness (don't want
extensions to accidentally slow things down in a way where they blame chrome
itself), and partly because the registration and event dispatch happen in a
different renderer (the background page typically) from where the click took
place because we didn't have support for calling extension APIs from content
scripts. See comment 7 on crbug.com/60758 for an approach I thought of about the
jankiness issue.

-No way to give a handle to the actual DOM node that the context menu was
brought up on. This is again because the event gets dispatched to the background
page, which is in a different renderer. See crbug.com/39507.


For platform apps, it would be nice to chart a different path. We should let
registration of menu items and event dispatch happen in their own renderer, and
we should give them an optional blocking onBeforeShow event which they can shoot
themselves in the foot with if they want. The current code which places the
items in the context menu (RenderViewContextMenu::Init) runs in the browser
process and just has access to the data in the ContextMenuParams struct (see
webkit/glue/context_menu.h) that was passed over IPC. There is a "custom_items"
vector there of type WebMenuItem, so we might just need to arrange to populate
that based on what the app wants.

[benwells, 2011-12-09 03:29:16]

Implemented using approach #2.

I haven't changed the way platform apps context menus work to address the issues
Antony raised. Maybe this can happen in a later change.

http://codereview.chromium.org/8834008/diff/3002/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/8834008/diff/3002/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:515: Browser*
active_browser = BrowserList::GetLastActive();
I don't really like this, but I think it should be OK for a context menu.

[asargent_no_longer_on_chrome, 2011-12-09 05:34:21]

Testing question: we recently added the extension_function_test_utils.{h,cc}
file with utilities that make some kinds of unit and browser tests easier - in
many cases you can avoid needing to use the javascript based test framework that
comes along with ExtensionApiTest. Can you take a quick look and see if it would
simplify any of your tests to use this? If not no big deal.

http://codereview.chromium.org/8834008/diff/7002/chrome/browser/extensions/pl...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/8834008/diff/7002/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_browsertest.cc:112:
PlatformAppContextMenu* menu =new PlatformAppContextMenu(tab_contents,
nit: "=new" -> "= new"

http://codereview.chromium.org/8834008/diff/7002/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/8834008/diff/7002/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:515: Browser*
active_browser = BrowserList::GetLastActive();
nit: it's probably better to use BrowserList::FindBrowserWithTabContents here
instead of GetLastActive.

http://codereview.chromium.org/8834008/diff/7002/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:517: const Extension*
platform_app = active_browser->GetPlatformApp();
Instead of having the GetPlatformApp method on the Browser class, would it make
sense to instead do:

ExtensionService* service =  profile_->GetExtensionService();
Extension* extension = service->GetInstalledApp(params_.src_url);
if (extension && extension->is_platform_app()) {
  ...
}

[asargent_no_longer_on_chrome, 2011-12-09 05:36:16]

> I haven't changed the way platform apps context menus work to address the
issues
> Antony raised. Maybe this can happen in a later change.

That's fine to leave for later - some of that may be a bit complicated and
require multiple CL's, etc.

[benwells, 2011-12-12 05:30:39]

I checked out the extension_function_test stuff but couldn't see how it would
help the tests I've added.

http://codereview.chromium.org/8834008/diff/7002/chrome/browser/extensions/pl...
File chrome/browser/extensions/platform_app_browsertest.cc (right):

http://codereview.chromium.org/8834008/diff/7002/chrome/browser/extensions/pl...
chrome/browser/extensions/platform_app_browsertest.cc:112:
PlatformAppContextMenu* menu =new PlatformAppContextMenu(tab_contents,
On 2011/12/09 05:34:21, Antony Sargent wrote:
> nit: "=new" -> "= new"

Done.

http://codereview.chromium.org/8834008/diff/7002/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/8834008/diff/7002/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:515: Browser*
active_browser = BrowserList::GetLastActive();
On 2011/12/09 05:34:21, Antony Sargent wrote:
> nit: it's probably better to use BrowserList::FindBrowserWithTabContents here
> instead of GetLastActive.

That's much better.

http://codereview.chromium.org/8834008/diff/7002/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:517: const Extension*
platform_app = active_browser->GetPlatformApp();
On 2011/12/09 05:34:21, Antony Sargent wrote:
> Instead of having the GetPlatformApp method on the Browser class, would it
make
> sense to instead do:
> 
> ExtensionService* service =  profile_->GetExtensionService();
> Extension* extension = service->GetInstalledApp(params_.src_url);
> if (extension && extension->is_platform_app()) {
>   ...
> }

To use the URL I would have to look at page_url, not src_url - or that's what my
debugger tells me - and strip out the scheme. That's not a problem.

I was going to use the Browser::GetPlatformApp for another change I have under
development to handle Chrome accelerators differently. Also, will the page url
method work for platform apps which open other windows? I don't know if they can
do that yet but I think its on the list of things for platform apps to do.

I don't know which way is better, I'm happy to do it either way.

[asargent_no_longer_on_chrome, 2011-12-12 18:17:42]

As it stands, LGTM. 

My thinking with the suggestion about using ExtensionService::GetInstalledApp
was that it might be nice to avoid having to teach the Browser class about
platform apps if we didn't need to. If there are going to be other places where
it would be helpful to have that, then it's fine to use it here too I think.

I've added sky to the CC list in case he has an opinion about the method you're
adding to the Browser class.

[sky, 2011-12-12 22:04:44]

GetPlatformApp feels like it should be a utility some where rather than a method
in Browser.

[benwells, 2011-12-12 23:53:51]

Moved code out of browser.cc into render_view_context_menu.cc. If this chunk of
code becomes common I'll extract it later.

sky: could you look at views/tab_contents/tab_contents_view_views.cc?

+avi as owner for tab_contents/render_view_context_menu.cc

[sky, 2011-12-13 00:15:11]

LGTM

[Avi (use Gerrit), 2011-12-13 00:41:37]

context menu lgtm

[commit-bot: I haz the power, 2011-12-13 00:43:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/8834008/19002

[Mihai Parparita -not on Chrome, 2011-12-13 01:12:44]

Feel free to fix this in a follow-up CL.

http://codereview.chromium.org/8834008/diff/19002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/8834008/diff/19002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:524: if (app &&
app->is_platform_app()) {
It seems like you would also want to do another is_platform_app check in
AppendAllExtensionItems, so that a platform app's menus don't get appended to
other pages.

[Mihai Parparita -not on Chrome, 2011-12-13 01:13:08]

LGTM in case my comment angers the commit bot.

[commit-bot: I haz the power, 2011-12-13 02:53:24]

Change committed as 114162