Show hardware addresses for wifi and ethernet networks.

Show hardware addresses for wifi and ethernet networks.

Network menu (in status area button) is modified in following way:
In oobe and login screen we add "More..." submenu and show hardware addresses in submenu.
In login screen we additionally show proxy settings in this submenu.

In browser this menu is unchanged because:
(1) we assume that there is no need in those hardware addresses since login was successful.
(2) user can see them via other means.

We add some methods (GetWifiDevice and GetEthernetDevice) to login_library because we need to obtain hardware addresses for ethernet and wifi even if there is no visible wi-fi networks and if ethernet is disconected.

BUG=chromium-os:13010
TEST=Manual

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

[Denis Lagno, 2011-04-22 18:40:04]

please take a look.

And meanwhile I'd appreciate any hint on why tests (like
NetworkScreenTest.Cellular) are failing.

[Denis Lagno, 2011-04-24 20:38:04]

tests are OK.

Adding rhashimoto@ as reviewer to check merge with his recent change.

BTW, Roy, have you tested your change on debug build?

[Nikita (slow), 2011-04-25 08:40:17]

http://codereview.chromium.org/6899025/diff/4011/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6899025/diff/4011/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:11110: <message
name="IDS_STATUSBAR_NETWORK_MORE" desc="Message for last link in the network
menu requesting more technical and less common stuff (proxy settings, IP and
hardware addresses)">
Please reuse IDS_LANGUAGES_MORE string instead.
This one being added after string freeze won't be translated for R12.

[Nikita (slow), 2011-04-25 08:51:35]

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:992: // Returns random device
with TYPE_ETHERNET. Returns NULL if none exists.
"Returns random device with TYPE_ETHERNET" is misleading.
Typically we'll have only one device of each type: Ethernet/Wifi/Cellular.

In this particular case I see that you return vector of the devices. I think
that's not needed. Through the NetworkLibrary implementation you could see that
overall assumption is that only one device of each type exists.
I think if we connect multiple adaptors, they might work but things like
wifi_enabled(), wifi_connected() support only one device/network etc.

[stevenjb, 2011-04-25 17:54:27]

http://codereview.chromium.org/6899025/diff/4011/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6899025/diff/4011/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:11110: <message
name="IDS_STATUSBAR_NETWORK_MORE" desc="Message for last link in the network
menu requesting more technical and less common stuff (proxy settings, IP and
hardware addresses)">
On 2011/04/25 08:40:17, Nikita Kostylev wrote:
> Please reuse IDS_LANGUAGES_MORE string instead.
> This one being added after string freeze won't be translated for R12.
This should be it's own string, since 'More... [networks]' might not be
translated the same as 'More... [languages]'. We just need to email the UI leads
if we decide to merge this into R12.

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:2224:
rv.push_back(iter->second);
return iter->second (first ethernet device; see comment in header).

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:2234:
rv.push_back(iter->second);
return iter->second (first wifi device; see comment in header).

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:2855: HardwareAddressFormat
format) {
DCHECK(hardware_address)

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:2881: if (i != 0 && i % 2 == 0)
nit: if ((i != 0) && (i % 2 == 0))

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:2885:
hardware_address->swap(output);
hardware_address = output;  (no reason to swap)

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:992: // Returns random device
with TYPE_ETHERNET. Returns NULL if none exists.
On 2011/04/25 08:51:35, Nikita Kostylev wrote:
> "Returns random device with TYPE_ETHERNET" is misleading.
> Typically we'll have only one device of each type: Ethernet/Wifi/Cellular.
> 
> In this particular case I see that you return vector of the devices. I think
> that's not needed. Through the NetworkLibrary implementation you could see
that
> overall assumption is that only one device of each type exists.
> I think if we connect multiple adaptors, they might work but things like
> wifi_enabled(), wifi_connected() support only one device/network etc.

In the future, there may be hardware more than one device of a given type.
However, for now I agree that we should keep that out of the interface
until/unless we need it, and make all of the Find[TYPE]Device functions return a
pointer to the first device matching TYPE (not a random device).

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

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:964: MenuItemVector link_items_;
'link_items' for local variable name

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:965: MenuItemVector
address_items_;
'address_items' for local variable name

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:1014: }
Since we list ethernet networks first, shouldn't we list ethernet hardware
addresses first?
Also, any reason not to show cellular device hardware address?

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:1017:
menu_items_.swap(link_items_);
manu_items_ = link_items;  (no need for swap)

[Nikita (slow), 2011-04-25 18:54:37]

http://codereview.chromium.org/6899025/diff/4011/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6899025/diff/4011/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:11110: <message
name="IDS_STATUSBAR_NETWORK_MORE" desc="Message for last link in the network
menu requesting more technical and less common stuff (proxy settings, IP and
hardware addresses)">
On 2011/04/25 17:54:27, Steven Bennetts wrote:
> On 2011/04/25 08:40:17, Nikita Kostylev wrote:
> > Please reuse IDS_LANGUAGES_MORE string instead.
> > This one being added after string freeze won't be translated for R12.
> This should be it's own string, since 'More... [networks]' might not be
> translated the same as 'More... [languages]'. We just need to email the UI
leads
> if we decide to merge this into R12.

