Skip to content

Commit

Permalink
[CrOS Tether] Invoke "network properties updated" observer function w…
Browse files Browse the repository at this point in the history 
…hen Tether network properties change.

Previously, "network list" updates (i.e., updates which signify that networks were added/remvoed) were sent instead. This fixes an issue in which the settings UI was not properly updated.

BUG=672263,726869

Review-Url: https://codereview.chromium.org/2931003002
Cr-Commit-Position: refs/heads/master@{#478428}
  • Loading branch information
khorimoto authored and Commit Bot committed Jun 9, 2017
1 parent 478f3b2 commit e367383
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 44 deletions.
82 changes: 53 additions & 29 deletions chromeos/network/network_state_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ bool NetworkStateHandler::UpdateTetherNetworkProperties(
tether_network_state->set_battery_percentage(battery_percentage);
tether_network_state->set_signal_strength(signal_strength);

NotifyNetworkListChanged();
NotifyNetworkPropertiesUpdated(tether_network_state);
return true;
}

Expand All @@ -581,7 +581,8 @@ bool NetworkStateHandler::SetTetherNetworkHasConnectedToHost(
}

tether_network_state->set_tether_has_connected_to_host(true);
NotifyNetworkListChanged();

NotifyNetworkPropertiesUpdated(tether_network_state);
return true;
}

