ChromeOS - Fix some network related strings.

ChromeOS - Fix some network related strings.

This fixes the ethernet string and removes broken/unused code for VPN
address in network info bubble.

BUG=706085
Review-Url: https://codereview.chromium.org/2781233002
Cr-Commit-Position: refs/heads/master@{#460841}
Committed: https://chromium.googlesource.com/chromium/src/+/4980bf9a7b63805fceae1fa3fb17922abb99f59a

[Evan Stade, 2017-03-30 02:40:26]

Description was changed from

==========
ChromeOS - Fix some network related strings.

BUG=706085
==========

to

==========
ChromeOS - Fix some network related strings.

This fixes the ethernet string and a related bug for VPN networks.

BUG=706085
==========

[Evan Stade, 2017-03-30 02:40:26]

estade@chromium.org changed reviewers:
+ stevenjb@chromium.org

[Evan Stade, 2017-03-30 02:42:50]

https://codereview.chromium.org/2781233002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
(right):

https://codereview.chromium.org/2781233002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:425:
handler->FormattedHardwareAddressForType(NetworkTypePattern::VPN());
as far as I can tell this always returns null. Steven, you added this (VPN
address line) in [1]. Is it possible for a VPN service to have an address? If
so, perhaps it is an oversight that FakeShillManagerClient doesn't set an
address. If not, we can delete this code and the VPN_ADDRESS string.

[1] https://codereview.chromium.org/12387065

[stevenjb, 2017-03-30 17:10:05]

https://codereview.chromium.org/2781233002/diff/1/ash/common/system/chromeos/...
File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
(right):

https://codereview.chromium.org/2781233002/diff/1/ash/common/system/chromeos/...
ash/common/system/chromeos/network/network_state_list_detailed_view.cc:425:
handler->FormattedHardwareAddressForType(NetworkTypePattern::VPN());
On 2017/03/30 02:42:50, Evan Stade wrote:
> as far as I can tell this always returns null. Steven, you added this (VPN
> address line) in [1]. Is it possible for a VPN service to have an address? If
> so, perhaps it is an oversight that FakeShillManagerClient doesn't set an
> address. If not, we can delete this code and the VPN_ADDRESS string.
> 
> [1] https://codereview.chromium.org/12387065

Don't rely on the fake implementations for 100% correct Shill behavior, that
would be a monumental effort :) We add support as we need it for testing.

That said, a VPN should have an IP address (network->ip_address()), but not a
hardware (MAC) address (there is no associated device). I don't recall the
history of this code. It's possible it has simply always been wrong and nobody
noticed.

[Evan Stade, 2017-03-30 17:26:25]

On 2017/03/30 17:10:05, stevenjb wrote:
>
https://codereview.chromium.org/2781233002/diff/1/ash/common/system/chromeos/...
> File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
> (right):
> 
>
https://codereview.chromium.org/2781233002/diff/1/ash/common/system/chromeos/...
> ash/common/system/chromeos/network/network_state_list_detailed_view.cc:425:
> handler->FormattedHardwareAddressForType(NetworkTypePattern::VPN());
> On 2017/03/30 02:42:50, Evan Stade wrote:
> > as far as I can tell this always returns null. Steven, you added this (VPN
> > address line) in [1]. Is it possible for a VPN service to have an address?
If
> > so, perhaps it is an oversight that FakeShillManagerClient doesn't set an
> > address. If not, we can delete this code and the VPN_ADDRESS string.
> > 
> > [1] https://codereview.chromium.org/12387065
> 
> Don't rely on the fake implementations for 100% correct Shill behavior, that
> would be a monumental effort :) We add support as we need it for testing.
> 
> That said, a VPN should have an IP address (network->ip_address()), but not a
> hardware (MAC) address (there is no associated device). I don't recall the
> history of this code. It's possible it has simply always been wrong and nobody
> noticed.

Are you suggesting we should show the ip address then?

[stevenjb, 2017-03-30 17:45:17]

On 2017/03/30 17:26:25, Evan Stade wrote:
> On 2017/03/30 17:10:05, stevenjb wrote:
> >
>
https://codereview.chromium.org/2781233002/diff/1/ash/common/system/chromeos/...
> > File ash/common/system/chromeos/network/network_state_list_detailed_view.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/2781233002/diff/1/ash/common/system/chromeos/...
> > ash/common/system/chromeos/network/network_state_list_detailed_view.cc:425:
> > handler->FormattedHardwareAddressForType(NetworkTypePattern::VPN());
> > On 2017/03/30 02:42:50, Evan Stade wrote:
> > > as far as I can tell this always returns null. Steven, you added this (VPN
> > > address line) in [1]. Is it possible for a VPN service to have an address?
> If
> > > so, perhaps it is an oversight that FakeShillManagerClient doesn't set an
> > > address. If not, we can delete this code and the VPN_ADDRESS string.
> > > 
> > > [1] https://codereview.chromium.org/12387065
> > 
> > Don't rely on the fake implementations for 100% correct Shill behavior, that
> > would be a monumental effort :) We add support as we need it for testing.
> > 
> > That said, a VPN should have an IP address (network->ip_address()), but not
a
> > hardware (MAC) address (there is no associated device). I don't recall the
> > history of this code. It's possible it has simply always been wrong and
nobody
> > noticed.
> 
> Are you suggesting we should show the ip address then?

Yeah, we should show just the IP address for VPN, but the logic is actually
broken. I'm going to put up a quick fix for that, but here you can just remove
vpn_address entirely.

[Evan Stade, 2017-03-30 18:16:09]

Description was changed from

==========
ChromeOS - Fix some network related strings.

This fixes the ethernet string and a related bug for VPN networks.

BUG=706085
==========

to

==========
ChromeOS - Fix some network related strings.

This fixes the ethernet string and removes broken/unused code for VPN
address in network info bubble.

BUG=706085
==========

[Evan Stade, 2017-03-30 18:16:51]

updated, ptal

[Evan Stade, 2017-03-30 18:16:54]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 18:17:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stevenjb, 2017-03-30 18:29:05]

lgtm

[Evan Stade, 2017-03-30 18:40:24]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2017-03-30 18:40:29]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-03-30 18:41:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 19:01:06]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1490899229010770,
"parent_rev": "4178c1385df3da6a8ba8dc1a0adb06f628a9ebdc", "commit_rev":
"4980bf9a7b63805fceae1fa3fb17922abb99f59a"}

[commit-bot: I haz the power, 2017-03-30 19:06:11]

Description was changed from

==========
ChromeOS - Fix some network related strings.

This fixes the ethernet string and removes broken/unused code for VPN
address in network info bubble.

BUG=706085
==========

to

==========
ChromeOS - Fix some network related strings.

This fixes the ethernet string and removes broken/unused code for VPN
address in network info bubble.

BUG=706085

Review-Url: https://codereview.chromium.org/2781233002
Cr-Commit-Position: refs/heads/master@{#460841}
Committed:
https://chromium.googlesource.com/chromium/src/+/4980bf9a7b63805fceae1fa3fb17...
==========

[commit-bot: I haz the power, 2017-03-30 19:06:12]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/4980bf9a7b63805fceae1fa3fb17...