Change status button menu implementation from Menu2 to MenuItemView.

Change status button menu (ClockMenu, NetworkMenu, PowerMenu) implementation from Menu2 to MenuItemView.

This required modifying MenuItemView to support per-item fonts, which are needed to display the currently associated network in boldface.

This CL depends on http://codereview.chromium.org/6870016/ to work properly on the ChromeOS login screen.

BUG=chromium-os:13887
TEST=none


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

[oshima, 2011-04-08 17:21:17]

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/clock_menu_button.cc:20: using
views::MenuItemView;
move this nito chromeos namespace.

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/clock_menu_button.cc:30: } // anonymous namespace
two space before //
just "namespace"

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/clock_menu_button.cc:105: break;
just use if
I assume else case shouldn't happen? If so, add log/DCHECK

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/clock_menu_button.cc:115: return false;
same here.

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/clock_menu_button.cc:126:
host_->OpenButtonOptions(this);
same here

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/clock_menu_button.cc:170: gfx::Rect
bounds(screen_loc, source->size());
will RTL work with this?

[rhashimoto, 2011-04-14 00:58:54]

I expanded the scope of the CL from just ClockMenuButton so that all the status
button menus would be updated at once.

Roy

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/clock_menu_button.cc:20: using
views::MenuItemView;
On 2011/04/08 17:21:17, oshima wrote:
> move this nito chromeos namespace.

The using declaration or the anonymous namespace contents?  If the latter, then
it's done.

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/clock_menu_button.cc:30: } // anonymous namespace
On 2011/04/08 17:21:17, oshima wrote:
> two space before //
> just "namespace"

Done.

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/clock_menu_button.cc:105: break;
On 2011/04/08 17:21:17, oshima wrote:
> just use if
> I assume else case shouldn't happen? If so, add log/DCHECK

Done.

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/clock_menu_button.cc:115: return false;
On 2011/04/08 17:21:17, oshima wrote:
> same here.

This one is disabled only for CLOCK_DISPLAY_ITEM.  I changed it to if-else.

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/clock_menu_button.cc:126:
host_->OpenButtonOptions(this);
On 2011/04/08 17:21:17, oshima wrote:
> same here

Done.

http://codereview.chromium.org/6811025/diff/6001/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/clock_menu_button.cc:170: gfx::Rect
bounds(screen_loc, source->size());
On 2011/04/08 17:21:17, oshima wrote:
> will RTL work with this?

I don't know, but this is how the WrenchMenu is implemented with MenuItemView.

[oshima, 2011-04-14 17:27:31]

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:39: #endif
can you move these (enum to kFontSizeDelta) into anonymous namespace?

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:98: DCHECK(id ==
CLOCK_DISPLAY_ITEM);
DCHECK_EQ

note: expected value first

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:106: if (id ==
CLOCK_DISPLAY_ITEM)
return id == CLOCK_OPEN_OPTIONS_ITEM

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:114: DCHECK(id ==
CLOCK_OPEN_OPTIONS_ITEM);
DCHECK_EQ

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:137: DCHECK(source == this);
DCHECK_EQ

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:156: }
It's probably better to separate above code. I'm fine with either

MaybeInitMenu() (or EnsureMenu()) and check menu_.get() inside, 

or

if (!menu_.get()) {
   InitMenu();
}

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:164: // to a NativeWindow
that we can pass to MenuItemView::RunMenuAt().
won't this work?

