Fix logic for default network notification

chromeos/network/network_state_handler_unittest.cc

 // Copyright (c) 2012 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 "chromeos/network/network_state_handler.h"
 
 #include <map>
 #include <set>
 #include <string>
 
@@ skipping 64 matching lines @@
                                    &networks);
     network_count_ = networks.size();
     if (network_count_ == 0) {
       default_network_ = "";
       default_network_connection_state_ = "";
     }
     ++network_list_changed_count_;
   }
 
   void DefaultNetworkChanged(const NetworkState* network) override {
+    EXPECT_TRUE(!network || network->IsConnectedState());
     ++default_network_change_count_;
     default_network_ = network ? network->path() : "";
     default_network_connection_state_ =
         network ? network->connection_state() : "";
     DVLOG(1) << "DefaultNetworkChanged: " << default_network_
              << " State: " << default_network_connection_state_;
   }
 
   void NetworkConnectionStateChanged(const NetworkState* network) override {
     network_connection_state_[network->path()] = network->connection_state();
@@ skipping 67 matching lines @@
   DISALLOW_COPY_AND_ASSIGN(TestObserver);
 };
 
 }  // namespace
 
 namespace chromeos {
 
 class NetworkStateHandlerTest : public testing::Test {
  public:
   NetworkStateHandlerTest()
-      : device_test_(NULL),
-        manager_test_(NULL),
-        profile_test_(NULL),
-        service_test_(NULL) {}
+      : device_test_(nullptr),

[pneubeck (no reviews), 2015/03/12 19:40:09]

nit: you could put all of these also to the declaration (since C++11) and drop
the constructor.

[stevenjb, 2015/03/12 19:57:07]

I'd rather do that consistently throughout these classes at some point, but
noted.

+        manager_test_(nullptr),
+        profile_test_(nullptr),
+        service_test_(nullptr) {}
   ~NetworkStateHandlerTest() override {}
 
   void SetUp() override {
     // Initialize DBusThreadManager with a stub implementation.
     DBusThreadManager::Initialize();
     SetupDefaultShillState();
     network_state_handler_.reset(new NetworkStateHandler);
     test_observer_.reset(new TestObserver(network_state_handler_.get()));
     network_state_handler_->AddObserver(test_observer_.get(), FROM_HERE);
     network_state_handler_->InitShillPropertyHandler();
@@ skipping 61 matching lines @@
                "cellular1_guid",
                "cellular1",
                shill::kTypeCellular,
                shill::kStateIdle);
   }
 
   void UpdateManagerProperties() {
     message_loop_.RunUntilIdle();
   }
 
+  void SetServiceProperty(const std::string& service_path,
+                          const std::string& key,
+                          const base::Value& value) {
+    DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
+        dbus::ObjectPath(service_path), key, value,
+        base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  }
+
   base::MessageLoopForUI message_loop_;
   scoped_ptr<NetworkStateHandler> network_state_handler_;
   scoped_ptr<TestObserver> test_observer_;
   ShillDeviceClient::TestInterface* device_test_;
   ShillManagerClient::TestInterface* manager_test_;
   ShillProfileClient::TestInterface* profile_test_;
   ShillServiceClient::TestInterface* service_test_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(NetworkStateHandlerTest);
@@ skipping 131 matching lines @@
   message_loop_.RunUntilIdle();
   EXPECT_EQ(kNumShillManagerClientStubImplServices + 1,
             test_observer_->network_count());
 
   // Get visible networks.
   NetworkStateHandler::NetworkStateList networks;
   network_state_handler_->GetVisibleNetworkList(&networks);
   EXPECT_EQ(kNumShillManagerClientStubImplServices, networks.size());
 
   // Change the visible state of a network.
-  DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-      dbus::ObjectPath(kShillManagerClientStubWifi2),
-      shill::kVisibleProperty, base::FundamentalValue(false),
-      base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  SetServiceProperty(kShillManagerClientStubWifi2, shill::kVisibleProperty,
+                     base::FundamentalValue(false));
   message_loop_.RunUntilIdle();
   network_state_handler_->GetVisibleNetworkList(&networks);
   EXPECT_EQ(kNumShillManagerClientStubImplServices - 1, networks.size());
 }
 
 TEST_F(NetworkStateHandlerTest, TechnologyChanged) {
   // Disable a technology. Will immediately set the state to AVAILABLE and
   // notify observers.
   network_state_handler_->SetTechnologyEnabled(
       NetworkTypePattern::WiFi(), false, network_handler::ErrorCallback());
@@ skipping 65 matching lines @@
 }
 
 TEST_F(NetworkStateHandlerTest, ServicePropertyChanged) {
   // Set a service property.
   const std::string eth1 = kShillManagerClientStubDefaultService;
   const NetworkState* ethernet = network_state_handler_->GetNetworkState(eth1);
   ASSERT_TRUE(ethernet);
   EXPECT_EQ("", ethernet->security_class());
   EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(eth1));
   base::StringValue security_class_value("TestSecurityClass");
-  DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-      dbus::ObjectPath(eth1),
-      shill::kSecurityClassProperty, security_class_value,
-      base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  SetServiceProperty(eth1, shill::kSecurityClassProperty, security_class_value);
   message_loop_.RunUntilIdle();
   ethernet = network_state_handler_->GetNetworkState(eth1);
   EXPECT_EQ("TestSecurityClass", ethernet->security_class());
   EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(eth1));
 
   // Changing a service to the existing value should not trigger an update.
