Skip to content

Commit

Permalink
Fix logic for default network notification
Browse files Browse the repository at this point in the history 
BUG=432404

Review URL: https://codereview.chromium.org/979183003

Cr-Commit-Position: refs/heads/master@{#320670}
  • Loading branch information
@stevenjb
stevenjb authored and Commit bot committed Mar 14, 2015
1 parent 0b3328f commit 89c8b88
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 45 deletions.
46 changes: 32 additions & 14 deletions chromeos/network/network_state_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -772,15 +772,23 @@ void NetworkStateHandler::DefaultNetworkServiceChanged(
if (!network) {
// If NetworkState is not available yet, do not notify observers here,
// they will be notified when the state is received.
NET_LOG_DEBUG("Default NetworkState not available",
default_network_path_);
NET_LOG(EVENT) << "Default NetworkState not available: "
<< default_network_path_;
return;
}
}
if (network && !network->IsConnectedState()) {
NET_LOG_ERROR(
"DefaultNetwork is not connected: " + network->connection_state(),
network->path());
if (network->IsConnectingState()) {
NET_LOG(EVENT) << "DefaultNetwork is connecting: " << GetLogName(network)
<< ": " << network->connection_state();
} else {
NET_LOG(ERROR) << "DefaultNetwork in unexpected state: "
<< GetLogName(network) << ": "
<< network->connection_state();
}
// Do not notify observers here, the notification will occur when the
// connection state changes.
return;
}
NotifyDefaultNetworkChanged(network);
}
Expand Down Expand Up @@ -867,23 +875,33 @@ void NetworkStateHandler::OnNetworkConnectionStateChanged(
NetworkState* network) {
SCOPED_NET_LOG_IF_SLOW();
DCHECK(network);
std::string event = "NetworkConnectionStateChanged";
bool notify_default = false;
if (network->path() == default_network_path_) {
event = "Default" + event;
if (!network->IsConnectedState()) {
NET_LOG_EVENT(
"DefaultNetwork is not connected: " + network->connection_state(),
network->path());
if (network->IsConnectedState()) {
notify_default = true;
} else if (network->IsConnectingState()) {
// Wait until the network is actually connected to notify that the default
// network changed.
NET_LOG(EVENT) << "Default network is not connected: "
<< GetLogName(network)
<< "State: " << network->connection_state();
} else {
NET_LOG(ERROR) << "Default network in unexpected state: "
<< GetLogName(network)
<< "State: " << network->connection_state();
default_network_path_.clear();
SortNetworkList();
NotifyDefaultNetworkChanged(nullptr);
}
}
NET_LOG_EVENT("NOTIFY:" + event + ": " + network->connection_state(),
GetLogName(network));
std::string desc = "NetworkConnectionStateChanged";
if (notify_default)
desc = "Default" + desc;
NET_LOG(EVENT) << "NOTIFY: " << desc << ": " << GetLogName(network) << ": "
<< network->connection_state();
FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
NetworkConnectionStateChanged(network));
if (network->path() == default_network_path_)
if (notify_default)
NotifyDefaultNetworkChanged(network);
}

Expand Down
84 changes: 53 additions & 31 deletions chromeos/network/network_state_handler_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ class TestObserver : public chromeos::NetworkStateHandlerObserver {
}

void DefaultNetworkChanged(const NetworkState* network) override {
EXPECT_TRUE(!network || network->IsConnectedState());
++default_network_change_count_;
default_network_ = network ? network->path() : "";
default_network_connection_state_ =
Expand Down Expand Up @@ -169,10 +170,10 @@ namespace chromeos {
class NetworkStateHandlerTest : public testing::Test {
public:
NetworkStateHandlerTest()
: device_test_(NULL),
manager_test_(NULL),
profile_test_(NULL),
service_test_(NULL) {}
: device_test_(nullptr),
manager_test_(nullptr),
profile_test_(nullptr),
service_test_(nullptr) {}
~NetworkStateHandlerTest() override {}

void SetUp() override {
Expand Down Expand Up @@ -254,6 +255,14 @@ class NetworkStateHandlerTest : public testing::Test {
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_;
Expand Down Expand Up @@ -405,10 +414,8 @@ TEST_F(NetworkStateHandlerTest, GetVisibleNetworks) {
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());
Expand Down Expand Up @@ -494,20 +501,14 @@ TEST_F(NetworkStateHandlerTest, ServicePropertyChanged) {
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));
}
Expand Down Expand Up @@ -614,42 +615,63 @@ TEST_F(NetworkStateHandlerTest, DefaultServiceChanged) {
EXPECT_EQ(eth1, test_observer_->default_network());

// Change the default network by changing Manager.DefaultService.
// This should only generate one default network notification when the
// DefaultService property changes.
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));
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.
// Change the state of the default network. This should generate a
// default network notification.
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
// Updating a property on the default network should also 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 notification should only
// fire once when 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) {
Expand Down

0 comments on commit 89c8b88

Please sign in to comment.