source->GetWidget()->GetToplevelWidget()->GetNativeView()

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.h (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.h:35: virtual std::wstring
GetLabel(int id) const;
include compiler_specific.h and add

OVERRIDE

(original code didn't have this because this is new)

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_dropdown_button.h (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_dropdown_button.h:39: virtual bool
IsBrowserMode() const { return browser_mode_; }
OVERRIDE

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:265: DCHECK(id >= 0 && id <
static_cast<int>(menu_items_.size()));
use two DCHECK_LE (or _GT)

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:280: return false;
can this happen? or should we use DCHECK?

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:284: DCHECK(id >= 0 && id <
static_cast<int>(menu_items_.size()));
same here

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:289: DCHECK(id >= 0 && id <
static_cast<int>(menu_items_.size()));
same here

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:340: {
move { to previous ine

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:533: DCHECK(source ==
GetMenuButton());
DCHECK_EQ

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.h (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.h:89: virtual std::wstring
GetLabel(int id) const;
OVERRIDE

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/power_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:27: };
move this (and const below) to anonymous namespace

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:53: {
move { to previous line

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:64: break;
no need for break (chrome tends to prefer shorter description of two identical.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:67: return
UTF16ToWide(label);
or you can simply return UTF16ToWide(Get...)

it's up to you.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:71: {
same here

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/power_menu_button.h (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.h:33: virtual std::wstring
GetLabel(int id) const;
OVERRIDE

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_co...
File views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_co...
views/controls/menu/menu_controller.cc:310: #if !defined(OS_CHROMEOS)
This looks suspicious. Let's discuss offline.

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_de...
File views/controls/menu/menu_delegate.h (right):

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_de...
views/controls/menu/menu_delegate.h:64: virtual const gfx::Font*
GetLabelFont(int id) const;
const gfx::Font&

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_it...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_it...
views/controls/menu/menu_item_view.cc:591: {
move { to previous line

[rhashimoto, 2011-04-14 18:27:04]

I can chat about the MenuController reference counting issue whenever it's
convenient for you.

Roy

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:39: #endif
On 2011/04/14 17:27:31, oshima wrote:
> can you move these (enum to kFontSizeDelta) into anonymous namespace?

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:98: DCHECK(id ==
CLOCK_DISPLAY_ITEM);
On 2011/04/14 17:27:31, oshima wrote:
> DCHECK_EQ
> 
> note: expected value first

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:106: if (id ==
CLOCK_DISPLAY_ITEM)
On 2011/04/14 17:27:31, oshima wrote:
> return id == CLOCK_OPEN_OPTIONS_ITEM

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:114: DCHECK(id ==
CLOCK_OPEN_OPTIONS_ITEM);
On 2011/04/14 17:27:31, oshima wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:137: DCHECK(source == this);
On 2011/04/14 17:27:31, oshima wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:156: }
On 2011/04/14 17:27:31, oshima wrote:
> It's probably better to separate above code. I'm fine with either
> 
> MaybeInitMenu() (or EnsureMenu()) and check menu_.get() inside, 
> 
> or
> 
> if (!menu_.get()) {
>    InitMenu();
> }
> 
> 

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:164: // to a NativeWindow
that we can pass to MenuItemView::RunMenuAt().
On 2011/04/14 17:27:31, oshima wrote:
> won't this work?
> 
> source->GetWidget()->GetToplevelWidget()->GetNativeView() 

That gets the NativeView, but we have to pass a NativeWindow to
MenuItemView::RunMenuAt(), and I don't see a way to convert a NativeView to a
NativeWindow in a platform-independent manner.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.h (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.h:35: virtual std::wstring
GetLabel(int id) const;
On 2011/04/14 17:27:31, oshima wrote:
> include compiler_specific.h and add
> 
> OVERRIDE
> 
> (original code didn't have this because this is new)

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_dropdown_button.h (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_dropdown_button.h:39: virtual bool
IsBrowserMode() const { return browser_mode_; }
On 2011/04/14 17:27:31, oshima wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:265: DCHECK(id >= 0 && id <
static_cast<int>(menu_items_.size()));
On 2011/04/14 17:27:31, oshima wrote:
> use two DCHECK_LE (or _GT)

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:280: return false;
On 2011/04/14 17:27:31, oshima wrote:
> can this happen? or should we use DCHECK?

This can happen.  This method is called with 0 before any menu items are added. 
Perhaps for showing the empty menu?

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:284: DCHECK(id >= 0 && id <
static_cast<int>(menu_items_.size()));
On 2011/04/14 17:27:31, oshima wrote:
> same here

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:289: DCHECK(id >= 0 && id <
static_cast<int>(menu_items_.size()));
On 2011/04/14 17:27:31, oshima wrote:
> same here

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:340: {
On 2011/04/14 17:27:31, oshima wrote:
> move { to previous ine

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:533: DCHECK(source ==
GetMenuButton());
On 2011/04/14 17:27:31, oshima wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.h (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.h:89: virtual std::wstring
GetLabel(int id) const;
On 2011/04/14 17:27:31, oshima wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/power_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:27: };
On 2011/04/14 17:27:31, oshima wrote:
> move this (and const below) to anonymous namespace

enum moved.  The const below is a class static member.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:53: {
On 2011/04/14 17:27:31, oshima wrote:
> move { to previous line

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:64: break;
On 2011/04/14 17:27:31, oshima wrote:
> no need for break (chrome tends to prefer shorter description of two
identical.

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:67: return
UTF16ToWide(label);
On 2011/04/14 17:27:31, oshima wrote:
> or you can simply return UTF16ToWide(Get...)
> 
> it's up to you.

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:71: {
On 2011/04/14 17:27:31, oshima wrote:
> same here

Done.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/power_menu_button.h (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.h:33: virtual std::wstring
GetLabel(int id) const;
On 2011/04/14 17:27:31, oshima wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_co...
File views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_co...
views/controls/menu/menu_controller.cc:310: #if !defined(OS_CHROMEOS)
On 2011/04/14 17:27:31, oshima wrote:
> This looks suspicious. Let's discuss offline.

Sounds like a good idea.

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_de...
File views/controls/menu/menu_delegate.h (right):

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_de...
views/controls/menu/menu_delegate.h:64: virtual const gfx::Font*
GetLabelFont(int id) const;
On 2011/04/14 17:27:31, oshima wrote:
> const gfx::Font&

That wouldn't allow subclasses a convenient way to specify the default font,
currently done by returning NULL.  Should we add a GetDefaultFont() for them to
return?

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_it...
File views/controls/menu/menu_item_view.cc (right):

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_it...
views/controls/menu/menu_item_view.cc:591: {
On 2011/04/14 17:27:31, oshima wrote:
> move { to previous line

Done.

[oshima, 2011-04-15 17:40:11]

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:164: // to a NativeWindow
that we can pass to MenuItemView::RunMenuAt().
On 2011/04/14 18:27:04, rhashimoto wrote:
> On 2011/04/14 17:27:31, oshima wrote:
> > won't this work?
> > 
> > source->GetWidget()->GetToplevelWidget()->GetNativeView() 
> 
> That gets the NativeView, but we have to pass a NativeWindow to
> MenuItemView::RunMenuAt(), and I don't see a way to convert a NativeView to a
> NativeWindow in a platform-independent manner.

NativeView is gtk_windonw instance in gtk_widget type, so you can do the
same as

window = GTK_WINDOW(source->GetWidget()->GetToplevelWidget()->GetNativeView() 
)

Yes, this part is a bit mess, and I think menu API should simply takes either
view or Widget and let implementation do the details. (outside of this CL's
scope)

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:174: #endif
no need for ifdefs as ths is only for chromeos

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_de...
File views/controls/menu/menu_delegate.h (right):

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_de...
views/controls/menu/menu_delegate.h:64: virtual const gfx::Font*
GetLabelFont(int id) const;
On 2011/04/14 18:27:04, rhashimoto wrote:
> On 2011/04/14 17:27:31, oshima wrote:
> > const gfx::Font&
> 
> That wouldn't allow subclasses a convenient way to specify the default font,
> currently done by returning NULL.  Should we add a GetDefaultFont() for them
to
> return?

Ah i see. Hmm, then pointer may be fine. Alternative way is to return the
default
font in base class. It can eliminate one if in the menu_item_view.

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:134: {
move { to pervious line. (and other places)

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:135:
DCHECK_EQ(CLOCK_DISPLAY_ITEM,id);
space after ,

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:203: }
Looks like we can move this to StatusAreaButton (assuming all status area button
has a menu). Window switcher button is currently StatusAreaButton, but this does
not seem to be necessary. What do you think?

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:157: DCHECK_LE(0,id);
space after , . same for the rest

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.h (right):

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.h:143: virtual views::MenuButton*
GetMenuButton() = 0;
where are we using this?

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/power_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:34: const int
PowerMenuButton::kNumPowerImages = 16;
looks like this can move to anonymous namespace (no need to be static member)

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:87: DCHECK(source == this);
DCHECK_EQ

[rhashimoto, 2011-04-15 21:20:27]

I reverted the change to MenuController that modified ref counting behavior for
ChromeOS.  We will depend on the login screen holding a ref count on the message
loop.  I tested Nikita's suggestion to ref count in BaseLoginDisplayHost - that
worked fine and I will submit that in a separate CL (and not commit this one
until that one is in).

Roy

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:164: // to a NativeWindow
that we can pass to MenuItemView::RunMenuAt().
On 2011/04/15 17:40:12, oshima wrote:
> On 2011/04/14 18:27:04, rhashimoto wrote:
> > On 2011/04/14 17:27:31, oshima wrote:
> > > won't this work?
> > > 
> > > source->GetWidget()->GetToplevelWidget()->GetNativeView() 
> > 
> > That gets the NativeView, but we have to pass a NativeWindow to
> > MenuItemView::RunMenuAt(), and I don't see a way to convert a NativeView to
a
> > NativeWindow in a platform-independent manner.
> 
> NativeView is gtk_windonw instance in gtk_widget type, so you can do the
> same as
> 
> window = GTK_WINDOW(source->GetWidget()->GetToplevelWidget()->GetNativeView() 
> )

I think this is fundamentally what I've done, i.e. casting NativeView using the
GTK_WINDOW casting macro.  I'm not quite sure if you want something different
here.

> Yes, this part is a bit mess, and I think menu API should simply takes either
> view or Widget and let implementation do the details. (outside of this CL's
> scope)

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:174: #endif
On 2011/04/15 17:40:12, oshima wrote:
> no need for ifdefs as ths is only for chromeos

Done.  I was thinking that the #ifdefs signalled the platform-specific ugliness
in this supposedly platform-independent class, but perhaps the TODO is signal
enough.

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_co...
File views/controls/menu/menu_controller.cc (right):

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_co...
views/controls/menu/menu_controller.cc:310: #if !defined(OS_CHROMEOS)
On 2011/04/14 18:27:04, rhashimoto wrote:
> On 2011/04/14 17:27:31, oshima wrote:
> > This looks suspicious. Let's discuss offline.
> 
> Sounds like a good idea.

Reverted to original file.  The login screen should maintain a reference count
on the message loop.

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_de...
File views/controls/menu/menu_delegate.h (right):

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_de...
views/controls/menu/menu_delegate.h:64: virtual const gfx::Font*
GetLabelFont(int id) const;
On 2011/04/15 17:40:12, oshima wrote:
> On 2011/04/14 18:27:04, rhashimoto wrote:
> > On 2011/04/14 17:27:31, oshima wrote:
> > > const gfx::Font&
> > 
> > That wouldn't allow subclasses a convenient way to specify the default font,
> > currently done by returning NULL.  Should we add a GetDefaultFont() for them
> to
> > return?
> 
> Ah i see. Hmm, then pointer may be fine. Alternative way is to return the
> default
> font in base class. It can eliminate one if in the menu_item_view.

If we put the default in the base class then subclasses will need to call the
base class for the default font (fonts can be set per menu item):

const gfx::Font* Subclass::GetLabelFont(int id) const {
  if (id == ITEM_WITH_SPECIAL_FONT)
    return GetSpecialFontPtr();
  else
    return MenuDelegate::GetLabelFont(id);
}

I think it's easier for subclasses to use a default of NULL.

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:134: {
On 2011/04/15 17:40:12, oshima wrote:
> move { to pervious line. (and other places)

Done.

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:135:
DCHECK_EQ(CLOCK_DISPLAY_ITEM,id);
On 2011/04/15 17:40:12, oshima wrote:
> space after ,

Done.

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:203: }
On 2011/04/15 17:40:12, oshima wrote:
> Looks like we can move this to StatusAreaButton (assuming all status area
button
> has a menu). Window switcher button is currently StatusAreaButton, but this
does
> not seem to be necessary. What do you think?

If we were going to consolidate it, I think I'd put it on MenuButton (the
superclass of StatusAreaButton).  I'd rather put that refactoring in a separate
CL, though, because otherwise I'd have to put all the other menus that need
conversion to MenuItemView in this CL.

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:157: DCHECK_LE(0,id);
On 2011/04/15 17:40:12, oshima wrote:
> space after , . same for the rest

Done.

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.h (right):

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.h:143: virtual views::MenuButton*
GetMenuButton() = 0;
On 2011/04/15 17:40:12, oshima wrote:
> where are we using this?

NetworkMenu is a superclass for NetworkMenuButton (for the status area) and
NetworkDropdownButton (for the network configuration dialog).  The subclasses
instantiate different menu buttons that the base class needs to pass to
MenuItemView::RunMenuAt().  GetMenuButton() provides the mechanism for the base
class to get the MenuButton* from the subclasses, used in
NetworkMenu::RunMenu().

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/power_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:34: const int
PowerMenuButton::kNumPowerImages = 16;
On 2011/04/15 17:40:12, oshima wrote:
> looks like this can move to anonymous namespace (no need to be static member)

Done.

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/power_menu_button.cc:87: DCHECK(source == this);
On 2011/04/15 17:40:12, oshima wrote:
> DCHECK_EQ

Done.

[oshima, 2011-04-15 22:26:51]

mostly looks good. You will need ben or sky to check in changes to menu.

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:280: return false;
On 2011/04/14 18:27:04, rhashimoto wrote:
> On 2011/04/14 17:27:31, oshima wrote:
> > can this happen? or should we use DCHECK?
> 
> This can happen.  This method is called with 0 before any menu items are
added. 
> Perhaps for showing the empty menu?

Hmm, that sounds odd. Can you check why this is happening?

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_de...
File views/controls/menu/menu_delegate.h (right):

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_de...
views/controls/menu/menu_delegate.h:64: virtual const gfx::Font*
GetLabelFont(int id) const;
On 2011/04/15 21:20:27, rhashimoto wrote:
> On 2011/04/15 17:40:12, oshima wrote:
> > On 2011/04/14 18:27:04, rhashimoto wrote:
> > > On 2011/04/14 17:27:31, oshima wrote:
> > > > const gfx::Font&
> > > 
> > > That wouldn't allow subclasses a convenient way to specify the default
font,
> > > currently done by returning NULL.  Should we add a GetDefaultFont() for
them
> > to
> > > return?
> > 
> > Ah i see. Hmm, then pointer may be fine. Alternative way is to return the
> > default
> > font in base class. It can eliminate one if in the menu_item_view.
> 
> If we put the default in the base class then subclasses will need to call the
> base class for the default font (fonts can be set per menu item):
> 
> const gfx::Font* Subclass::GetLabelFont(int id) const {
>   if (id == ITEM_WITH_SPECIAL_FONT)
>     return GetSpecialFontPtr();
>   else
>     return MenuDelegate::GetLabelFont(id);
> }
> 
> I think it's easier for subclasses to use a default of NULL.

My preference is actually above. It guarantees that it will do the same
thing as super class does, although in this case, this is probably fine.
There were several bugs due to this pattern, subclass changing the default
behavior of super class. Since there is no guarantee that returning NULL is same
as what superclass does, except for you know when you write the code, this tends
to be error prone. (This is just FYI)

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:132: // ClockMenuButton,
views::MenuDelegate implementation:
please use new format
// ClockMenuButton, ...... : -----------------
(to 80)

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:183: window =
GTK_WINDOW(source->GetWidget()->GetNativeView());
i now understand. all you need is this. 

window = GTK_WINDOW(source->GetWidget()->GetNativeView())

they're same thing. Just different type. (and you get null for Window because
Window is not a Widget, but Widget is a Window)

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_dropdown_button.h (right):

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_dropdown_button.h:43: }
actually, can you move implementations to .cc? We're trying to eliminate
inlined virtual functions. (to reduce binary size)

[oshima, 2011-04-15 22:35:18]

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/16019/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:203: }
On 2011/04/15 21:20:27, rhashimoto wrote:
> On 2011/04/15 17:40:12, oshima wrote:
> > Looks like we can move this to StatusAreaButton (assuming all status area
> button
> > has a menu). Window switcher button is currently StatusAreaButton, but this
> does
> > not seem to be necessary. What do you think?
> 
> If we were going to consolidate it, I think I'd put it on MenuButton (the
> superclass of StatusAreaButton).  I'd rather put that refactoring in a
separate
> CL, though, because otherwise I'd have to put all the other menus that need
> conversion to MenuItemView in this CL.

sgtm

[rhashimoto, 2011-04-15 23:44:32]

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:280: return false;
On 2011/04/15 22:26:51, oshima wrote:
> On 2011/04/14 18:27:04, rhashimoto wrote:
> > On 2011/04/14 17:27:31, oshima wrote:
> > > can this happen? or should we use DCHECK?
> > 
> > This can happen.  This method is called with 0 before any menu items are
> added. 
> > Perhaps for showing the empty menu?
> 
> Hmm, that sounds odd. Can you check why this is happening?

Commands are ids, not indices, and there is one namespace for commands for each
MenuItemView hierarchy - a menu is defined by a tree of MenuItemView instances
with the root instance a container for the rest.

As each MenuItemView instance is initialized, the delegate is queried to see if
it is enabled.  We get a query for the root instance (command id 0) before the
rest of the menu items are populated.

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_de...
File views/controls/menu/menu_delegate.h (right):

http://codereview.chromium.org/6811025/diff/15001/views/controls/menu/menu_de...
views/controls/menu/menu_delegate.h:64: virtual const gfx::Font*
GetLabelFont(int id) const;
On 2011/04/15 22:26:51, oshima wrote:
> On 2011/04/15 21:20:27, rhashimoto wrote:
> > On 2011/04/15 17:40:12, oshima wrote:
> > > On 2011/04/14 18:27:04, rhashimoto wrote:
> > > > On 2011/04/14 17:27:31, oshima wrote:
> > > > > const gfx::Font&
> > > > 
> > > > That wouldn't allow subclasses a convenient way to specify the default
> font,
> > > > currently done by returning NULL.  Should we add a GetDefaultFont() for
> them
> > > to
> > > > return?
> > > 
> > > Ah i see. Hmm, then pointer may be fine. Alternative way is to return the
> > > default
> > > font in base class. It can eliminate one if in the menu_item_view.
> > 
> > If we put the default in the base class then subclasses will need to call
the
> > base class for the default font (fonts can be set per menu item):
> > 
> > const gfx::Font* Subclass::GetLabelFont(int id) const {
> >   if (id == ITEM_WITH_SPECIAL_FONT)
> >     return GetSpecialFontPtr();
> >   else
> >     return MenuDelegate::GetLabelFont(id);
> > }
> > 
> > I think it's easier for subclasses to use a default of NULL.
> 
> My preference is actually above. It guarantees that it will do the same
> thing as super class does, although in this case, this is probably fine.
> There were several bugs due to this pattern, subclass changing the default
> behavior of super class. Since there is no guarantee that returning NULL is
same
> as what superclass does, except for you know when you write the code, this
tends
> to be error prone. (This is just FYI)
> 

Done.

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:132: // ClockMenuButton,
views::MenuDelegate implementation:
On 2011/04/15 22:26:51, oshima wrote:
> please use new format
> // ClockMenuButton, ...... : -----------------
> (to 80)

Done.

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:183: window =
GTK_WINDOW(source->GetWidget()->GetNativeView());
On 2011/04/15 22:26:51, oshima wrote:
> i now understand. all you need is this. 
> 
> window = GTK_WINDOW(source->GetWidget()->GetNativeView())
> 
> they're same thing. Just different type. (and you get null for Window because
> Window is not a Widget, but Widget is a Window)

I'd like to keep the other line present:

  window = source->GetWindow()->GetNativeWindow();

This is the line that will survive when WebUI provides a proper Window at the
top of the hierarchy.  The workaround with GTK_WINDOW (and the if-else) will
eventually go away.  It will be easier to make the change if the
platform-independent code is present.

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_dropdown_button.h (right):

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_dropdown_button.h:43: }
On 2011/04/15 22:26:51, oshima wrote:
> actually, can you move implementations to .cc? We're trying to eliminate
> inlined virtual functions. (to reduce binary size)

Done.

[oshima, 2011-04-15 23:52:19]

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6811025/diff/15001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:280: return false;
On 2011/04/15 23:44:32, rhashimoto wrote:
> On 2011/04/15 22:26:51, oshima wrote:
> > On 2011/04/14 18:27:04, rhashimoto wrote:
> > > On 2011/04/14 17:27:31, oshima wrote:
> > > > can this happen? or should we use DCHECK?
> > > 
> > > This can happen.  This method is called with 0 before any menu items are
> > added. 
> > > Perhaps for showing the empty menu?
> > 
> > Hmm, that sounds odd. Can you check why this is happening?
> 
> Commands are ids, not indices, and there is one namespace for commands for
each
> MenuItemView hierarchy - a menu is defined by a tree of MenuItemView instances
> with the root instance a container for the rest.
> 
> As each MenuItemView instance is initialized, the delegate is queried to see
if
> it is enabled.  We get a query for the root instance (command id 0) before the
> rest of the menu items are populated.

Yes, but i still don't understand why we're doing it. There may be legit reason,
but curious to know why.

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:183: window =
GTK_WINDOW(source->GetWidget()->GetNativeView());
On 2011/04/15 23:44:32, rhashimoto wrote:
> On 2011/04/15 22:26:51, oshima wrote:
> > i now understand. all you need is this. 
> > 
> > window = GTK_WINDOW(source->GetWidget()->GetNativeView())
> > 
> > they're same thing. Just different type. (and you get null for Window
because
> > Window is not a Widget, but Widget is a Window)
> 
> I'd like to keep the other line present:
> 
>   window = source->GetWindow()->GetNativeWindow();
> 
> This is the line that will survive when WebUI provides a proper Window at the
> top of the hierarchy.  The workaround with GTK_WINDOW (and the if-else) will
> eventually go away.  It will be easier to make the change if the
> platform-independent code is present.

It will not survive when you move to MenuButton. Window here means "Browser"
window, but not a window in X/Win sense.

[rhashimoto, 2011-04-20 15:43:56]

Hi Scott -

Can you please have a look at this CL?  I needed to add per-item font support to
MenuItemView menus to show the currently associated network in boldface in the
ChromeOS network menus.

Thanks,
Roy

[sky, 2011-04-20 16:02:57]

The menu changes LGTM

http://codereview.chromium.org/6811025/diff/23007/views/controls/menu/menu_de...
File views/controls/menu/menu_delegate.h (right):

http://codereview.chromium.org/6811025/diff/23007/views/controls/menu/menu_de...
views/controls/menu/menu_delegate.h:64: virtual const gfx::Font&
GetLabelFont(int id) const;
Last sentence isn't right. Maybe just remove it entirely.

[rhashimoto, 2011-04-21 19:31:16]

Redid NetworkMenu because of merge collisions, and added stevenjb to reviewers.

Roy

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/clock_menu_button.cc (right):

http://codereview.chromium.org/6811025/diff/25002/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/clock_menu_button.cc:183: window =
GTK_WINDOW(source->GetWidget()->GetNativeView());
On 2011/04/15 23:52:19, oshima wrote:
> On 2011/04/15 23:44:32, rhashimoto wrote:
> > On 2011/04/15 22:26:51, oshima wrote:
> > > i now understand. all you need is this. 
> > > 
> > > window = GTK_WINDOW(source->GetWidget()->GetNativeView())
> > > 
> > > they're same thing. Just different type. (and you get null for Window
> because
> > > Window is not a Widget, but Widget is a Window)
> > 
> > I'd like to keep the other line present:
> > 
> >   window = source->GetWindow()->GetNativeWindow();
> > 
> > This is the line that will survive when WebUI provides a proper Window at
the
> > top of the hierarchy.  The workaround with GTK_WINDOW (and the if-else) will
> > eventually go away.  It will be easier to make the change if the
> > platform-independent code is present.
> 
> It will not survive when you move to MenuButton. Window here means "Browser"
> window, but not a window in X/Win sense.
> 

Done.

http://codereview.chromium.org/6811025/diff/23007/views/controls/menu/menu_de...
File views/controls/menu/menu_delegate.h (right):

http://codereview.chromium.org/6811025/diff/23007/views/controls/menu/menu_de...
views/controls/menu/menu_delegate.h:64: virtual const gfx::Font&
GetLabelFont(int id) const;
On 2011/04/20 16:02:58, sky wrote:
> Last sentence isn't right. Maybe just remove it entirely.

Done.

[stevenjb, 2011-04-21 22:25:31]

The network_menu changes lgtm.

http://codereview.chromium.org/6811025/diff/28028/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6811025/diff/28028/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:277: item.icon);
nit: slightly more readable with { }

http://codereview.chromium.org/6811025/diff/28028/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:682: item.icon);
nit: {}

http://codereview.chromium.org/6811025/diff/28028/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.h (right):

http://codereview.chromium.org/6811025/diff/28028/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.h:31: class NetworkMenuModel :
public views::MenuDelegate {
This should have come up in the CL where this was added, but there is no reason
for NetworkMenuModel to be in network_menu.h; it should be moved into
network_menu.cc and forward declared here.

[rhashimoto, 2011-04-21 22:49:57]

http://codereview.chromium.org/6811025/diff/28028/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/6811025/diff/28028/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:277: item.icon);
On 2011/04/21 22:25:31, Steven Bennetts wrote:
> nit: slightly more readable with { }

Done.

http://codereview.chromium.org/6811025/diff/28028/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:682: item.icon);
On 2011/04/21 22:25:31, Steven Bennetts wrote:
> nit: {}

Done.

http://codereview.chromium.org/6811025/diff/28028/chrome/browser/chromeos/sta...
File chrome/browser/chromeos/status/network_menu.h (right):

http://codereview.chromium.org/6811025/diff/28028/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.h:31: class NetworkMenuModel :
public views::MenuDelegate {
On 2011/04/21 22:25:31, Steven Bennetts wrote:
> This should have come up in the CL where this was added, but there is no
reason
> for NetworkMenuModel to be in network_menu.h; it should be moved into
> network_menu.cc and forward declared here.

Done.

[commit-bot: I haz the power, 2011-04-22 18:29:53]

Change committed as 82676

[Nikita (slow), 2011-04-25 10:54:02]

Are we planning to merge this into R12 branch? I think we should, otherwise it
complicates some merges, for instance:
http://codereview.chromium.org/6899025/