wifi_sync: add function to create a network in Shill

wifi_sync: add function to create a network in Shill

We're going to need to create networks from multiple places,
so let's put the code someplace where it can be re-used.

(The two places we'll need this from are a) WifiConfigDelegate,
which creates networks in the normal case, and b) sync integration
tests.)

BUG=chromium:431435
TEST=components_unittests --gtest_filter="Wifi*"

[mukesh agrawal, 2015-01-07 18:37:24]

quiche@chromium.org changed reviewers:
+ erikwright@chromium.org, pstew@chromium.org, stevenjb@chromium.org

[mukesh agrawal, 2015-01-07 18:37:24]

quiche@chromium.org changed required reviewers:
+ erikwright@chromium.org, stevenjb@chromium.org

[mukesh agrawal, 2015-01-07 18:48:41]

On 2015/01/07 18:37:34, mukesh agrawal wrote:

The build failure is due to this being an incremental CL. (I didn't realize the
trybots don't have logic to pull in dependent CLs.)

[stevenjb, 2015-01-07 19:29:58]

This looks good, but we should definitely have a test for this using the Fake
Shill implementation; that will at least test ONC validation and translation to
Shill properties.

https://codereview.chromium.org/831693005/diff/1/components/wifi_sync/network...
File components/wifi_sync/network_state_helper_chromeos.cc (right):

https://codereview.chromium.org/831693005/diff/1/components/wifi_sync/network...
components/wifi_sync/network_state_helper_chromeos.cc:20:
chromeos::ManagedNetworkConfigurationHandler* managed_net_config_handler,
nit: network_config_handler is probably more clear

[stevenjb, 2015-01-07 19:30:35]

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

[stevenjb, 2015-01-07 19:30:36]

+pneubeck@

[erikwright (departed), 2015-01-07 20:07:15]

As mentioned, needs a test. Otherwise LG.

https://codereview.chromium.org/831693005/diff/1/components/wifi_sync/network...
File components/wifi_sync/network_state_helper_chromeos.h (right):

https://codereview.chromium.org/831693005/diff/1/components/wifi_sync/network...
components/wifi_sync/network_state_helper_chromeos.h:24: // asynchronously,
invoking |success_callback| or |error_callback| as
using what thread/message loop/task runner?

[mukesh agrawal, 2015-01-07 22:07:14]

On 2015/01/07 19:29:58, stevenjb wrote:
> This looks good, but we should definitely have a test for this using the Fake
> Shill implementation; that will at least test ONC validation and translation
to
> Shill properties.

Quite true. And I'd like to get your advice on the best way to do that.

In particular, I think the test for this function would look quite a bit
like the test for WifiConfigDelegateChromeOs (see
https://codereview.chromium.org/836363002/patch/1/10010).

I don't want to just copy-paste that code, because that would be a maintenance
burden. But, on the other hand, I don't know a good way to structure the
test code, so as to share it between WifiConfigDelegateChromeOs tests,
and network_state_helper_chromeos tests.

Any suggestions?

[mukesh agrawal, 2015-01-07 22:11:45]

On 2015/01/07 22:07:14, mukesh agrawal wrote:
> In particular, I think the test for this function would look quite a bit
> like the test for WifiConfigDelegateChromeOs (see
> https://codereview.chromium.org/836363002/patch/1/10010).
> 
> I don't want to just copy-paste that code, because that would be a maintenance
> burden. But, on the other hand, I don't know a good way to structure the
> test code, so as to share it between WifiConfigDelegateChromeOs tests,
> and network_state_helper_chromeos tests.
> 
> Any suggestions?

Hmm... maybe I should revise this function, so that it doesn't actually call
CreateConfiguration. Instead, it just returns an onc_properties object, and
leaves it to the caller to actually CreateConfiguration. Then the test for the
function should be (relatively) simple.

How's that sound?

[mukesh agrawal, 2015-01-07 22:14:18]

https://codereview.chromium.org/831693005/diff/1/components/wifi_sync/network...
File components/wifi_sync/network_state_helper_chromeos.cc (right):

https://codereview.chromium.org/831693005/diff/1/components/wifi_sync/network...
components/wifi_sync/network_state_helper_chromeos.cc:20:
chromeos::ManagedNetworkConfigurationHandler* managed_net_config_handler,
On 2015/01/07 19:29:58, stevenjb wrote:
> nit: network_config_handler is probably more clear

Done.

https://codereview.chromium.org/831693005/diff/1/components/wifi_sync/network...
File components/wifi_sync/network_state_helper_chromeos.h (right):

https://codereview.chromium.org/831693005/diff/1/components/wifi_sync/network...
components/wifi_sync/network_state_helper_chromeos.h:24: // asynchronously,
invoking |success_callback| or |error_callback| as
On 2015/01/07 20:07:14, erikwright wrote:
> using what thread/message loop/task runner?

AIUI, the callback will be executed using the origin_task_runner_ of dbus::Bus.
That executes tasks on the dbus::Bus "origin thread". Per the documentation for
dbus::Bus, the origin thread is the one "that created the Bus object", and is
"usually Chrome's UI thread".

stevenjb: Please confirm or correct my understanding.
erikwright: For the function's documentation, would it be better to simply say
that the callbacks usually execute on the UI thread? Or should I give a fuller
explanation?

[stevenjb, 2015-01-07 22:31:12]

On 2015/01/07 22:11:45, mukesh agrawal wrote:
> On 2015/01/07 22:07:14, mukesh agrawal wrote:
> > In particular, I think the test for this function would look quite a bit
> > like the test for WifiConfigDelegateChromeOs (see
> > https://codereview.chromium.org/836363002/patch/1/10010).
> > 
> > I don't want to just copy-paste that code, because that would be a
maintenance
> > burden. But, on the other hand, I don't know a good way to structure the
> > test code, so as to share it between WifiConfigDelegateChromeOs tests,
> > and network_state_helper_chromeos tests.
> > 
> > Any suggestions?
> 
> Hmm... maybe I should revise this function, so that it doesn't actually call
> CreateConfiguration. Instead, it just returns an onc_properties object, and
> leaves it to the caller to actually CreateConfiguration. Then the test for the
> function should be (relatively) simple.
> 
> How's that sound?

That would be reasonable, then the test for this would just document the
expected ONC -> Shill translation and any integration tests (using the
NetworkConfigHandler + FakeShill implementations) would be associated with the
caller.

[mukesh agrawal, 2015-01-08 02:06:39]

Thanks, Steven. After some more thought, I realized that, since I'm just
converting a WifiCredential to an ONC dictionary, the code would fit better in
WifiCredential.

To that end, I've posted https://codereview.chromium.org/809803005/, and I'll
abandon this CL. (Sorry for the churn.)