-  DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-      dbus::ObjectPath(eth1),
-      shill::kSecurityClassProperty, security_class_value,
-      base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  SetServiceProperty(eth1, shill::kSecurityClassProperty, security_class_value);
   message_loop_.RunUntilIdle();
   EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(eth1));
 }
 
 TEST_F(NetworkStateHandlerTest, GetState) {
   const std::string profile = "/profile/profile1";
   const std::string wifi_path = kShillManagerClientStubDefaultWifi;
 
   // Add a wifi service to a Profile.
   profile_test_->AddProfile(profile, "" /* userhash */);
@@ skipping 87 matching lines @@
   EXPECT_EQ(1u, test_observer_->default_network_change_count());
 }
 
 TEST_F(NetworkStateHandlerTest, DefaultServiceChanged) {
   const std::string eth1 = kShillManagerClientStubDefaultService;
   // The default service should be eth1.
   EXPECT_EQ(eth1, test_observer_->default_network());
 
   // Change the default network by changing Manager.DefaultService.
   const std::string wifi1 = kShillManagerClientStubDefaultWifi;
-  base::StringValue wifi1_value(wifi1);
-  manager_test_->SetManagerProperty(
-      shill::kDefaultServiceProperty, wifi1_value);
+  SetServiceProperty(eth1, shill::kStateProperty,
+                     base::StringValue(shill::kStateIdle));

[pneubeck (no reviews), 2015/03/12 19:40:09]

since both of these calls send a notification to NSH, it's not obvious to the
reader how many updates are to expected and why.
Maybe add a comment to the EXPECT_EQ(1u, ...).

[stevenjb, 2015/03/12 19:57:07]

Done.

+  manager_test_->SetManagerProperty(shill::kDefaultServiceProperty,
+                                    base::StringValue(wifi1));
   message_loop_.RunUntilIdle();
   EXPECT_EQ(wifi1, test_observer_->default_network());
   EXPECT_EQ(1u, test_observer_->default_network_change_count());
 
   // Change the state of the default network.
   test_observer_->reset_change_counts();
-  base::StringValue connection_state_ready_value(shill::kStateReady);
   service_test_->SetServiceProperty(wifi1, shill::kStateProperty,
-                                   connection_state_ready_value);
+                                    base::StringValue(shill::kStateReady));
   message_loop_.RunUntilIdle();
   EXPECT_EQ(shill::kStateReady,
             test_observer_->default_network_connection_state());
   EXPECT_EQ(1u, test_observer_->default_network_change_count());
 
   // Updating a property on the default network should trigger
   // a default network change.
   test_observer_->reset_change_counts();
-  DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-      dbus::ObjectPath(wifi1),
-      shill::kSecurityClassProperty, base::StringValue("TestSecurityClass"),
-      base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  SetServiceProperty(wifi1, shill::kSecurityClassProperty,
+                     base::StringValue("TestSecurityClass"));
   message_loop_.RunUntilIdle();
   EXPECT_EQ(1u, test_observer_->default_network_change_count());
 
   // No default network updates for signal strength changes.
   test_observer_->reset_change_counts();
-  DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
-      dbus::ObjectPath(wifi1),
-      shill::kSignalStrengthProperty, base::FundamentalValue(32),
-      base::Bind(&base::DoNothing), base::Bind(&ErrorCallbackFunction));
+  SetServiceProperty(wifi1, shill::kSignalStrengthProperty,
+                     base::FundamentalValue(32));
   message_loop_.RunUntilIdle();
   EXPECT_EQ(0u, test_observer_->default_network_change_count());
+
+  // Change the default network to a Connecting network, then set the
+  // state to Connected. The DefaultNetworkChange dnotification should not

[pneubeck (no reviews), 2015/03/12 19:40:09]

dnotification -> notification

+  // fire until the network is connected.
+  test_observer_->reset_change_counts();
+  SetServiceProperty(wifi1, shill::kStateProperty,
+                     base::StringValue(shill::kStateIdle));
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(1u, test_observer_->default_network_change_count());
+  EXPECT_EQ(std::string(), test_observer_->default_network());
+
+  const std::string wifi2 = kShillManagerClientStubWifi2;
+  manager_test_->SetManagerProperty(shill::kDefaultServiceProperty,
+                                    base::StringValue(wifi2));
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(1u, test_observer_->default_network_change_count());
+  // Change the connection state of the default network, observer should fire.
+  SetServiceProperty(wifi2, shill::kStateProperty,
+                     base::StringValue(shill::kStateReady));
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(wifi2, test_observer_->default_network());
+  EXPECT_EQ(2u, test_observer_->default_network_change_count());
 }
 
 TEST_F(NetworkStateHandlerTest, RequestUpdate) {
   // Request an update for kShillManagerClientStubDefaultWifi.
   EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(
       kShillManagerClientStubDefaultWifi));
   network_state_handler_->RequestUpdateForNetwork(
       kShillManagerClientStubDefaultWifi);
   message_loop_.RunUntilIdle();
   EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(
@@ skipping 109 matching lines @@
       kShillManagerClientStubDefaultWifi, shill::kIPConfigProperty,
       base::StringValue(kIPConfigPath));
   UpdateManagerProperties();
   EXPECT_EQ(1, test_observer_->PropertyUpdatesForDevice(
       kShillManagerClientStubWifiDevice));
   EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(
       kShillManagerClientStubDefaultWifi));
 }
 
 }  // namespace chromeos