Expand All @@ -605,31 +606,37 @@ bool NetworkStateHandler::RemoveTetherNetworkState(const std::string& guid) {

bool NetworkStateHandler::DisassociateTetherNetworkStateFromWifiNetwork(
const std::string& tether_network_guid) {
NetworkState* tether_network =
NetworkState* tether_network_state =
GetModifiableNetworkStateFromGuid(tether_network_guid);

if (!tether_network) {
if (!tether_network_state) {
NET_LOG(ERROR) << "DisassociateTetherNetworkStateWithWifiNetwork(): Tether "
<< "network with ID " << tether_network_guid
<< " not registered; could not remove association.";
return false;
}

std::string wifi_network_guid = tether_network->tether_guid();
NetworkState* wifi_network =
std::string wifi_network_guid = tether_network_state->tether_guid();
NetworkState* wifi_network_state =
GetModifiableNetworkStateFromGuid(wifi_network_guid);

if (!wifi_network) {
if (!wifi_network_state) {
NET_LOG(ERROR) << "DisassociateTetherNetworkStateWithWifiNetwork(): Wi-Fi "
<< "network with ID " << wifi_network_guid
<< " not registered; could not remove association.";
return false;
}

wifi_network->set_tether_guid(std::string());
tether_network->set_tether_guid(std::string());
if (wifi_network_state->tether_guid().empty() &&
tether_network_state->tether_guid().empty()) {
return true;
}

wifi_network_state->set_tether_guid(std::string());
tether_network_state->set_tether_guid(std::string());

NotifyNetworkListChanged();
NotifyNetworkPropertiesUpdated(wifi_network_state);
NotifyNetworkPropertiesUpdated(tether_network_state);

return true;
}
Expand All @@ -644,46 +651,52 @@ bool NetworkStateHandler::AssociateTetherNetworkStateWithWifiNetwork(
return false;
}

NetworkState* tether_network =
NetworkState* tether_network_state =
GetModifiableNetworkStateFromGuid(tether_network_guid);
if (!tether_network) {
if (!tether_network_state) {
NET_LOG(ERROR) << "Tether network does not exist: " << tether_network_guid;
return false;
}
if (!NetworkTypePattern::Tether().MatchesType(tether_network->type())) {
if (!NetworkTypePattern::Tether().MatchesType(tether_network_state->type())) {
NET_LOG(ERROR) << "Network is not a Tether network: "
<< tether_network_guid;
return false;
}

NetworkState* wifi_network =
NetworkState* wifi_network_state =
GetModifiableNetworkStateFromGuid(wifi_network_guid);
if (!wifi_network) {
if (!wifi_network_state) {
NET_LOG(ERROR) << "Wi-Fi Network does not exist: " << wifi_network_guid;
return false;
}
if (!NetworkTypePattern::WiFi().MatchesType(wifi_network->type())) {
if (!NetworkTypePattern::WiFi().MatchesType(wifi_network_state->type())) {
NET_LOG(ERROR) << "Network is not a W-Fi network: " << wifi_network_guid;
return false;
}

tether_network->set_tether_guid(wifi_network_guid);
wifi_network->set_tether_guid(tether_network_guid);
NotifyNetworkListChanged();
if (wifi_network_state->tether_guid() == tether_network_guid &&
tether_network_state->tether_guid() == wifi_network_guid) {
return true;
}

tether_network_state->set_tether_guid(wifi_network_guid);
wifi_network_state->set_tether_guid(tether_network_guid);

NotifyNetworkPropertiesUpdated(wifi_network_state);
NotifyNetworkPropertiesUpdated(tether_network_state);

return true;
}

void NetworkStateHandler::SetTetherNetworkStateDisconnected(
const std::string& guid) {
// TODO(khorimoto): Remove the Tether network as the default network, and
// send a connection status change.
// TODO(khorimoto): Remove the Tether network as the default network.
SetTetherNetworkStateConnectionState(guid, shill::kStateDisconnect);
}

void NetworkStateHandler::SetTetherNetworkStateConnecting(
const std::string& guid) {
// TODO(khorimoto): Set the Tether network as the default network, and send
// a connection status change.
// TODO(khorimoto): Set the Tether network as the default network.
SetTetherNetworkStateConnectionState(guid, shill::kStateConfiguration);
}

Expand All @@ -694,24 +707,35 @@ void NetworkStateHandler::SetTetherNetworkStateConnected(
DCHECK(GetNetworkStateFromGuid(GetNetworkStateFromGuid(guid)->tether_guid())
->tether_guid() == guid);

// TODO(khorimoto): Send a connection status change.
SetTetherNetworkStateConnectionState(guid, shill::kStateOnline);
}

void NetworkStateHandler::SetTetherNetworkStateConnectionState(
const std::string& guid,
const std::string& connection_state) {
NetworkState* tether_network = GetModifiableNetworkStateFromGuid(guid);
if (!tether_network) {
NetworkState* tether_network_state = GetModifiableNetworkStateFromGuid(guid);
if (!tether_network_state) {
NET_LOG(ERROR) << "SetTetherNetworkStateConnectionState: Tether network "
<< "not found: " << guid;
return;
}

DCHECK(NetworkTypePattern::Tether().MatchesType(tether_network->type()));
DCHECK(
NetworkTypePattern::Tether().MatchesType(tether_network_state->type()));

tether_network->set_connection_state(connection_state);
NotifyNetworkListChanged();
std::string prev_connection_state = tether_network_state->connection_state();
tether_network_state->set_connection_state(connection_state);
DCHECK(!tether_network_state->is_captive_portal());

if (ConnectionStateChanged(tether_network_state, prev_connection_state,
false /* prev_is_captive_portal */)) {
NET_LOG(EVENT) << "Changing connection state for Tether network with GUID "
<< guid << ". Old state: " << prev_connection_state << ", "
<< "New state: " << connection_state;

OnNetworkConnectionStateChanged(tether_network_state);
NotifyNetworkPropertiesUpdated(tether_network_state);
}
}

void NetworkStateHandler::EnsureTetherDeviceState() {
Expand Down
82 changes: 67 additions & 15 deletions chromeos/network/network_state_handler_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -721,12 +721,14 @@ TEST_F(NetworkStateHandlerTest, TetherNetworkState) {
NetworkStateHandler::TECHNOLOGY_ENABLED);

EXPECT_EQ(0u, test_observer_->network_list_changed_count());
EXPECT_EQ(0, test_observer_->PropertyUpdatesForService(kTetherGuid1));

network_state_handler_->AddTetherNetworkState(
kTetherGuid1, kTetherName1, kTetherCarrier1, kTetherBatteryPercentage1,
kTetherSignalStrength1, false /* has_connected_to_network */);

EXPECT_EQ(1u, test_observer_->network_list_changed_count());
EXPECT_EQ(0, test_observer_->PropertyUpdatesForService(kTetherGuid1));

const NetworkState* tether_network =
network_state_handler_->GetNetworkStateFromGuid(kTetherGuid1);
Expand All @@ -743,7 +745,8 @@ TEST_F(NetworkStateHandlerTest, TetherNetworkState) {
kTetherGuid1, "NewCarrier", 5 /* battery_percentage */,
10 /* signal_strength */));

EXPECT_EQ(2u, test_observer_->network_list_changed_count());
EXPECT_EQ(1u, test_observer_->network_list_changed_count());
EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(kTetherGuid1));

tether_network =
network_state_handler_->GetNetworkStateFromGuid(kTetherGuid1);
Expand All @@ -759,18 +762,21 @@ TEST_F(NetworkStateHandlerTest, TetherNetworkState) {
EXPECT_TRUE(
network_state_handler_->SetTetherNetworkHasConnectedToHost(kTetherGuid1));

EXPECT_EQ(3u, test_observer_->network_list_changed_count());
EXPECT_EQ(1u, test_observer_->network_list_changed_count());
EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(kTetherGuid1));

// Try calling that function again. It should return false and should not
// trigger a NetworkListChanged() callback for observers.
EXPECT_FALSE(
network_state_handler_->SetTetherNetworkHasConnectedToHost(kTetherGuid1));

EXPECT_EQ(3u, test_observer_->network_list_changed_count());
EXPECT_EQ(1u, test_observer_->network_list_changed_count());
EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(kTetherGuid1));

network_state_handler_->RemoveTetherNetworkState(kTetherGuid1);

EXPECT_EQ(4u, test_observer_->network_list_changed_count());
EXPECT_EQ(2u, test_observer_->network_list_changed_count());
EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(kTetherGuid1));

ASSERT_FALSE(network_state_handler_->GetNetworkStateFromGuid(kTetherGuid1));

Expand All @@ -784,29 +790,34 @@ TEST_F(NetworkStateHandlerTest, TetherNetworkStateAssociation) {
network_state_handler_->SetTetherTechnologyState(
NetworkStateHandler::TECHNOLOGY_ENABLED);

EXPECT_EQ(0u, test_observer_->network_list_changed_count());

const std::string profile = "/profile/profile1";
const std::string wifi_path = "/service/wifi_with_guid";
AddService(wifi_path, kWifiGuid1, kWifiName1, shill::kTypeWifi,
shill::kStateOnline);
profile_test_->AddProfile(profile, "" /* userhash */);
EXPECT_TRUE(profile_test_->AddService(profile, wifi_path));
UpdateManagerProperties();
test_observer_->reset_updates();

EXPECT_EQ(1u, test_observer_->network_list_changed_count());
EXPECT_EQ(0, test_observer_->PropertyUpdatesForService(kTetherGuid1));
EXPECT_EQ(0, test_observer_->PropertyUpdatesForService(wifi_path));

network_state_handler_->AddTetherNetworkState(
kTetherGuid1, kTetherName1, kTetherCarrier1, kTetherBatteryPercentage1,
kTetherSignalStrength1, kTetherHasConnectedToHost1);

EXPECT_EQ(2u, test_observer_->network_list_changed_count());
EXPECT_EQ(0, test_observer_->PropertyUpdatesForService(kTetherGuid1));
EXPECT_EQ(0, test_observer_->PropertyUpdatesForService(wifi_path));

EXPECT_TRUE(
network_state_handler_->AssociateTetherNetworkStateWithWifiNetwork(
kTetherGuid1, kWifiGuid1));

EXPECT_EQ(3u, test_observer_->network_list_changed_count());
EXPECT_EQ(2u, test_observer_->network_list_changed_count());
EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(kTetherGuid1));
EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(wifi_path));

const NetworkState* wifi_network =
network_state_handler_->GetNetworkStateFromGuid(kWifiGuid1);
Expand All @@ -816,12 +827,28 @@ TEST_F(NetworkStateHandlerTest, TetherNetworkStateAssociation) {
network_state_handler_->GetNetworkStateFromGuid(kTetherGuid1);
EXPECT_EQ(kWifiGuid1, tether_network->tether_guid());

// Try associating again. The function call should return true since the
// association was successful, but no new observer updates should occur since
// no changes happened.
EXPECT_TRUE(
network_state_handler_->AssociateTetherNetworkStateWithWifiNetwork(
kTetherGuid1, kWifiGuid1));

EXPECT_EQ(kTetherGuid1, wifi_network->tether_guid());
EXPECT_EQ(kWifiGuid1, tether_network->tether_guid());

EXPECT_EQ(2u, test_observer_->network_list_changed_count());
EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(kTetherGuid1));
EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(wifi_path));

network_state_handler_->RemoveTetherNetworkState(kTetherGuid1);

EXPECT_EQ(4u, test_observer_->network_list_changed_count());
EXPECT_EQ(3u, test_observer_->network_list_changed_count());
EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(kTetherGuid1));
EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(wifi_path));