I know, but we most likely going to merge it to R12, thus it won't have
translations. My point is to have it as a temporary measure - reusing similar
string message. That one is used in the same menu situation and most likely is
translated the same.

[stevenjb (google-dont-use), 2011-04-25 23:34:40]

I spoke to Kan and he was OK with re-using the other "More..." string
(although apparently there is some discussion about changing the design not
to use that).

Regardless, we agreed that it would be best to commit this correctly to
trunk, and implement the hack (i.e. using the language menu "More..."
string) when merging to the branch.

On Mon, Apr 25, 2011 at 11:54 AM, <nkostylev@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6899025/diff/4011/chrome/app/generated_resourc...
> File chrome/app/generated_resources.grd (right):
>
>
>
http://codereview.chromium.org/6899025/diff/4011/chrome/app/generated_resourc...
> chrome/app/generated_resources.grd:11110: <message
> name="IDS_STATUSBAR_NETWORK_MORE" desc="Message for last link in the
> network menu requesting more technical and less common stuff (proxy
> settings, IP and hardware addresses)">
> On 2011/04/25 17:54:27, Steven Bennetts wrote:
>
>> On 2011/04/25 08:40:17, Nikita Kostylev wrote:
>> > Please reuse IDS_LANGUAGES_MORE string instead.
>> > This one being added after string freeze won't be translated for
>>
> R12.
>
>> This should be it's own string, since 'More... [networks]' might not
>>
> be
>
>> translated the same as 'More... [languages]'. We just need to email
>>
> the UI leads
>
>> if we decide to merge this into R12.
>>
>
> I know, but we most likely going to merge it to R12, thus it won't have
> translations. My point is to have it as a temporary measure - reusing
> similar string message. That one is used in the same menu situation and
> most likely is translated the same.
>
>
> http://codereview.chromium.org/6899025/
>

[rhashimoto, 2011-04-26 00:29:51]

On 2011/04/24 20:38:04, Denis Lagno wrote:
> Adding rhashimoto@ as reviewer to check merge with his recent change.
> 
> BTW, Roy, have you tested your change on debug build?

I reverted http://codereview.chromium.org/6811025/ to look at the crash.  I
don't understand it yet - the stack trace shows that after the menu is posted,
the second mouse press tries to repost the menu.  It will take me some time to
unravel it - seems like Nikita's proposal to hold my CL out would be best.

[Denis Lagno, 2011-04-26 15:54:58]

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.cc (right):

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:2224:
rv.push_back(iter->second);
On 2011/04/25 17:54:27, Steven Bennetts wrote:
> return iter->second (first ethernet device; see comment in header).

Done.

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:2234:
rv.push_back(iter->second);
On 2011/04/25 17:54:27, Steven Bennetts wrote:
> return iter->second (first wifi device; see comment in header).

Done.

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:2855: HardwareAddressFormat
format) {
On 2011/04/25 17:54:27, Steven Bennetts wrote:
> DCHECK(hardware_address)

Done.

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:2881: if (i != 0 && i % 2 == 0)
On 2011/04/25 17:54:27, Steven Bennetts wrote:
> nit: if ((i != 0) && (i % 2 == 0))

Done.

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.cc:2885:
hardware_address->swap(output);
On 2011/04/25 17:54:27, Steven Bennetts wrote:
> hardware_address = output;  (no reason to swap)

Done.

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/network_library.h (right):

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/network_library.h:992: // Returns random device
with TYPE_ETHERNET. Returns NULL if none exists.
On 2011/04/25 17:54:27, Steven Bennetts wrote:
> On 2011/04/25 08:51:35, Nikita Kostylev wrote:
> > "Returns random device with TYPE_ETHERNET" is misleading.
> > Typically we'll have only one device of each type: Ethernet/Wifi/Cellular.
> > 
> > In this particular case I see that you return vector of the devices. I think
> > that's not needed. Through the NetworkLibrary implementation you could see
> that
> > overall assumption is that only one device of each type exists.
> > I think if we connect multiple adaptors, they might work but things like
> > wifi_enabled(), wifi_connected() support only one device/network etc.
> 
> In the future, there may be hardware more than one device of a given type.
> However, for now I agree that we should keep that out of the interface
> until/unless we need it, and make all of the Find[TYPE]Device functions return
a
> pointer to the first device matching TYPE (not a random device).

Done.

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

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:964: MenuItemVector link_items_;
On 2011/04/25 17:54:27, Steven Bennetts wrote:
> 'link_items' for local variable name

Done.

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:965: MenuItemVector
address_items_;
On 2011/04/25 17:54:27, Steven Bennetts wrote:
> 'address_items' for local variable name

Done.

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:1014: }
On 2011/04/25 17:54:27, Steven Bennetts wrote:
> Since we list ethernet networks first, shouldn't we list ethernet hardware
> addresses first?

