Pepper/Flapper: First pass at context menu implementation.

Pepper/Flapper: First pass at context menu implementation.

This meets the needs of Flapper. We may want to generalize/restrict/modify the
API for inclusion into PPAPI, but hopefully this lays a foundation.

BUG=none
TEST=Flapper context menus work (with the right version of Flapper)

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

[piman, 2011-01-28 23:00:14]

http://codereview.chromium.org/6253017/diff/5001/ppapi/c/private/ppb_flash.h
File ppapi/c/private/ppb_flash.h (right):

http://codereview.chromium.org/6253017/diff/5001/ppapi/c/private/ppb_flash.h#...
ppapi/c/private/ppb_flash.h:22: #define PPB_FLASH_INTERFACE "PPB_Flash;4"
rev please !

[brettw, 2011-01-29 00:56:52]

http://codereview.chromium.org/6253017/diff/10001/chrome/common/render_messag...
File chrome/common/render_messages.cc (right):

http://codereview.chromium.org/6253017/diff/10001/chrome/common/render_messag...
chrome/common/render_messages.cc:105: void** iter,
Fix indentation.

http://codereview.chromium.org/6253017/diff/10001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6253017/diff/10001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:4723: has_saved_context_menu_action_ = true;
It would be nice if this was on the PepperPluginDelegate so that we could avoid
adding more state to the RenderView.

[viettrungluu, 2011-01-29 21:49:13]

