Improve NetworkStateHandler API

chromeos/network/network_state_handler_unittest.cc

Index: chromeos/network/network_state_handler_unittest.cc
diff --git a/chromeos/network/network_state_handler_unittest.cc b/chromeos/network/network_state_handler_unittest.cc
index 448bf4c98c8e3fffe059ec9c921b6e71450e75c8..368173c9936927d92b8ee6d8b9fdc5f8589baf2e 100644
--- a/chromeos/network/network_state_handler_unittest.cc
+++ b/chromeos/network/network_state_handler_unittest.cc
@@ -29,64 +29,77 @@ void ErrorCallbackFunction(const std::string& error_name,
 }
 
 using chromeos::NetworkState;
+using chromeos::NetworkStateHandler;
 
 class TestObserver : public chromeos::NetworkStateHandlerObserver {
  public:
-  TestObserver()
-      : manager_changed_count_(0),
+  explicit TestObserver(NetworkStateHandler* handler)
+      : handler_(handler),
+        manager_changed_count_(0),
         network_count_(0) {
   }
 
   virtual ~TestObserver() {
   }
 
-  virtual void NetworkManagerChanged() {
+  virtual void NetworkManagerChanged() OVERRIDE {
     ++manager_changed_count_;
   }
 
   virtual void NetworkListChanged(
-      const chromeos::NetworkStateHandler::NetworkStateList& networks) {
+      const NetworkStateHandler::NetworkStateList& networks) OVERRIDE {
     network_count_ = networks.size();
+    if (network_count_ == 0) {
+      default_network_ = "";
+      default_network_connection_state_ = "";
+    }
   }
 
-  virtual void ActiveNetworkChanged(const NetworkState* network) {
-    active_network_ = network ? network->path() : "";
-    active_network_state_ = network ? network->state() : "";
+  virtual void DefaultNetworkChanged(const NetworkState* network) OVERRIDE {
+    default_network_ = network ? network->path() : "";
+    default_network_connection_state_ =
+        network ?  network->connection_state() : "";
   }
 
-  virtual void ActiveNetworkStateChanged(const NetworkState* network) {
-    active_network_state_ = network ? network->state() : "";
+  virtual void NetworkStateChanged(const NetworkState* network) OVERRIDE {
+    network_connection_state_[network->path()] = network->connection_state();
+    connection_state_changes_[network->path()]++;
   }
 
-  virtual void NetworkServiceChanged(const NetworkState* network) {
+  virtual void NetworkPropertyChanged(const NetworkState* network) OVERRIDE {
     DCHECK(network);
-    std::map<std::string, int>::iterator iter =
-        property_changes_.find(network->path());
-    if (iter == property_changes_.end())
-      property_changes_[network->path()] = 1;
-    else
-      iter->second++;
+    property_changes_[network->path()]++;
   }
 
   size_t manager_changed_count() { return manager_changed_count_; }
   size_t network_count() { return network_count_; }
-  std::string active_network() { return active_network_; }
-  std::string active_network_state() { return active_network_state_; }
+  std::string default_network() { return default_network_; }
+  std::string default_network_connection_state() {
+    return default_network_connection_state_;
+  }
 
   int PropertyChangesForService(const std::string& service_path) {
-    std::map<std::string, int>::iterator iter =
-        property_changes_.find(service_path);
-    if (iter == property_changes_.end())
-      return 0;
-    return iter->second;
+    return property_changes_[service_path];
+  }
+
+  int ConnectionStateChangesForService(const std::string& service_path) {
+    return connection_state_changes_[service_path];
+  }
+
+  std::string NetworkConnectionStateForService(
+      const std::string& service_path) {
+    return network_connection_state_[service_path];
   }
 
  private:
+  NetworkStateHandler* handler_;
   size_t manager_changed_count_;
   size_t network_count_;
-  std::string active_network_;
-  std::string active_network_state_;
+  std::string default_network_;
+  std::string default_network_connection_state_;
   std::map<std::string, int> property_changes_;
+  std::map<std::string, int> connection_state_changes_;
+  std::map<std::string, std::string> network_connection_state_;
 
   DISALLOW_COPY_AND_ASSIGN(TestObserver);
 };
@@ -112,8 +125,8 @@ class NetworkStateHandlerTest : public testing::Test {
   }
 
   void SetupNetworkStateHandler() {
-    test_observer_.reset(new TestObserver);
     network_state_handler_.reset(new NetworkStateHandler);
+    test_observer_.reset(new TestObserver(network_state_handler_.get()));
     network_state_handler_->AddObserver(test_observer_.get());
     network_state_handler_->InitShillPropertyHandler();
   }
@@ -136,19 +149,25 @@ TEST_F(NetworkStateHandlerTest, NetworkStateHandlerStub) {
   const size_t kNumShillManagerClientStubImplServices = 4;
   EXPECT_EQ(kNumShillManagerClientStubImplServices,
             test_observer_->network_count());
-  // Ensure that the first stub network is the active network.
-  const std::string kShillManagerClientStubActiveService = "stub_ethernet";
-  EXPECT_EQ(kShillManagerClientStubActiveService,
-            test_observer_->active_network());
-  EXPECT_EQ(kShillManagerClientStubActiveService,
-            network_state_handler_->ActiveNetwork()->path());
-  EXPECT_EQ(kShillManagerClientStubActiveService,
+  // Ensure that the first stub network is the default network.
+  const std::string kShillManagerClientStubDefaultService = "stub_ethernet";
+  EXPECT_EQ(kShillManagerClientStubDefaultService,
+            test_observer_->default_network());
+  EXPECT_EQ(kShillManagerClientStubDefaultService,
+            network_state_handler_->ConnectedNetworkByType(
+                NetworkState::kMatchTypeDefault)->path());
+  EXPECT_EQ(kShillManagerClientStubDefaultService,
             network_state_handler_->ConnectedNetworkByType(
                 flimflam::kTypeEthernet)->path());
-  EXPECT_EQ(flimflam::kStateOnline, test_observer_->active_network_state());
+  const std::string kShillManagerClientStubDefaultWireless = "stub_wifi1";
+  EXPECT_EQ(kShillManagerClientStubDefaultWireless,
+            network_state_handler_->ConnectedNetworkByType(
+                NetworkState::kMatchTypeWireless)->path());
+  EXPECT_EQ(flimflam::kStateOnline,
+            test_observer_->default_network_connection_state());
 }
 
-TEST_F(NetworkStateHandlerTest, NetworkStateHandlerTechnologyChanged) {
+TEST_F(NetworkStateHandlerTest, TechnologyChanged) {
   // This relies on the stub dbus implementations for ShillManagerClient,
   SetupNetworkStateHandler();
   message_loop_.RunUntilIdle();
@@ -163,7 +182,7 @@ TEST_F(NetworkStateHandlerTest, NetworkStateHandlerTechnologyChanged) {
   EXPECT_TRUE(network_state_handler_->TechnologyEnabled(flimflam::kTypeWimax));
 }
 
-TEST_F(NetworkStateHandlerTest, NetworkStateHandlerServicePropertyChanged) {
+TEST_F(NetworkStateHandlerTest, ServicePropertyChanged) {
   // This relies on the stub dbus implementations for ShillManagerClient,
   SetupNetworkStateHandler();
   message_loop_.RunUntilIdle();
@@ -180,35 +199,63 @@ TEST_F(NetworkStateHandlerTest, NetworkStateHandlerServicePropertyChanged) {
   EXPECT_EQ("TestSecurity",
             network_state_handler_->GetNetworkState(eth0)->security());
   EXPECT_EQ(2, test_observer_->PropertyChangesForService(eth0));
+  // Confirm that changing a property to the same value still signals the
+  // observer.
+  DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(

[pneubeck (no reviews), 2012/12/19 19:40:06]

Is that really a requirement about the NetworkStateHandler?
Who relies on such repeated notifications? It's also not explicitly stated in
the class/method descriptions. So nobody should rely on that.

[stevenjb, 2012/12/19 21:36:50]

Good point. I added that to confirm that the behavior is what I expected, but
you're right, it's not something we especially desire or should rely on. Removed
the extra test.

+      dbus::ObjectPath(eth0),
+      flimflam::kSecurityProperty, security_value,
+      base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(3, test_observer_->PropertyChangesForService(eth0));
+}
+
+TEST_F(NetworkStateHandlerTest, NetworkStateChanged) {
+  // This relies on the stub dbus implementations for ShillManagerClient,
+  SetupNetworkStateHandler();
+  message_loop_.RunUntilIdle();
+  // Change a network state.
+  ShillServiceClient::TestInterface* service_test =
+      DBusThreadManager::Get()->GetShillServiceClient()->GetTestInterface();
+  const std::string eth0 = "stub_ethernet";
+  base::StringValue connection_state_idle_value(flimflam::kStateIdle);
+  service_test->SetServiceProperty(eth0, flimflam::kStateProperty,
+                                   connection_state_idle_value);
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(flimflam::kStateIdle,
+            test_observer_->NetworkConnectionStateForService(eth0));
+  EXPECT_EQ(2, test_observer_->ConnectionStateChangesForService(eth0));
+  // Confirm that changing the connection state to the same value does *not*
+  // signal the observer.
+  service_test->SetServiceProperty(eth0, flimflam::kStateProperty,
+                                   connection_state_idle_value);
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(2, test_observer_->ConnectionStateChangesForService(eth0));
 }
 
-TEST_F(NetworkStateHandlerTest, NetworkStateHandlerActiveServiceChanged) {
+TEST_F(NetworkStateHandlerTest, DefaultServiceChanged) {
   // This relies on the stub dbus implementations for ShillManagerClient,
   SetupNetworkStateHandler();
   message_loop_.RunUntilIdle();
 
-  // Change the active network by inserting wifi1 at the front of the list.
   ShillManagerClient::TestInterface* manager_test =
       DBusThreadManager::Get()->GetShillManagerClient()->GetTestInterface();
   ASSERT_TRUE(manager_test);
-  const std::string wifi1 = "stub_wifi1";
-  manager_test->AddServiceAtIndex(wifi1, 0, true);
-  message_loop_.RunUntilIdle();
-  EXPECT_EQ(wifi1, test_observer_->active_network());
-  EXPECT_EQ(flimflam::kStateOnline, test_observer_->active_network_state());
-
-  // Change the state of wifi1, ensure that triggers the active state changed
-  // observer.
   ShillServiceClient::TestInterface* service_test =
       DBusThreadManager::Get()->GetShillServiceClient()->GetTestInterface();
   ASSERT_TRUE(service_test);
-  base::StringValue state_value(flimflam::kStateConfiguration);
-  service_test->SetServiceProperty(wifi1, flimflam::kStateProperty,
-                                   state_value);
+
+  // Change the default network by inserting wifi1 at the front of the list
+  // and changing the state of stub_ethernet to Idle.
+  const std::string wifi1 = "stub_wifi1";
+  manager_test->AddServiceAtIndex(wifi1, 0, true);
+  const std::string eth0 = "stub_ethernet";
+  base::StringValue connection_state_idle_value(flimflam::kStateIdle);
+  service_test->SetServiceProperty(eth0, flimflam::kStateProperty,
+                                   connection_state_idle_value);
   message_loop_.RunUntilIdle();
-  EXPECT_EQ(wifi1, test_observer_->active_network());
-  EXPECT_EQ(flimflam::kStateConfiguration,
-            test_observer_->active_network_state());
+  EXPECT_EQ(wifi1, test_observer_->default_network());
+  EXPECT_EQ(flimflam::kStateOnline,
+            test_observer_->default_network_connection_state());
 }
 
 }  // namespace chromeos