Done.

> Also, any reason not to show cellular device hardware address?

We do not see why it may be needed.
Would add it if the need will arise.

http://codereview.chromium.org/6899025/diff/4011/chrome/browser/chromeos/stat...
chrome/browser/chromeos/status/network_menu.cc:1017:
menu_items_.swap(link_items_);
On 2011/04/25 17:54:27, Steven Bennetts wrote:
> manu_items_ = link_items;  (no need for swap)

Done.

[stevenjb, 2011-04-26 17:05:08]

LGTM with nits

http://codereview.chromium.org/6899025/diff/10002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/network_screen_browsertest.cc (right):

http://codereview.chromium.org/6899025/diff/10002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/network_screen_browsertest.cc:79:
EXPECT_CALL(*mock_network_library_, FindWifiDevice()).Times(AnyNumber());
nit: we generally put each .Times, etc on a new line regardless of whether or
not it fits (easier to read).

http://codereview.chromium.org/6899025/diff/10002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_browsertest.cc (right):

http://codereview.chromium.org/6899025/diff/10002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_browsertest.cc:67:
EXPECT_CALL(*mock_network_library_, FindWifiDevice()).Times(AnyNumber());
nit: we generally put each .Times, etc on a new line regardless of whether or
not it fits (easier to read).

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

http://codereview.chromium.org/6899025/diff/10002/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:610: }
This was designed to be general purpose, but since we only have two sub menus,
and we already have an exception for more_menu_model_, I think it would be more
clear just to call vpn_menu_model_->InitMenuItems() explicitly up where we
insert it and remove this loop?

[Denis Lagno, 2011-04-26 18:04:09]

http://codereview.chromium.org/6899025/diff/10002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/network_screen_browsertest.cc (right):

http://codereview.chromium.org/6899025/diff/10002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/network_screen_browsertest.cc:79:
EXPECT_CALL(*mock_network_library_, FindWifiDevice()).Times(AnyNumber());
On 2011/04/26 17:05:08, Steven Bennetts wrote:
> nit: we generally put each .Times, etc on a new line regardless of whether or
> not it fits (easier to read).

Done.

http://codereview.chromium.org/6899025/diff/10002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/update_screen_browsertest.cc (right):

http://codereview.chromium.org/6899025/diff/10002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/update_screen_browsertest.cc:67:
EXPECT_CALL(*mock_network_library_, FindWifiDevice()).Times(AnyNumber());
On 2011/04/26 17:05:08, Steven Bennetts wrote:
> nit: we generally put each .Times, etc on a new line regardless of whether or
> not it fits (easier to read).

Done.

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

http://codereview.chromium.org/6899025/diff/10002/chrome/browser/chromeos/sta...
chrome/browser/chromeos/status/network_menu.cc:610: }
On 2011/04/26 17:05:08, Steven Bennetts wrote:
> This was designed to be general purpose, but since we only have two sub menus,
> and we already have an exception for more_menu_model_, I think it would be
more
> clear just to call vpn_menu_model_->InitMenuItems() explicitly up where we
> insert it and remove this loop?

that's reasonable.  Done.

[zel, 2011-04-26 21:50:08]

http://codereview.chromium.org/6899025/diff/12004/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6899025/diff/12004/chrome/app/generated_resour...
chrome/app/generated_resources.grd:11104: More...
there is already string like this one - IDS_LANGUAGES_MORE

can we use that instead to avoid TC changes?

[Denis Lagno, 2011-04-26 22:26:52]

http://codereview.chromium.org/6899025/diff/12004/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6899025/diff/12004/chrome/app/generated_resour...
chrome/app/generated_resources.grd:11104: More...
On 2011/04/26 21:50:08, zel wrote:
> there is already string like this one - IDS_LANGUAGES_MORE
> 
> can we use that instead to avoid TC changes?

I can make this adjustment for branch.

[zel, 2011-04-26 22:44:35]

LGTM then


On Tue, Apr 26, 2011 at 3:26 PM, <dilmah@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6899025/diff/12004/chrome/app/generated_resour...
> File chrome/app/generated_resources.grd (right):
>
>
>
http://codereview.chromium.org/6899025/diff/12004/chrome/app/generated_resour...
> chrome/app/generated_resources.grd:11104: More...
> On 2011/04/26 21:50:08, zel wrote:
>
>> there is already string like this one - IDS_LANGUAGES_MORE
>>
>
>  can we use that instead to avoid TC changes?
>>
>
> I can make this adjustment for branch.
>
>
> http://codereview.chromium.org/6899025/
>