wifi_sync: add ability to convert WifiCredential to onc properties

components/wifi_sync/wifi_credential.h

 // 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.
 
 #ifndef COMPONENTS_WIFI_SYNC_WIFI_CREDENTIAL_H_
 #define COMPONENTS_WIFI_SYNC_WIFI_CREDENTIAL_H_
 
 #include <stdint.h>
 
 #include <set>
 #include <string>
 #include <vector>
 
 #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
 // to a WiFi network.
 class WifiCredential final {  // final because the class is copyable

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

Is it your intention to permit use of the default constructor?

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

No, I don't expect there will be a need for default construction.

  public:
   using SsidBytes = std::vector<uint8_t>;
   using CredentialSet = std::set<
       WifiCredential,
       bool(*)(const WifiCredential&a, const WifiCredential& b)>;
 
   // Constructs a credential with the given |ssid|, |security_class|,
   // and |passphrase|. No assumptions are made about the input

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

You now make an assumption that it is UTF-8.

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

See below (comment on ToOncProperties).

   // encoding of |ssid|. The passphrase must be valid UTF-8.
   WifiCredential(const SsidBytes& ssid,
                  WifiSecurityClass security_class,
                  const std::string& passphrase);
   ~WifiCredential();
 
   const SsidBytes& ssid() const { return ssid_; }
   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;
 
   // Returns true if credential |a| comes before credential |b|.
   static bool IsLessThan(const WifiCredential& a, const WifiCredential& b);
 
   // Returns an empty set of WifiCredentials, with the IsLessThan
   // 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_;
   // The WiFi network's security class (e.g. WEP, PSK).
   const WifiSecurityClass security_class_;
   // The passphrase for connecting to the network.
   const std::string passphrase_;
 };
 
 }  // namespace wifi_sync
 
 #endif  // COMPONENTS_WIFI_SYNC_WIFI_CREDENTIAL_H_