Convert NetworkMenu from Menu2 to MenuItemView.

Convert NetworkMenu from Menu2 to MenuItemView.

This CL depends on http://codereview.chromium.org/6931039/ to handle changes to MenuItemView menus while the menu is open.  It includes a default minimum width setting on the menu to avoid unnecessary resizing.

I have tested network names with ampersands and they display correctly with the same escaping code as before.

BUG=chromium-os:13887, chromium-os:12988
TEST=none


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

[rhashimoto, 2011-05-16 19:20:48]

Hi Steven -

This is another try at converting the NetworkMenu to from Menu2 to MenuItemView.
 The previous attempt (http://codereview.chromium.org/6811025/) ran afoul of
MenuItemView not handling changes while the menu was displayed.  I addressed
that in http://codereview.chromium.org/6931039/.

One question I had was whether NetworkMenuButton::OnNetworkChanged() and
NetworkDropdownButton::OnNetworkChanged() are guaranteed to be called into the
UI thread.  If that is not the case then I will need to repost the current
NetworkMenu::UpdateMenu() internals into that thread.

Thanks,
Roy

[rhashimoto, 2011-05-18 15:57:42]

Hi Steven, Charlie -

Steven, did you add Charlie as a reviewer for this CL?  Someone apparently did
but I don't think any email went out to that effect.

Thanks,
Roy

On 2011/05/16 19:20:48, rhashimoto wrote:
> Hi Steven -
> 
> This is another try at converting the NetworkMenu to from Menu2 to
MenuItemView.
>  The previous attempt (http://codereview.chromium.org/6811025/) ran afoul of
> MenuItemView not handling changes while the menu was displayed.  I addressed
> that in http://codereview.chromium.org/6931039/.
> 
> One question I had was whether NetworkMenuButton::OnNetworkChanged() and
> NetworkDropdownButton::OnNetworkChanged() are guaranteed to be called into the
> UI thread.  If that is not the case then I will need to repost the current
> NetworkMenu::UpdateMenu() internals into that thread.
> 
> Thanks,
> Roy

[stevenjb, 2011-05-18 17:36:14]

I added charlie in case he had a chance to review this before I did. (He is also
very familiar with the network menu code).

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:90: bool ConnectToNetworkAt(int
index,
I don't believe the return value for this is used any more, we should eliminate
it.

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:206: int command_id) OVERRIDE;
nit: use same indentation style for above functions.

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:224: int command_id) OVERRIDE;
nit: use same indentation style for above functions.

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:243: int command_id) OVERRIDE;
nit: use same indentation style for above functions.

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:275: const int old_count =
menu->CreateSubmenu()->child_count();
CreateSubmenu() here is confusing since we are just removing items. We should
use GetSubmenu() here and wrap it in 'if (HasSubmenu())'

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:303: }
nit: use same indentation style for each if clause.

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:897: return font ? *font:
views::MenuDelegate::GetLabelFont(id);
nit: ' ' after * and before :

[rhashimoto, 2011-05-18 17:40:47]

Patch 5 guarantees that changes to the menu items are done in the UI thread (by
checking the thread and reposting if necessary).  If we are guaranteed that the
network observers are always called in the UI thread then it might be
unnecessary, but this should make it safe regardless.

Roy

[rhashimoto, 2011-05-18 18:15:45]

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:90: bool ConnectToNetworkAt(int
index,
On 2011/05/18 17:36:15, Steven Bennetts wrote:
> I don't believe the return value for this is used any more, we should
eliminate
> it.

Done.

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:206: int command_id) OVERRIDE;
On 2011/05/18 17:36:15, Steven Bennetts wrote:
> nit: use same indentation style for above functions.

Done.

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:224: int command_id) OVERRIDE;
On 2011/05/18 17:36:15, Steven Bennetts wrote:
> nit: use same indentation style for above functions.

Done.

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:243: int command_id) OVERRIDE;
On 2011/05/18 17:36:15, Steven Bennetts wrote:
> nit: use same indentation style for above functions.

Done.

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:275: const int old_count =
menu->CreateSubmenu()->child_count();
On 2011/05/18 17:36:15, Steven Bennetts wrote:
> CreateSubmenu() here is confusing since we are just removing items. We should
> use GetSubmenu() here and wrap it in 'if (HasSubmenu())'

Done.

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:303: }
On 2011/05/18 17:36:15, Steven Bennetts wrote:
> nit: use same indentation style for each if clause.

Done.

http://codereview.chromium.org/7016042/diff/4002/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:897: return font ? *font:
views::MenuDelegate::GetLabelFont(id);
On 2011/05/18 17:36:15, Steven Bennetts wrote:
> nit: ' ' after * and before :

Done.

[stevenjb, 2011-05-18 18:43:36]

+oshima - do we need the proxying? It seems like UpdateMenu() would always be
called on the UI thread and we could just CHECK() that?