wifi_network = network_state_handler_->GetNetworkStateFromGuid(kWifiGuid1);
ASSERT_TRUE(wifi_network->tether_guid().empty());
EXPECT_TRUE(wifi_network->tether_guid().empty());
}

TEST_F(NetworkStateHandlerTest, TetherNetworkStateDisassociation) {
Expand All @@ -835,20 +862,27 @@ TEST_F(NetworkStateHandlerTest, TetherNetworkStateDisassociation) {
profile_test_->AddProfile(profile, "" /* userhash */);
EXPECT_TRUE(profile_test_->AddService(profile, wifi_path));
UpdateManagerProperties();
test_observer_->reset_updates();

EXPECT_EQ(1u, test_observer_->network_list_changed_count());
EXPECT_EQ(0, test_observer_->PropertyUpdatesForService(kTetherGuid1));
EXPECT_EQ(0, test_observer_->PropertyUpdatesForService(wifi_path));

network_state_handler_->AddTetherNetworkState(
kTetherGuid1, kTetherName1, kTetherCarrier1, kTetherBatteryPercentage1,
kTetherSignalStrength1, kTetherHasConnectedToHost1);

EXPECT_EQ(2u, test_observer_->network_list_changed_count());
EXPECT_EQ(0, test_observer_->PropertyUpdatesForService(kTetherGuid1));
EXPECT_EQ(0, test_observer_->PropertyUpdatesForService(wifi_path));

EXPECT_TRUE(
network_state_handler_->AssociateTetherNetworkStateWithWifiNetwork(
kTetherGuid1, kWifiGuid1));

EXPECT_EQ(3u, test_observer_->network_list_changed_count());
EXPECT_EQ(2u, test_observer_->network_list_changed_count());
EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(kTetherGuid1));
EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(wifi_path));

