components: add wifi_sync component

components/wifi_sync/network_state_helper_chromeos.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/wifi_sync/network_state_helper_chromeos.h"
+
+#include "base/logging.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
+#include "components/wifi_sync/wifi_credential.h"
+#include "components/wifi_sync/wifi_security_class.h"
+
+namespace wifi_sync {
+
+WifiCredential::CredentialSet GetWifiCredentialsForShillProfile(
+    chromeos::NetworkStateHandler* network_state_handler,
+    const std::string& shill_profile_path) {
+  if (!network_state_handler) {
+    LOG(ERROR) << "network_state_handler is null";

[stevenjb, 2014/12/05 20:28:17]

Should this ever happen? Maybe just DCHECK?

[mukesh agrawal, 2014/12/05 23:48:11]

Done.

+    return WifiCredential::MakeSet();
+  }
+
+  chromeos::NetworkStateHandler::NetworkStateList networks;
+  network_state_handler->GetNetworkListByType(
+      chromeos::NetworkTypePattern::WiFi(),
+      true,  /* configured_only */

[stevenjb, 2014/12/05 20:28:17]

nit: Either use /* */ inside the comma, or // outside the comma (and align). I
think the former is preferred.

[mukesh agrawal, 2014/12/05 23:48:11]

Done.

I wasn't sure if alignment applied to /**/ or not. I've followed the pattern
used in network_state_handler.cc.

+      false,  /* visible_only */
+      0,  /* unlimited result size */
+      &networks);
+
+  auto credentials(WifiCredential::MakeSet());
+  for (const chromeos::NetworkState* network : networks) {
+    DCHECK(network);

[stevenjb, 2014/12/05 20:28:17]

Unnecessary.

[mukesh agrawal, 2014/12/05 23:48:11]

Done.

+
+    if (network->profile_path() != shill_profile_path) {
+      continue;
+    }

[stevenjb, 2014/12/05 20:28:17]

nit: no {}

[mukesh agrawal, 2014/12/05 23:48:10]

Done.

+
+    // TODO(quiche): Switch away from network->security(), once we have
+    // a security_class() field in NetworkState.

[stevenjb, 2014/12/05 20:28:17]

Is there any reason why NetworkState should know about and track "security
class"?

[mukesh agrawal, 2014/12/05 23:48:11]

Good question. When we start adding networks, we want kSecurityWpa and
kSecurityRsn networks to be interoperable. shill's SecurityClass property could
solve this problem by collapsing these two security types into just
kSecurityPsk.

Alternatively, I could incorporate this logic into
WifiSecurityClassFromShillSecurity. Let me know which you prefer.

[stevenjb, 2014/12/08 17:54:26]

I looked through the Chrome code. We already have a function,
shill_property_util::GetSecurityClass that maps Wpa and Rsn to Psk. This code
could use that function for now, and Shill could stop differentiating (i.e. map
Rns and Wpa to Security or a new SecurityClass which NetworkState could switch
to) at any time.

[mukesh agrawal, 2014/12/09 01:23:50]

Acknowledged. Migration to SecurityClass is underway in crbug.com/440032.

+    //
+    // TODO(quiche): Fill in the actual passphrase via an asynchronous
+    // call to a chromeos::NetworkConfigurationHandler instance's
+    // GetProperties method.

[stevenjb, 2014/12/05 20:28:17]

I don't think that Shill provides the passphrase via GetProperties. I thought
that the plan was to only sync newly entered passphrases (available via
NetworkConfigurationObserver::OnConfigurationCreated or OnPropertiesSet)?

[mukesh agrawal, 2014/12/05 23:48:11]

Correct on both counts.

The plan is still to only sync newly entered passphrases. This helper function
is intended to be called by tests, to verify that local network state has been
updated correctly. Part of that is checking that the passphrase has been set
correctly.

I believe FakeShillServiceClient retains all the properties set on it. My plan
was to take advantage of that, to allow tests to verify that the passphrase is
correct.

(Side note: In this CL, the helper function is only called from one place. That
might argue for putting this code in wifi_credentials_helper_chromeos.cc. But in
the next CL, I use the same code from the WifiConfigDelegate tests.)

[stevenjb, 2014/12/08 17:54:26]

Anything that depends on Fake behavior should clearly be designated as test only
code, either in the name of the method (e.g. append ForTest) or by putting the
behavior in test only code. This is fine I guess for now, I'll look more closely
in the follow up CLs.

[mukesh agrawal, 2014/12/09 01:23:50]

Acknowledged. Opened crbug.com/440206 for renaming the function.

+    credentials.insert(
+        WifiCredential(network->raw_ssid(),
+                       WifiSecurityClassFromShillString(network->security()),
+                       ""  /* empty passphrase */));
+  }
+  return credentials;
+}
+
+}  // namespace wifi_sync