Convert RenderViewContextMenu to MenuItemView.

Convert RenderViewContextMenu to MenuItemView.

This CL is part of general GTK removal for ChromiumOS.  Menu2 uses GTK on linux so we are replacing it with MenuItemView.  Chrome Frame currently passes the context menu between processes by using the HMENU.  Because MenuItemView does not use HMENU, we need to use another mechanism.

This CL creates a ContextMenuModel struct that is serialized into an automation message for Chrome Frame.  ContextMenuModel contains the context menu definition in-band replacing the out-of-band HMENU.

BUG=chromium-os:13887
TEST=none


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

[rhashimoto, 2011-06-14 23:51:16]

Hi Amit and Ananta -

I've decided to create a new automation message struct, ContextMenuModel, and
use it to serialize the menu specification in ui::MenuModel to pass to Chrome
Frame (passing the spec in-band instead of out-of-band with the HMENU).

This CL needs more work, but it does work for simple context menus (no submenu,
checked, or radio items).  Could one of you take a look and let me know if this
looks like a reasonable approach?

This method should support extension items in the context menu.  Getting the
extension icon across would be some work - from the comments on
UtilCloneContextMenuImpl() it looks like the current implementation does not
support icons?

Thanks,
Roy

[rhashimoto, 2011-06-17 18:36:07]

Hi Amit and Ananta -

I think this may be ready for review.  I'm not sure which of you is more
appropriate as reviewer (or if it should be someone else).

I could use some advice on testing changes to Chrome Frame.  Is there any way to
run a try job?  Is there a standard way to log the automation messages?  And how
would I enable extensions in the Chrome Frame profile as Ananta suggested to see
if those menu items work properly?

Thanks,
Roy

On 2011/06/14 23:51:16, rhashimoto wrote:
> Hi Amit and Ananta -
> 
> I've decided to create a new automation message struct, ContextMenuModel, and
> use it to serialize the menu specification in ui::MenuModel to pass to Chrome
> Frame (passing the spec in-band instead of out-of-band with the HMENU).
> 
> This CL needs more work, but it does work for simple context menus (no
submenu,
> checked, or radio items).  Could one of you take a look and let me know if
this
> looks like a reasonable approach?
> 
> This method should support extension items in the context menu.  Getting the
> extension icon across would be some work - from the comments on
> UtilCloneContextMenuImpl() it looks like the current implementation does not
> support icons?
> 
> Thanks,
> Roy

[ananta, 2011-06-17 19:03:37]

I just reviewed the ChromeFrame specific portion. Just a few comments.

http://codereview.chromium.org/7167002/diff/10001/chrome/common/automation_me...
File chrome/common/automation_messages.h (right):