It might even work (I can't test it from home)....

http://codereview.chromium.org/6253017/diff/5001/ppapi/c/private/ppb_flash.h
File ppapi/c/private/ppb_flash.h (right):

http://codereview.chromium.org/6253017/diff/5001/ppapi/c/private/ppb_flash.h#...
ppapi/c/private/ppb_flash.h:22: #define PPB_FLASH_INTERFACE "PPB_Flash;4"
On 2011/01/28 23:00:14, piman wrote:
> rev please !

Done.

http://codereview.chromium.org/6253017/diff/10001/chrome/common/render_messag...
File chrome/common/render_messages.cc (right):

http://codereview.chromium.org/6253017/diff/10001/chrome/common/render_messag...
chrome/common/render_messages.cc:105: void** iter,
On 2011/01/29 00:56:52, brettw wrote:
> Fix indentation.

Done.

http://codereview.chromium.org/6253017/diff/10001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6253017/diff/10001/chrome/renderer/render_view...
chrome/renderer/render_view.cc:4723: has_saved_context_menu_action_ = true;
On 2011/01/29 00:56:52, brettw wrote:
> It would be nice if this was on the PepperPluginDelegate so that we could
avoid
> adding more state to the RenderView.

Done. I just have the RenderView bounce things over to the PepperPluginDelegate
for Pepper menus.

[brettw, 2011-01-31 18:19:49]

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

http://codereview.chromium.org/6253017/diff/16033/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:122:
menu_model->AddSubMenu(
I would like to see some protection against infinite recursion. Currently the
plugin can set up a loop of references to sub menus which will cause the
renderer to blow out its stack and crash. It would be nice if this couldn't
happen. Maybe limit the maximum depth of menus to 5 or so?

http://codereview.chromium.org/6253017/diff/16033/chrome/renderer/pepper_plug...
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/6253017/diff/16033/chrome/renderer/pepper_plug...
chrome/renderer/pepper_plugin_delegate_impl.cc:896: // TODO(viettrungluu): "user
cancel" for everything else is not right....
What do you mean by "everything else" here? What cases is user cancel not
appropriate for?

http://codereview.chromium.org/6253017/diff/16033/ppapi/cpp/private/flash_menu.h
File ppapi/cpp/private/flash_menu.h (right):

http://codereview.chromium.org/6253017/diff/16033/ppapi/cpp/private/flash_men...
ppapi/cpp/private/flash_menu.h:23: class Menu : public Resource {
Is it really necessary to have C++ wrappers for this? I'd prefer to not have
wrappers for the private stuff since it seems like it might encourage random
people to use them, and makes the C++ wrapper library larger for no reason.
Especially since this wrapper is so simple.

http://codereview.chromium.org/6253017/diff/16033/webkit/glue/webmenuitem.h
File webkit/glue/webmenuitem.h (right):

http://codereview.chromium.org/6253017/diff/16033/webkit/glue/webmenuitem.h#n...
webkit/glue/webmenuitem.h:21: // TODO(viettrungluu): This is currently not
supported/used by WebKit.
It's not clear to me what this comment means. Is the TODO to make it used by
WebKit? I assume it's used by Pepper now? Maybe the comment should just say that
it's currently used by Pepper only and isn't supported by WebKit, without the
TODO.

[viettrungluu, 2011-01-31 19:01:40]

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

http://codereview.chromium.org/6253017/diff/16033/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:122:
menu_model->AddSubMenu(
On 2011/01/31 18:19:49, brettw wrote:
> I would like to see some protection against infinite recursion. Currently the
> plugin can set up a loop of references to sub menus which will cause the
> renderer to blow out its stack and crash. It would be nice if this couldn't
> happen. Maybe limit the maximum depth of menus to 5 or so?

This code (which is on the browser side) can't actually fall victim to infinite
recursion due to the way the menu is serialized, but I should add some sanity
checks on menu size anyway.

The PPAPI interface code already enforces size and recursion limitations (on the
renderer side).

http://codereview.chromium.org/6253017/diff/16033/chrome/renderer/pepper_plug...
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/6253017/diff/16033/chrome/renderer/pepper_plug...
chrome/renderer/pepper_plugin_delegate_impl.cc:896: // TODO(viettrungluu): "user
cancel" for everything else is not right....
On 2011/01/31 18:19:49, brettw wrote:
> What do you mean by "everything else" here? What cases is user cancel not
> appropriate for?

Conceivably, other things (not directly done by the user) might make the context
menu disappear -- e.g., some other program grabbing focus. It's not worth the
TODO though, so I'll get rid of it.

http://codereview.chromium.org/6253017/diff/16033/ppapi/cpp/private/flash_menu.h
File ppapi/cpp/private/flash_menu.h (right):

http://codereview.chromium.org/6253017/diff/16033/ppapi/cpp/private/flash_men...
ppapi/cpp/private/flash_menu.h:23: class Menu : public Resource {
On 2011/01/31 18:19:49, brettw wrote:
> Is it really necessary to have C++ wrappers for this? I'd prefer to not have
> wrappers for the private stuff since it seems like it might encourage random
> people to use them, and makes the C++ wrapper library larger for no reason.
> Especially since this wrapper is so simple.

It makes the Flapper-side code nicer (e.g., no need to explicitly get the
interface). For IP reasons (especially if it gets turned into a non-private
interface), I'd rather that this live in the Chromium tree. (It doesn't get
linked in to the standard C++ wrapper library.)

[viettrungluu, 2011-01-31 20:31:14]

New patch up; take another look?

http://codereview.chromium.org/6253017/diff/16033/webkit/glue/webmenuitem.h
File webkit/glue/webmenuitem.h (right):

http://codereview.chromium.org/6253017/diff/16033/webkit/glue/webmenuitem.h#n...
webkit/glue/webmenuitem.h:21: // TODO(viettrungluu): This is currently not
supported/used by WebKit.
On 2011/01/31 18:19:49, brettw wrote:
> It's not clear to me what this comment means. Is the TODO to make it used by
> WebKit? I assume it's used by Pepper now? Maybe the comment should just say
that
> it's currently used by Pepper only and isn't supported by WebKit, without the
> TODO.

Done.

[brettw, 2011-01-31 20:40:48]

LGTM stop bugging me.

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

http://codereview.chromium.org/6253017/diff/16034/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:153: void
AddCustomItemsToMenu(const std::vector<WebMenuItem>& items,
It seems like this is unnecessary.