http://codereview.chromium.org/7016042/diff/8008/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/7016042/diff/8008/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:365: if
(wifi->IsPassphraseRequired()) {
else if

http://codereview.chromium.org/7016042/diff/8008/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:394:
cros->ConnectToCellularNetwork(cellular);
else { }

http://codereview.chromium.org/7016042/diff/8008/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:418: if
(vpn->NeedMoreInfoToConnect()) {
else if

http://codereview.chromium.org/7016042/diff/8008/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:421:
cros->ConnectToVirtualNetwork(vpn);
else { }

[rhashimoto, 2011-05-19 00:36:54]

> +oshima - do we need the proxying? It seems like UpdateMenu() would always be
> called on the UI thread and we could just CHECK() that?

NetworkMenuButton implements NetworkDeviceObserver, NetworkManagerObserver,
NetworkObserver, and CellularDataPlanObserver.  NetworkDropdownButton only
implements NetworkManagerObserver.

NetworkDeviceObserver is called on the UI thread
(NetworkLibraryImpl::UpdateNetworkDeviceStatus() checks).
NetworkObserver is called on the UI thread
(NetworkLibraryImpl::UpdateNetworkStatus() checks).
NetworkManagerObserver is called on the UI thread
(NetworkLibraryImpl::NotifyNetworkManagerChanged() checks).

CellularDataPlanObserver is the only interface that does not currently have a
check to ensure it is on the UI thread.  MonitorCellularDataPlan is implemented
slightly differently inside libcros than MonitorNetworkManager,
MonitorNetworkService, and MonitorNetworkDevice (using DataPlanUpdateHandler
instead of PropertyChangedHandler).  I'm guessing it comes out on the UI thread,
too, but I can't tell for sure.

Oshima-san, do you have any insight on this?

Thanks,
Roy

http://codereview.chromium.org/7016042/diff/8008/chrome/browser/chromeos/stat...
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/7016042/diff/8008/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:365: if
(wifi->IsPassphraseRequired()) {
On 2011/05/18 18:43:36, Steven Bennetts wrote:
> else if

Yikes, thanks for catching these.  Done.

http://codereview.chromium.org/7016042/diff/8008/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:394:
cros->ConnectToCellularNetwork(cellular);
On 2011/05/18 18:43:36, Steven Bennetts wrote:
> else { }

Done.

http://codereview.chromium.org/7016042/diff/8008/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:418: if
(vpn->NeedMoreInfoToConnect()) {
On 2011/05/18 18:43:36, Steven Bennetts wrote:
> else if

Done.

http://codereview.chromium.org/7016042/diff/8008/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:421:
cros->ConnectToVirtualNetwork(vpn);
On 2011/05/18 18:43:36, Steven Bennetts wrote:
> else { }

Done.

[stevenjb, 2011-05-19 01:31:05]

All DBus calls need to happen on the UI thread, so CellularDataPlanObserver
calls should also always be on the UI thread. I would just do a CHECK that we
are running on the UI thread instead of the nicely implemented, but I think
unnecessary, proxy code.

[rhashimoto, 2011-05-19 16:05:56]

On 2011/05/19 01:31:05, Steven Bennetts wrote:
> All DBus calls need to happen on the UI thread, so CellularDataPlanObserver
> calls should also always be on the UI thread. I would just do a CHECK that we
> are running on the UI thread instead of the nicely implemented, but I think
> unnecessary, proxy code.

Oshima-san concurs with Steven on every point (he stopped by just before your
message was sent).  I have replaced the proxying with a DCHECK to ensure
UpdateMenu() is invoked on the UI thread.

Roy

[stevenjb, 2011-05-19 20:13:05]

LGTM with nit

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

http://codereview.chromium.org/7016042/diff/21001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:292: SkBitmap icon;
nit: 2 space indentation even with {} (see below)

[rhashimoto, 2011-05-20 00:36:41]

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

http://codereview.chromium.org/7016042/diff/21001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:292: SkBitmap icon;
On 2011/05/19 20:13:05, Steven Bennetts wrote:
> nit: 2 space indentation even with {} (see below)

Done.

[rhashimoto, 2011-05-23 16:23:24]

Hi Zel -

Can you please review Patch 13, which sets the network menu vertical spacing to
the values I showed you on Friday.

Thanks,
Roy

[zel, 2011-05-23 16:33:37]

On 2011/05/23 16:23:24, rhashimoto wrote:
> Hi Zel -
> 
> Can you please review Patch 13, which sets the network menu vertical spacing
to
> the values I showed you on Friday.
> 
> Thanks,
> Roy

LGTM

[commit-bot: I haz the power, 2011-05-24 16:32:53]

Change committed as 86434

[Nikita (slow), 2011-05-30 11:32:53]

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

http://codereview.chromium.org/7016042/diff/24001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:65: class NetworkMenuModel :
public views::MenuDelegate {
What was the reason for dropping ui::MenuModel interface?

I'm looking into integrating HTML/JS menu that oshima implemented at the WebUI
welcome page.

I'm looking into same ui::MenuModel to have implementation independent
representation of network menu model.

In case of WebUI menu implementation I think initial version wouldn't support
submenus.

[Nikita (slow), 2011-05-30 11:45:03]

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

http://codereview.chromium.org/7016042/diff/24001/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:415: // NetworkMenuModel,
ui::MenuModel implementation:
Note, comment is incorrect now.

menu_delegate.h has 
// TODO(sky): merge this with ui::MenuModel.

[rhashimoto1, 2011-05-31 15:36:36]

On Mon, May 30, 2011 at 4:32 AM,  <nkostylev@chromium.org> wrote:
http://codereview.chromium.org/7016042/diff/24001/chrome/browser/chromeos/sta...
> chrome/browser/chromeos/status/network_menu.cc:65: class
> NetworkMenuModel : public views::MenuDelegate {
> What was the reason for dropping ui::MenuModel interface?

It was only that the views::MenuItemView menu implementation uses
views::MenuDelegate rather than ui::MenuModel.  At the time I thought
that MenuDelegate was the latest menu definition interface but I have
since been told that ui::MenuModel is newer.  I have written an
adapter class that wraps a ui::MenuModel in a views::MenuDelegate
interface but it is not quite ready to go yet.

Roy