[CrOS Tether] Update NetworkConfigurationHandler::GetShillProperties() to work with Tether networks.

[CrOS Tether] Update NetworkConfigurationHandler::GetShillProperties() to work with Tether networks.

Previously, this function called ShillServiceClient::GetProperties() to obtain network properties, but this does not work for Tether networks since they are not managed by Shill. After this change, when GetShillProperties() is called, the properties are obtained via NetworkStateHandler.

With this change, the chrome.networkingPrivate.get{Managed}Properties() API functions return the correct value for Tether networks.

BUG=672263
Review-Url: https://codereview.chromium.org/2836083002
Cr-Commit-Position: refs/heads/master@{#467045}
Committed: https://chromium.googlesource.com/chromium/src/+/d0e89425386049c68ae2b997d4fd874e4c65b2b6

[stevenjb, 2017-04-24 17:15:51]

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

[stevenjb, 2017-04-24 17:15:52]

https://codereview.chromium.org/2836083002/diff/1/chromeos/network/managed_ne...
File chromeos/network/managed_network_configuration_handler_impl.cc (left):

https://codereview.chromium.org/2836083002/diff/1/chromeos/network/managed_ne...
chromeos/network/managed_network_configuration_handler_impl.cc:192: profile));
This always needs to be called, even if profile is null.

[Kyle Horimoto, 2017-04-24 19:48:26]

Description was changed from

==========
[CrOS Tether] ManagedNetworkConfigurationHandlerImpl

WIP - not ready for review yet.

BUG=672263
==========

to

==========
[CrOS Tether] Update NetworkConfigurationHandler::GetShillProperties() to work
with Tether networks.

Previously, this function called ShillServiceClient::GetProperties() to obtain
network properties, but this does not work for Tether networks since they are
not managed by Shill. After this change, when GetShillProperties() is called,
the properties are obtained via NetworkStateHandler.

With this change, the chrome.networkingPrivate.get{Managed}Properties() API
functions return the correct value for Tether networks.

BUG=672263
==========

[Kyle Horimoto, 2017-04-24 19:49:38]

PTAL - this CL is now ready for review.

https://codereview.chromium.org/2836083002/diff/1/chromeos/network/managed_ne...
File chromeos/network/managed_network_configuration_handler_impl.cc (left):

https://codereview.chromium.org/2836083002/diff/1/chromeos/network/managed_ne...
chromeos/network/managed_network_configuration_handler_impl.cc:192: profile));
On 2017/04/24 17:15:52, stevenjb wrote:
> This always needs to be called, even if profile is null.

Done.

[stevenjb, 2017-04-24 21:26:42]