const NetworkState* wifi_network =
network_state_handler_->GetNetworkStateFromGuid(kWifiGuid1);
Expand All @@ -858,13 +892,16 @@ TEST_F(NetworkStateHandlerTest, TetherNetworkStateDisassociation) {
network_state_handler_->GetNetworkStateFromGuid(kTetherGuid1);
EXPECT_EQ(kWifiGuid1, tether_network->tether_guid());

network_state_handler_->DisassociateTetherNetworkStateFromWifiNetwork(
kTetherGuid1);
EXPECT_TRUE(
network_state_handler_->DisassociateTetherNetworkStateFromWifiNetwork(
kTetherGuid1));

ASSERT_TRUE(wifi_network->tether_guid().empty());
ASSERT_TRUE(tether_network->tether_guid().empty());
EXPECT_TRUE(wifi_network->tether_guid().empty());
EXPECT_TRUE(tether_network->tether_guid().empty());

EXPECT_EQ(4u, test_observer_->network_list_changed_count());
EXPECT_EQ(2u, test_observer_->network_list_changed_count());
EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(kTetherGuid1));
EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(wifi_path));
}

TEST_F(NetworkStateHandlerTest, TetherNetworkStateAssociationWifiRemoved) {
Expand Down Expand Up @@ -953,6 +990,9 @@ TEST_F(NetworkStateHandlerTest, SetTetherNetworkStateConnectionState) {
network_state_handler_->AssociateTetherNetworkStateWithWifiNetwork(
kTetherGuid1, kWifiGuid1);

EXPECT_EQ(0, test_observer_->ConnectionStateChangesForService(kTetherGuid1));
EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(kTetherGuid1));

const NetworkState* tether_network =
network_state_handler_->GetNetworkStateFromGuid(kTetherGuid1);

Expand All @@ -962,15 +1002,27 @@ TEST_F(NetworkStateHandlerTest, SetTetherNetworkStateConnectionState) {
network_state_handler_->SetTetherNetworkStateConnecting(kTetherGuid1);
EXPECT_TRUE(tether_network->IsConnectingState());

EXPECT_EQ(1, test_observer_->ConnectionStateChangesForService(kTetherGuid1));
EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(kTetherGuid1));

network_state_handler_->SetTetherNetworkStateConnected(kTetherGuid1);
EXPECT_TRUE(tether_network->IsConnectedState());

EXPECT_EQ(2, test_observer_->ConnectionStateChangesForService(kTetherGuid1));
EXPECT_EQ(3, test_observer_->PropertyUpdatesForService(kTetherGuid1));

network_state_handler_->SetTetherNetworkStateConnecting(kTetherGuid1);
EXPECT_TRUE(tether_network->IsReconnecting());

EXPECT_EQ(3, test_observer_->ConnectionStateChangesForService(kTetherGuid1));
EXPECT_EQ(4, test_observer_->PropertyUpdatesForService(kTetherGuid1));

network_state_handler_->SetTetherNetworkStateDisconnected(kTetherGuid1);
EXPECT_FALSE(tether_network->IsConnectingState());
EXPECT_FALSE(tether_network->IsConnectedState());

EXPECT_EQ(4, test_observer_->ConnectionStateChangesForService(kTetherGuid1));
EXPECT_EQ(5, test_observer_->PropertyUpdatesForService(kTetherGuid1));
}

TEST_F(NetworkStateHandlerTest, NetworkConnectionStateChanged) {
Expand Down

0 comments on commit e367383

Please sign in to comment.