Add 3G Activation to the network menu.

Add 3G Activation to the network menu.
This includes some cleanup of the network_menu code to work with or without DOMUI menus.

BUG=http://code.google.com/p/chromium-os/issues/detail?id=8114
TEST=All of the network menu

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

[stevenjb, 2010-10-26 23:35:08]

http://codereview.chromium.org/4136002/diff/6001/7002
File chrome/browser/chromeos/cros/cros_mock.cc (left):

http://codereview.chromium.org/4136002/diff/6001/7002#oldcode312
chrome/browser/chromeos/cros/cros_mock.cc:312:
.WillRepeatedly((ReturnRef(wifi_networks_)))
Changed InitMenuItems to wrap all wifi and cellular on available() calls, so as
long as these return false, the other calls will not be made. This makes it
easier to change the logic in the wifi and cellular handlers without changing
the expectations each time.

[Charlie Lee, 2010-10-27 00:23:13]

http://codereview.chromium.org/4136002/diff/6001/7003
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/4136002/diff/6001/7003#newcode580
chrome/browser/chromeos/status/network_menu.cc:580:
IconForDisplay(*rb.GetBitmapNamed(IDR_STATUSBAR_NETWORK_BARS0),
I checked in a change that will conflict here. the id should now be BARS0_BLACK

http://codereview.chromium.org/4136002/diff/6001/7003#newcode617
chrome/browser/chromeos/status/network_menu.cc:617: if
(ShouldOpenButtonOptions()) {
What's the reasoning behind removing the cros->Connected() check? I think that
it was there for OOBE. Not sure though.

http://codereview.chromium.org/4136002/diff/6001/7003#newcode662
chrome/browser/chromeos/status/network_menu.cc:662: if (use_settings_ui_) {
is this missing "&& domui_menus_" or is there a reason for not having this here.

[stevenjb, 2010-10-27 17:39:06]

http://codereview.chromium.org/4136002/diff/6001/7003
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/4136002/diff/6001/7003#newcode580
chrome/browser/chromeos/status/network_menu.cc:580:
IconForDisplay(*rb.GetBitmapNamed(IDR_STATUSBAR_NETWORK_BARS0),
On 2010/10/27 00:23:13, Charlie Lee wrote:
> I checked in a change that will conflict here. the id should now be
BARS0_BLACK

Thanks for the heads-up. Rebased.

http://codereview.chromium.org/4136002/diff/6001/7003#newcode617
chrome/browser/chromeos/status/network_menu.cc:617: if
(ShouldOpenButtonOptions()) {
On 2010/10/27 00:23:13, Charlie Lee wrote:
> What's the reasoning behind removing the cros->Connected() check? I think that
> it was there for OOBE. Not sure though.

I'm not sure what Connected() was supposed to do here. If it is false than it
wouldn't have any affect. If it is true and ShouldOpenButtonOptions() is false,
then the menu would have an extra separator at the end, which seemed odd to me.
Do you know who we can add to the review list from the OOBE team to look at
this?

http://codereview.chromium.org/4136002/diff/6001/7003#newcode662
chrome/browser/chromeos/status/network_menu.cc:662: if (use_settings_ui_) {
On 2010/10/27 00:23:13, Charlie Lee wrote:
> is this missing "&& domui_menus_" or is there a reason for not having this
here.

We only want to show NetworkConfigView to input passwords, not if we are already
connected, so the extra check wasn't needed here, except that I was missing some
other logic, so good catch. I've changed the logic in all of these
appropriately. Hopefully it is readable enough. It will be nice when we settle
on one menu type or the other and we can clean all of this up...

[oshima, 2010-10-27 17:44:05]

http://codereview.chromium.org/4136002/diff/14001/15003
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/4136002/diff/14001/15003#newcode223
chrome/browser/chromeos/status/network_menu.cc:223: ShowWifi(wifi, true);
(I believe) you should have {} in this case.

http://codereview.chromium.org/4136002/diff/14001/15004
File chrome/browser/chromeos/status/network_menu.h (right):

http://codereview.chromium.org/4136002/diff/14001/15004#newcode214
chrome/browser/chromeos/status/network_menu.h:214: bool domui_menus_;
can you use MenuUI::IsEnabled instead?

[stevenjb, 2010-10-27 18:07:26]

http://codereview.chromium.org/4136002/diff/14001/15003
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/4136002/diff/14001/15003#newcode223
chrome/browser/chromeos/status/network_menu.cc:223: ShowWifi(wifi, true);
On 2010/10/27 17:44:06, oshima wrote:
> (I believe) you should have {} in this case. 
Done.

http://codereview.chromium.org/4136002/diff/14001/15004
File chrome/browser/chromeos/status/network_menu.h (right):

http://codereview.chromium.org/4136002/diff/14001/15004#newcode214
chrome/browser/chromeos/status/network_menu.h:214: bool domui_menus_;
On 2010/10/27 17:44:06, oshima wrote:
> can you use MenuUI::IsEnabled instead?
Will do; that's more readable than checking the switch (which is why I added the
memebr variable).

[oshima, 2010-10-27 18:57:15]

http://codereview.chromium.org/4136002/diff/19001/20003
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/4136002/diff/19001/20003#newcode423
chrome/browser/chromeos/status/network_menu.cc:423: void
NetworkMenu::InitMenuItems() {
Looks like this method is quite long. May be it's time to refactor into
InitEtherner, InitWifi, and InitCellular?

http://codereview.chromium.org/4136002/diff/19001/20003#newcode448
chrome/browser/chromeos/status/network_menu.cc:448: label =
l10n_util::GetStringUTF16(IDS_STATUSBAR_NETWORK_DEVICE_ETHERNET);
{}

http://codereview.chromium.org/4136002/diff/19001/20003#newcode524
chrome/browser/chromeos/status/network_menu.cc:524: label =
ASCIIToUTF16(cell_networks[i].name());
{}

[Charlie Lee, 2010-10-27 19:12:25]

http://codereview.chromium.org/4136002/diff/6001/7003
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/4136002/diff/6001/7003#newcode617
chrome/browser/chromeos/status/network_menu.cc:617: if
(ShouldOpenButtonOptions()) {
Well i tracked it down. Turns out you know most about this:
http://codereview.chromium.org/3675002/diff/7002/20007

I guess you are just cleaning up after yourself. :)

On 2010/10/27 17:39:06, Steven Bennetts wrote:
> On 2010/10/27 00:23:13, Charlie Lee wrote:
> > What's the reasoning behind removing the cros->Connected() check? I think
that
> > it was there for OOBE. Not sure though.
> 
> I'm not sure what Connected() was supposed to do here. If it is false than it
> wouldn't have any affect. If it is true and ShouldOpenButtonOptions() is
false,
> then the menu would have an extra separator at the end, which seemed odd to
me.
> Do you know who we can add to the review list from the OOBE team to look at
> this?
>

[stevenjb, 2010-10-27 19:28:26]

On 2010/10/27 19:12:25, Charlie Lee wrote:
> http://codereview.chromium.org/4136002/diff/6001/7003
> File chrome/browser/chromeos/status/network_menu.cc (right):
> 
> http://codereview.chromium.org/4136002/diff/6001/7003#newcode617
> chrome/browser/chromeos/status/network_menu.cc:617: if
> (ShouldOpenButtonOptions()) {
> Well i tracked it down. Turns out you know most about this:
> http://codereview.chromium.org/3675002/diff/7002/20007
> 
> I guess you are just cleaning up after yourself. :)
> 
Ah, yes, sure enough, sloppy of me!

[stevenjb, 2010-10-27 19:28:47]

http://codereview.chromium.org/4136002/diff/19001/20003
File chrome/browser/chromeos/status/network_menu.cc (right):

http://codereview.chromium.org/4136002/diff/19001/20003#newcode423
chrome/browser/chromeos/status/network_menu.cc:423: void
NetworkMenu::InitMenuItems() {
On 2010/10/27 18:57:15, oshima wrote:
> Looks like this method is quite long. May be it's time to refactor into
> InitEtherner, InitWifi, and InitCellular?
I could, but I would rather wait until we've picked a direction for the network
menu, or at least committed to tab settings? That will allow for some additional
cleanup that could be done at the same time.

http://codereview.chromium.org/4136002/diff/19001/20003#newcode448
chrome/browser/chromeos/status/network_menu.cc:448: label =
l10n_util::GetStringUTF16(IDS_STATUSBAR_NETWORK_DEVICE_ETHERNET);
On 2010/10/27 18:57:15, oshima wrote:
> {}

Done.

http://codereview.chromium.org/4136002/diff/19001/20003#newcode524
chrome/browser/chromeos/status/network_menu.cc:524: label =
ASCIIToUTF16(cell_networks[i].name());
On 2010/10/27 18:57:15, oshima wrote:
> {}

Done.

[stevenjb, 2010-10-27 19:32:42]

Uploaded.

[Charlie Lee, 2010-10-27 19:34:16]

LGTM

[oshima, 2010-10-27 20:04:37]

LGTM

On Wed, Oct 27, 2010 at 12:28 PM, <stevenjb@chromium.org> wrote:

>
> http://codereview.chromium.org/4136002/diff/19001/20003
>
> File chrome/browser/chromeos/status/network_menu.cc (right):
>
> http://codereview.chromium.org/4136002/diff/19001/20003#newcode423
> chrome/browser/chromeos/status/network_menu.cc:423: void
> NetworkMenu::InitMenuItems() {
>
> On 2010/10/27 18:57:15, oshima wrote:
>
>> Looks like this method is quite long. May be it's time to refactor
>>
> into
>
>> InitEtherner, InitWifi, and InitCellular?
>>
> I could, but I would rather wait until we've picked a direction for the
> network menu, or at least committed to tab settings? That will allow for
> some additional cleanup that could be done at the same time.
>
> I think this should be refactored regardless, but I understand exactly how
can depend on the direction. Can you add TODO to refactor?


>
> http://codereview.chromium.org/4136002/diff/19001/20003#newcode448
> chrome/browser/chromeos/status/network_menu.cc:448: label =
> l10n_util::GetStringUTF16(IDS_STATUSBAR_NETWORK_DEVICE_ETHERNET);
> On 2010/10/27 18:57:15, oshima wrote:
>
>> {}
>>
>
> Done.
>
>
> http://codereview.chromium.org/4136002/diff/19001/20003#newcode524
> chrome/browser/chromeos/status/network_menu.cc:524: label =
> ASCIIToUTF16(cell_networks[i].name());
> On 2010/10/27 18:57:15, oshima wrote:
>
>> {}
>>
>
> Done.
>
>
> http://codereview.chromium.org/4136002/show
>