http://codereview.chromium.org/7167002/diff/10001/chrome/common/automation_me...
chrome/common/automation_messages.h:152: struct ContextMenuModel {
Please add some comments describing this structure.

http://codereview.chromium.org/7167002/diff/10001/chrome/common/automation_me...
chrome/common/automation_messages.h:166: std::vector<Item> items;
Please include vector.h in this file.

http://codereview.chromium.org/7167002/diff/10001/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/7167002/diff/10001/chrome_frame/utils.cc#newco...
chrome_frame/utils.cc:607: switch (item.type)
The curly brace indicating the switch statement scope should move up alongside
the switch statement.

http://codereview.chromium.org/7167002/diff/10001/chrome_frame/utils.cc#newco...
chrome_frame/utils.cc:609: case ui::MenuModel::TYPE_BUTTON_ITEM:
Just replace this with a default case and print out the offending menu type in
the NOTREACHED() statement?. 

Please move the default label at the end.

http://codereview.chromium.org/7167002/diff/10001/chrome_frame/utils.cc#newco...
chrome_frame/utils.cc:609: case ui::MenuModel::TYPE_BUTTON_ITEM:
The alignment for the case labels is wrong. Please fix.

[rhashimoto, 2011-06-17 23:34:36]

http://codereview.chromium.org/7167002/diff/10001/chrome/common/automation_me...
File chrome/common/automation_messages.h (right):

http://codereview.chromium.org/7167002/diff/10001/chrome/common/automation_me...
chrome/common/automation_messages.h:152: struct ContextMenuModel {
On 2011/06/17 19:03:38, ananta wrote:
> Please add some comments describing this structure.

Done.

http://codereview.chromium.org/7167002/diff/10001/chrome/common/automation_me...
chrome/common/automation_messages.h:166: std::vector<Item> items;
On 2011/06/17 19:03:38, ananta wrote:
> Please include vector.h in this file.

Done.

http://codereview.chromium.org/7167002/diff/10001/chrome_frame/utils.cc
File chrome_frame/utils.cc (right):

http://codereview.chromium.org/7167002/diff/10001/chrome_frame/utils.cc#newco...
chrome_frame/utils.cc:607: switch (item.type)
On 2011/06/17 19:03:38, ananta wrote:
> The curly brace indicating the switch statement scope should move up alongside
> the switch statement.

Done.

http://codereview.chromium.org/7167002/diff/10001/chrome_frame/utils.cc#newco...
chrome_frame/utils.cc:609: case ui::MenuModel::TYPE_BUTTON_ITEM:
On 2011/06/17 19:03:38, ananta wrote:
> Just replace this with a default case and print out the offending menu type in
> the NOTREACHED() statement?. 
> 
> Please move the default label at the end.

Done.

http://codereview.chromium.org/7167002/diff/10001/chrome_frame/utils.cc#newco...
chrome_frame/utils.cc:609: case ui::MenuModel::TYPE_BUTTON_ITEM:
On 2011/06/17 19:03:38, ananta wrote:
> Just replace this with a default case and print out the offending menu type in
> the NOTREACHED() statement?. 
> 
> Please move the default label at the end.

Done.

[rhashimoto, 2011-06-21 23:09:02]

+sky for reviewing changes to RenderViewContextMenu/RenderViewContextMenuViews

amit, ananta: Any guidance on try jobs, logging, and changing the profile to
enable extensions?  Pointers to docs are fine.

Thanks,
Roy

[sky, 2011-06-21 23:37:03]

LGTM

[rhashimoto, 2011-06-23 15:28:10]

amit, ananta: ping

Thanks,
Roy

[rhashimoto, 2011-06-28 23:37:28]

amit, ananta: ping

If there's someone else better suited for these questions, please feel free to
pass me off to them.

Thanks,
Roy

[rhashimoto, 2011-07-08 22:11:18]

amit, ananta: This CL is ready for your review of the Chrome Frame portions. 
chrome_frame_tests.exe passes all the ContextMenuTest.* tests in Release build
(it passes in Debug if timeouts are increased).  It fails the following non-menu
tests on my local machine:

[==========] 200 tests from 33 test cases ran. (403037 ms total)
[  PASSED  ] 195 tests.
[  FAILED  ] 5 tests, listed below:
[  FAILED  ] FullTabSeleniumTest.Core
[  FAILED  ] ChromeFrameTestWithWebServer.FAILS_FullTabModeIE_RefreshMshtmlTest
[  FAILED  ] IE/FullTabNavigationTest.RestrictedSite/0, where GetParam() =
4-byte object <00-00 00-00>
[  FAILED  ] MetaTag/FullTabNavigationTest.RestrictedSite/0, where GetParam() =
4-byte object <01-00 00-00>
[  FAILED  ] HttpHeader/FullTabNavigationTest.RestrictedSite/0, where GetParam()
= 4-byte object <02-00 00-00>

but it also fails the identical tests on the trunk so I believe they have to do
with my local machine/environment and not this CL.  PTAL.

Thanks,
Roy

[iyengar, 2011-07-11 22:09:17]

ChromeFrame code LGTM

http://codereview.chromium.org/7167002/diff/25001/chrome/common/automation_me...
File chrome/common/automation_messages.cc (right):

http://codereview.chromium.org/7167002/diff/25001/chrome/common/automation_me...
chrome/common/automation_messages.cc:678: size_t item_count;
initialize to 0?. Easier for debugging.

http://codereview.chromium.org/7167002/diff/25001/chrome/common/automation_me...
File chrome/common/automation_messages.h (right):

http://codereview.chromium.org/7167002/diff/25001/chrome/common/automation_me...
chrome/common/automation_messages.h:157: struct ContextMenuModel {
We can probably reword the comment as below:-
"This struct passes information about the context menu in Chrome stored as a
ui::MenuModel to Chrome. 
". The other sentences are fine.

http://codereview.chromium.org/7167002/diff/25001/chrome/common/automation_me...
chrome/common/automation_messages.h:171: int item_id;
which will be passed by Chrome to ChromeFrame?

[rhashimoto, 2011-07-11 22:38:58]

http://codereview.chromium.org/7167002/diff/25001/chrome/common/automation_me...
File chrome/common/automation_messages.cc (right):

http://codereview.chromium.org/7167002/diff/25001/chrome/common/automation_me...
chrome/common/automation_messages.cc:678: size_t item_count;
On 2011/07/11 22:09:17, iyengar wrote:
> initialize to 0?. Easier for debugging.

Done.

http://codereview.chromium.org/7167002/diff/25001/chrome/common/automation_me...
File chrome/common/automation_messages.h (right):

http://codereview.chromium.org/7167002/diff/25001/chrome/common/automation_me...
chrome/common/automation_messages.h:157: struct ContextMenuModel {
On 2011/07/11 22:09:17, iyengar wrote:
> We can probably reword the comment as below:-
> "This struct passes information about the context menu in Chrome stored as a
> ui::MenuModel to Chrome. 
> ". The other sentences are fine.

Done.

http://codereview.chromium.org/7167002/diff/25001/chrome/common/automation_me...
chrome/common/automation_messages.h:171: int item_id;
On 2011/07/11 22:09:17, iyengar wrote:
> which will be passed by Chrome to ChromeFrame?

Done.

[commit-bot: I haz the power, 2011-07-12 16:54:15]

Change committed as 92182