wifi_sync: add ability to convert WifiCredential to onc properties

components/wifi_sync/wifi_credential.h

Index: components/wifi_sync/wifi_credential.h
diff --git a/components/wifi_sync/wifi_credential.h b/components/wifi_sync/wifi_credential.h
index 7c0342a6c7e142049e9d78854b1cf98d69cd26a9..a6d267a09d7a237d5c32b18e4f4cea301f693ee4 100644
--- a/components/wifi_sync/wifi_credential.h
+++ b/components/wifi_sync/wifi_credential.h
@@ -13,6 +13,10 @@
 
 #include "components/wifi_sync/wifi_security_class.h"
 
+namespace base {
+class DictionaryValue;
+}
+
 namespace wifi_sync {
 
 // A container to hold the information required to locate and connect
@@ -36,6 +40,12 @@ class WifiCredential final {  // final because the class is copyable
   WifiSecurityClass security_class() const { return security_class_; }
   const std::string& passphrase() const { return passphrase_; }
 
+  // Updates |onc_properties|, adding elements which represent the ONC
+  // properties for this credential. The resulting |onc_properties|
+  // can be used, e.g, to configure a new network.  On failure,
+  // returns false, and leaves |onc_properties| unmodified.
+  bool ToOncProperties(base::DictionaryValue* onc_properties) const;

[erikwright (departed), 2015/01/08 19:27:08]

It seems a bit strange to me that you can instantiate a valid WifiCredential
object that cannot be converted to OncProperties. Certainly, the fact that this
method performs additional validation is not evident from the description.

[erikwright (departed), 2015/01/08 19:27:08]

Does it make sense to add properties to an existing non-empty dictionary? If
not, consider:

scoped_ptr<DictionaryValue> ToOncProperties() const;

[mukesh agrawal, 2015/01/09 02:00:18]

It's definitely strange. We have a bug open for this (crbug.com/432546), which
pneubeck@ is working on. Until that's fixed, I've updated the method's
documentation, to note this oddity.

[mukesh agrawal, 2015/01/09 02:00:18]

Done.

Thanks for the idea. I wanted to do something like this, but switched to an out
parameter when the compiler pointed out that DictionaryValue is not copyable.
scoped_ptr is better, though. :)

+
   // Returns a string representation of the credential, for debugging
   // purposes. The string will not include the credential's passphrase.
   std::string ToString() const;
@@ -47,6 +57,11 @@ class WifiCredential final {  // final because the class is copyable
   // ordering function plumbed in.
   static CredentialSet MakeSet();
 
+  // Returns |ssid| as an SsidBytes instance. This convenience
+  // function simplifies some tests, which need to instantiate
+  // SsidBytes from string literals.
+  static SsidBytes MakeSsidBytes(const std::string& ssid);

[stevenjb, 2015/01/08 17:27:51]

If this is only used for tests, append ForTest to the end of the function name.

[erikwright (departed), 2015/01/08 19:27:07]

How will SsidBytes normally be constructed? What is the benefit of the custom
type vs. a plain-old std::string?

[mukesh agrawal, 2015/01/09 02:00:18]

Done.

[mukesh agrawal, 2015/01/09 02:00:18]

In the normal case, SsidBytes will be (copy-)constructed from a
std::vector<uint8_t>. SsidBytes is, itself, just an alias for
std::vector<uint8_t>.

The reason for introducing the custom type is to discourage people from assuming
that SSID is "text", or ASCII or UTF-8. I think people would be prone to such
assumptions if the type were std::string.

FWIW, so far, I've drafted three places where this happens:
1) when WifiCredentialSyncableService deserializes a WifiCredentialSpecifics
protobuf
2) when network_state_helper_chromeos reads raw_ssid() from a NetworkState
3) when WifiCredentialManager reads raw_ssid() from a NetworkState
(WifiCredentialManager is a new class that I haven't posted for review yet) 

[erikwright (departed), 2015/01/09 14:58:20]

Can you, in that case, please add a link or comment explaining what a valid SSID
is? Maybe right above the definition of SsidBytes?

If there is only a single test that uses this, and it's the test for this class,
I would suggest just inlining this method there. Maybe there will be others?

[mukesh agrawal, 2015/01/09 19:00:02]

Done. (Added comment about valid SSIDs.)
There will be others. E.g. in sync integration tests.

+
  private:
   // The WiFi network's SSID.
   const SsidBytes ssid_;