https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/manage...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:146: if (!profile
&& !onc::IsTetherShillDictionary(*shill_properties)) {
We should avoid putting IsTetherShillDictionary() in ONC code unless the ONC
code uses it. I would just make it a file local function unless it's needed
elsewhere.

https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:234: if (network_state &&
As long as we are doing this lookup, we can fail early if network_state is null;
just invoke error_callback with a "not found" error.

[Kyle Horimoto, 2017-04-25 01:18:33]

https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/manage...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:146: if (!profile
&& !onc::IsTetherShillDictionary(*shill_properties)) {
On 2017/04/24 21:26:41, stevenjb wrote:
> We should avoid putting IsTetherShillDictionary() in ONC code unless the ONC
> code uses it. I would just make it a file local function unless it's needed
> elsewhere.

Done.

https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/networ...
File chromeos/network/network_configuration_handler.cc (right):

https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/networ...
chromeos/network/network_configuration_handler.cc:234: if (network_state &&
On 2017/04/24 21:26:41, stevenjb wrote:
> As long as we are doing this lookup, we can fail early if network_state is
null;
> just invoke error_callback with a "not found" error.

I don't think this is the right thing to do. I made this change and ran it, and
it failed on VPN networks.

It also made these two tests fail:
ManagedNetworkConfigurationHandlerTest.AutoConnectDisallowed
NetworkConfigurationHandlerTest.GetProperties

I'm going to leave it as I wrote it.

[stevenjb, 2017-04-25 17:03:41]

On 2017/04/25 01:18:33, Kyle Horimoto wrote:
>
https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/manage...
> File chromeos/network/managed_network_configuration_handler_impl.cc (right):
> 
>
https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/manage...
> chromeos/network/managed_network_configuration_handler_impl.cc:146: if
(!profile
> && !onc::IsTetherShillDictionary(*shill_properties)) {
> On 2017/04/24 21:26:41, stevenjb wrote:
> > We should avoid putting IsTetherShillDictionary() in ONC code unless the ONC
> > code uses it. I would just make it a file local function unless it's needed
> > elsewhere.
> 
> Done.
> 
>
https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/networ...
> File chromeos/network/network_configuration_handler.cc (right):
> 
>
https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/networ...
> chromeos/network/network_configuration_handler.cc:234: if (network_state &&
> On 2017/04/24 21:26:41, stevenjb wrote:
> > As long as we are doing this lookup, we can fail early if network_state is
> null;
> > just invoke error_callback with a "not found" error.
> 
> I don't think this is the right thing to do. I made this change and ran it,
and
> it failed on VPN networks.
> 
> It also made these two tests fail:
> ManagedNetworkConfigurationHandlerTest.AutoConnectDisallowed
> NetworkConfigurationHandlerTest.GetProperties
> 
> I'm going to leave it as I wrote it.

Hmm. I'm curious what "it failed on VPN networks" means exactly. The tests
probably just need fixing.

That said, we don't necessarily need to do that here, so this lgtm as-is for
now.

[Kyle Horimoto, 2017-04-25 17:07:51]

On 2017/04/25 17:03:41, stevenjb wrote:
> On 2017/04/25 01:18:33, Kyle Horimoto wrote:
> >
>
https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/manage...
> > File chromeos/network/managed_network_configuration_handler_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/manage...
> > chromeos/network/managed_network_configuration_handler_impl.cc:146: if
> (!profile
> > && !onc::IsTetherShillDictionary(*shill_properties)) {
> > On 2017/04/24 21:26:41, stevenjb wrote:
> > > We should avoid putting IsTetherShillDictionary() in ONC code unless the
ONC
> > > code uses it. I would just make it a file local function unless it's
needed
> > > elsewhere.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/networ...
> > File chromeos/network/network_configuration_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/2836083002/diff/20001/chromeos/network/networ...
> > chromeos/network/network_configuration_handler.cc:234: if (network_state &&
> > On 2017/04/24 21:26:41, stevenjb wrote:
> > > As long as we are doing this lookup, we can fail early if network_state is
> > null;
> > > just invoke error_callback with a "not found" error.
> > 
> > I don't think this is the right thing to do. I made this change and ran it,
> and
> > it failed on VPN networks.
> > 
> > It also made these two tests fail:
> > ManagedNetworkConfigurationHandlerTest.AutoConnectDisallowed
> > NetworkConfigurationHandlerTest.GetProperties
> > 
> > I'm going to leave it as I wrote it.
> 
> Hmm. I'm curious what "it failed on VPN networks" means exactly. The tests
> probably just need fixing.
> 
> That said, we don't necessarily need to do that here, so this lgtm as-is for
> now.

Regarding the "it fails on VPN networks" comment: I ran the chromeos-linux build
on my Linux desktop with the Fake Shill DBus services (not actually on a Chrome
OS device). When I did that, I got error logs saying that the VPN network was
not found when I searched with NSH. Without the change, I did not get any error
logs. I don't know much about VPN networks, so I didn't dig into it very much.

Landing as-is. Thanks for the review!

[Kyle Horimoto, 2017-04-25 17:07:55]

The CQ bit was checked by khorimoto@chromium.org

[commit-bot: I haz the power, 2017-04-25 17:08:14]

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-04-25 18:01:49]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493140075657600,
"parent_rev": "418926bbe929301fa7d3aff43d375963ae0ce205", "commit_rev":
"d0e89425386049c68ae2b997d4fd874e4c65b2b6"}

[commit-bot: I haz the power, 2017-04-25 18:01:59]

Description was changed from

==========
[CrOS Tether] Update NetworkConfigurationHandler::GetShillProperties() to work
with Tether networks.

Previously, this function called ShillServiceClient::GetProperties() to obtain
network properties, but this does not work for Tether networks since they are
not managed by Shill. After this change, when GetShillProperties() is called,
the properties are obtained via NetworkStateHandler.

With this change, the chrome.networkingPrivate.get{Managed}Properties() API
functions return the correct value for Tether networks.

BUG=672263
==========

to

==========
[CrOS Tether] Update NetworkConfigurationHandler::GetShillProperties() to work
with Tether networks.

Previously, this function called ShillServiceClient::GetProperties() to obtain
network properties, but this does not work for Tether networks since they are
not managed by Shill. After this change, when GetShillProperties() is called,
the properties are obtained via NetworkStateHandler.

With this change, the chrome.networkingPrivate.get{Managed}Properties() API
functions return the correct value for Tether networks.

BUG=672263

Review-Url: https://codereview.chromium.org/2836083002
Cr-Commit-Position: refs/heads/master@{#467045}
Committed:
https://chromium.googlesource.com/chromium/src/+/d0e89425386049c68ae2b997d4fd...
==========

[commit-bot: I haz the power, 2017-04-25 18:02:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d0e89425386049c68ae2b997d4fd...