Adding a NetworkProfileHandler used by ManagedNetworkConfigurationHandler.

chromeos/dbus/shill_profile_client_stub.h

Index: chromeos/dbus/shill_profile_client_stub.h
diff --git a/chromeos/dbus/shill_profile_client_stub.h b/chromeos/dbus/shill_profile_client_stub.h
index efc86ce8da92c67efbe543c86c6c6a1162124807..25dda897ea4e684fe71324d1ac26d7a23385e187 100644
--- a/chromeos/dbus/shill_profile_client_stub.h
+++ b/chromeos/dbus/shill_profile_client_stub.h
@@ -5,9 +5,11 @@
 #ifndef CHROMEOS_DBUS_SHILL_PROFILE_CLIENT_STUB_H_
 #define CHROMEOS_DBUS_SHILL_PROFILE_CLIENT_STUB_H_
 
+#include <map>
 #include <string>
 
 #include "base/basictypes.h"
+#include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/shill_profile_client.h"
 
 namespace chromeos {
@@ -16,7 +18,8 @@ namespace chromeos {
 class ShillProfileClientStub : public ShillProfileClient,
                                public ShillProfileClient::TestInterface {
  public:
-  ShillProfileClientStub();
+  explicit ShillProfileClientStub(
+      ShillManagerClient::TestInterface* manager_test);

[stevenjb, 2013/04/26 21:00:49]

I don't know that I like this. Is there ever a situation where we would
initialize ShillProfileClientStub() without initializing
ShillManagerClientStub()? i.e. can't ShillProfileClientStub just use
DBusThreadManager::Get()->GetShillManagerClient()->GetTestInterface()?

[pneubeck (no reviews), 2013/04/29 18:05:51]

My thoughts were:
1.
managed_network_configuration_handler_unittest.cc is doing exactly that.
There, notifications from Shill and return values from calls to Shill have to be
consistent. With mocks that's rather cumbersome and repetitive.
Since this simple bookkeeping exists already in the Stubs, why not using it?
Of the ShillProfileClientStub it's using the storage of profile entries.
Of the NetworkProfileHandlerStub it's using the mapping between profiles and
users.

2.
Overall I find it rather unhealthy that the Stubs are mutually dependent on each
other. Because of that, we cannot use the Stubs one by one and cannot combine
Mocks with Stubs etc., but have to use all ClientStubs or none.
I see that that might be necessary for the ShillManagerClient but for the other
Clients?

[stevenjb, 2013/04/30 17:42:47]

I believe strongly that we *shouldn't* combine mocks with stubs, not within a
subdirectory (i.e chromeos/dbus) anyway. We should either use all of the shill
stubs, or none.

The stubs are consciously designed to work together to emulate Shill behavior
and not work independently. While some may work independently now, maintaining
that is going to be difficult.

What problems arise if you use ShillManagerClientStub?

[pneubeck (no reviews), 2013/05/03 17:32:55]

I have to use the ClientMocks in the unit test to verify exactly which calls are
done by the ConfigHandler.

I don't use the ProfileStub anymore, which required to add a seperate
ProfilesStub in the test file.

   virtual ~ShillProfileClientStub();
 
   // ShillProfileClient overrides.
@@ -41,18 +44,25 @@ class ShillProfileClientStub : public ShillProfileClient,
   virtual ShillProfileClient::TestInterface* GetTestInterface() OVERRIDE;
 
   // ShillProfileClient::TestInterface overrides.
-  virtual void AddProfile(const std::string& profile_path) OVERRIDE;
+  virtual void AddProfile(const std::string& profile_path,
+                          const std::string& userhash) OVERRIDE;
   virtual void AddEntry(const std::string& profile_path,
                         const std::string& entry_path,
                         const base::DictionaryValue& properties) OVERRIDE;
   virtual bool AddService(const std::string& service_path) OVERRIDE;
 
  private:
-  base::DictionaryValue* GetProfile(const dbus::ObjectPath& profile_path,
-                                    const ErrorCallback& error_callback);
+  struct ProfileProperties;
+  typedef std::map<std::string, ProfileProperties*> ProfileMap;
 
-  // This maps profile path -> entry path -> Shill properties.
-  base::DictionaryValue profile_entries_;
+  ProfileProperties* GetProfile(const dbus::ObjectPath& profile_path,
+                                const ErrorCallback& error_callback);
+
+  // The values are owned by this class and are explicitly destroyed where
+  // necessary.
+  ProfileMap profiles_;
+
+  ShillManagerClient::TestInterface* manager_test_;
 
   DISALLOW_COPY_AND_ASSIGN(ShillProfileClientStub);
 };