Add options menu on Windows.

Add options menu for panels on Windows.

BUG=none
TEST=panel_browser_view_browsertest
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=85006

[jennb, 2011-05-10 22:38:44]

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:23: #include
"views/controls/menu/menu_2.h"
alpha ordering wrong

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:365: // Currently no
accelerators.
Need TODO to add accelerators? or NOTIMPLEMENTED()?

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:545:
option_menu_manipulate_all_enabled_ =
Oh, I misunderstood that this is menu-state, not panels-state.  Is there a way
to check what is in the options menu rather than have to store menu-state
separately?  If the menu is closed, won't this menu-state be stale?

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:563: const string16
minimize_all_text =
Move the *_all* strings inside where they are actually used.

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:569: const string16
about_text =
Move down to where about menu item is added.

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:582: if
(rebuild_menu_items)
Seems like if you got this far, rebuild_menu_items should always be true?

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.h (right):

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.h:94: static const int
kMinimizeAllCommand = 0;
Could these commands be enums?

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.h:139: bool
option_menu_manipulate_all_enabled_;
Seems like this should be queried from the panel manager rather than state to be
tracked in every panel.  Same comment for the option_menu_restore_all_enabled_
var.

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:48:
ui::SimpleMenuModel* options_menu_contents, int id1, int id2, int id3) {
You could make this more flexible and pass in expected menu item count and a
list of item ids/separator.   Then you don't need 2 of these helper functions.

[jianli, 2011-05-10 23:46:58]

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:23: #include
"views/controls/menu/menu_2.h"
On 2011/05/10 22:38:44, jennb wrote:
> alpha ordering wrong

I think ".../menu/..." should be after ".../button/...", right?

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:365: // Currently no
accelerators.
On 2011/05/10 22:38:44, jennb wrote:
> Need TODO to add accelerators? or NOTIMPLEMENTED()?

I removed the comment since it is misleading. We do not support accelerators,
like lots of other menus.

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:545:
option_menu_manipulate_all_enabled_ =
On 2011/05/10 22:38:44, jennb wrote:
> Oh, I misunderstood that this is menu-state, not panels-state.  Is there a way
> to check what is in the options menu rather than have to store menu-state
> separately?  If the menu is closed, won't this menu-state be stale?

Changed to check the state by querying the menu.

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:563: const string16
minimize_all_text =
On 2011/05/10 22:38:44, jennb wrote:
> Move the *_all* strings inside where they are actually used.

Done.

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:569: const string16
about_text =
On 2011/05/10 22:38:44, jennb wrote:
> Move down to where about menu item is added.

Done.

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.cc:582: if
(rebuild_menu_items)
On 2011/05/10 22:38:44, jennb wrote:
> Seems like if you got this far, rebuild_menu_items should always be true?

This function could be called first time when there is not menu being created
and Rebuild is not necessary. Added a comment to make it clear.

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_frame_view.h (right):

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.h:94: static const int
kMinimizeAllCommand = 0;
On 2011/05/10 22:38:44, jennb wrote:
> Could these commands be enums?

Done.

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_frame_view.h:139: bool
option_menu_manipulate_all_enabled_;
On 2011/05/10 22:38:44, jennb wrote:
> Seems like this should be queried from the panel manager rather than state to
be
> tracked in every panel.  Same comment for the option_menu_restore_all_enabled_
> var.

Removed. See my comment in other place.

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7005006/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:48:
ui::SimpleMenuModel* options_menu_contents, int id1, int id2, int id3) {
On 2011/05/10 22:38:44, jennb wrote:
> You could make this more flexible and pass in expected menu item count and a
> list of item ids/separator.   Then you don't need 2 of these helper functions.

Done.

[jennb, 2011-05-11 18:06:10]

LGTM

Some comments are repeats of what we already talked about.  I only put them here
for the record.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.cc:23: #include
"views/controls/menu/menu_2.h"
I meant this goes below the next line.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.cc:547:
options_menu_contents_->GetCommandIdAt(0) == COMMAND_RESTORE_ALL;
I wish there was a way to check HasCommandId(COMMAND_RESTORE_ALL) so that this
doesn't break if we rearrange menu item ordering.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.cc:559:
options_menu_contents_->AddItem(
It would be easier to read if you did:
if (should_restore_all)
  ..AddItem(..., ...GetString...);
else
  ...AddItem(...,...GetString...);

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.cc:574: if
(rebuild_menu_items)
Feels like this method should return true if options_menu needs to be rebuilt. 
Then in CreateOptionsMenu(), call options_menu_->Rebuild() if needed.  Accessing
options_menu_ here confuses me when we return from this method to line 528 which
checks if options_menu_ even exists.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_frame_view.h (right):

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.h:100: static const int
kMinimizeAllCommand = 0;
Forgot to delete these.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:111: -1,
Add // Separator comment here and line 116?

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:126:
arraysize(single_panel_menu),
You could store the sizes in vars up above where you define the  menu arrays to
avoid computing arraysize every time.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:143: // When we
minimize one panel, "Minimize all" remain intact.
s/remain/remains

[jianli, 2011-05-11 18:31:30]

Will commit with all the issues addressed.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.cc:23: #include
"views/controls/menu/menu_2.h"
On 2011/05/11 18:06:10, jennb wrote:
> I meant this goes below the next line.

Done.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.cc:559:
options_menu_contents_->AddItem(
On 2011/05/11 18:06:10, jennb wrote:
> It would be easier to read if you did:
> if (should_restore_all)
>   ..AddItem(..., ...GetString...);
> else
>   ...AddItem(...,...GetString...);

Done.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.cc:574: if
(rebuild_menu_items)
On 2011/05/11 18:06:10, jennb wrote:
> Feels like this method should return true if options_menu needs to be rebuilt.

> Then in CreateOptionsMenu(), call options_menu_->Rebuild() if needed. 
Accessing
> options_menu_ here confuses me when we return from this method to line 528
which
> checks if options_menu_ even exists.

Done. I also renamed CreateOptionsMenu to CreateOrUpdateOptionsMenu and
CreateOptionsMenuItems to CreateOrUpdateOptionsMenuItems. In addition, I moved
the creation of options_menu_contents_ to CreateOrUpdateOptionsMenuItems.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_frame_view.h (right):

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_frame_view.h:100: static const int
kMinimizeAllCommand = 0;
On 2011/05/11 18:06:10, jennb wrote:
> Forgot to delete these.

Done.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:111: -1,
On 2011/05/11 18:06:10, jennb wrote:
> Add // Separator comment here and line 116?

Done.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:126:
arraysize(single_panel_menu),
On 2011/05/11 18:06:10, jennb wrote:
> You could store the sizes in vars up above where you define the  menu arrays
to
> avoid computing arraysize every time.

Done.

http://codereview.chromium.org/7005006/diff/6003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:143: // When we
minimize one panel, "Minimize all" remain intact.
On 2011/05/11 18:06:10, jennb wrote:
> s/remain/remains

Done.