Fix favorite list if a configuration of a hidden network is created.

The error was that NetworkStateHandler::RequestUpdateForNetwork only updated visible networks.

While there fixed the documentation of and unsafe calls to RequestUpdateForNetwork.

BUG=272544

Review URL: https://chromiumcodereview.appspot.com/22865008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@217460 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 48 insertions, 31 deletions

diff --git a/chromeos/network/network_cert_migrator.cc b/chromeos/network/network_cert_migrator.cc
index baca3c0..259f2a9 100644
--- a/chromeos/network/network_cert_migrator.cc
+++ b/chromeos/network/network_cert_migrator.cc
@@ -160,17 +160,16 @@ class NetworkCertMigrator::MigrationTask
 
   void ClearNssProperty(const std::string& service_path,
                         const std::string& nss_key) {
-    DBusThreadManager::Get()->GetShillServiceClient()
-        ->SetProperty(dbus::ObjectPath(service_path),
-                      nss_key,
-                      base::StringValue(std::string()),
-                      base::Bind(&base::DoNothing),
-                      base::Bind(&network_handler::ShillErrorCallbackFunction,
-                                 "MigrationTask.SetProperty failed",
-                                 service_path,
-                                 network_handler::ErrorCallback()));
-    cert_migrator_->network_state_handler_
-        ->RequestUpdateForNetwork(service_path);
+    DBusThreadManager::Get()->GetShillServiceClient()->SetProperty(
+        dbus::ObjectPath(service_path),
+        nss_key,
+        base::StringValue(std::string()),
+        base::Bind(
+            &MigrationTask::NotifyNetworkStateHandler, this, service_path),
+        base::Bind(&network_handler::ShillErrorCallbackFunction,
+                   "MigrationTask.SetProperty failed",
+                   service_path,
+                   network_handler::ErrorCallback()));
   }
 
   scoped_refptr<net::X509Certificate> FindCertificateWithNickname(
@@ -193,16 +192,35 @@ class NetworkCertMigrator::MigrationTask
     ca_cert_pems->AppendString(pem_encoded_cert);
     new_properties.SetWithoutPathExpansion(pem_key, ca_cert_pems.release());
 
-    DBusThreadManager::Get()->GetShillServiceClient()
-        ->SetProperties(dbus::ObjectPath(service_path),
-                        new_properties,
-                        base::Bind(&base::DoNothing),
-                        base::Bind(&network_handler::ShillErrorCallbackFunction,
-                                   "MigrationTask.SetProperties failed",
-                                   service_path,
-                                   network_handler::ErrorCallback()));
-    cert_migrator_->network_state_handler_
-        ->RequestUpdateForNetwork(service_path);
+    DBusThreadManager::Get()->GetShillServiceClient()->SetProperties(
+        dbus::ObjectPath(service_path),
+        new_properties,
+        base::Bind(
+            &MigrationTask::NotifyNetworkStateHandler, this, service_path),
+        base::Bind(&MigrationTask::LogErrorAndNotifyNetworkStateHandler,
+                   this,
+                   service_path));
+  }
+
+  void LogErrorAndNotifyNetworkStateHandler(const std::string& service_path,
+                                            const std::string& error_name,
+                                            const std::string& error_message) {
+    network_handler::ShillErrorCallbackFunction(
+        "MigrationTask.SetProperties failed",
+        service_path,
+        network_handler::ErrorCallback(),
+        error_name,
+        error_message);
+    NotifyNetworkStateHandler(service_path);
+  }
+
+  void NotifyNetworkStateHandler(const std::string& service_path) {
+    if (!cert_migrator_) {
+      VLOG(2) << "NetworkCertMigrator already destroyed. Aborting migration.";
+      return;
+    }
+    cert_migrator_->network_state_handler_->RequestUpdateForNetwork(
+        service_path);
   }
 
  private:
diff --git a/chromeos/network/network_state_handler.cc b/chromeos/network/network_state_handler.cc
index 04b6e28..106537f 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -345,16 +345,14 @@ void NetworkStateHandler::ConnectToBestWifiNetwork() {
                          shill_property_handler_->AsWeakPtr()));
 }
 
-bool NetworkStateHandler::RequestUpdateForNetwork(
+void NetworkStateHandler::RequestUpdateForNetwork(
     const std::string& service_path) {
   NetworkState* network = GetModifiableNetworkState(service_path);
-  if (!network)
-    return false;  // Only request an update for known networks.
-  network->set_update_requested(true);
+  if (network)
+    network->set_update_requested(true);
   NET_LOG_EVENT("RequestUpdate", service_path);
   shill_property_handler_->RequestProperties(
       ManagedState::MANAGED_TYPE_NETWORK, service_path);
-  return true;
 }
 
 void NetworkStateHandler::RequestUpdateForAllNetworks() {
diff --git a/chromeos/network/network_state_handler.h b/chromeos/network/network_state_handler.h
index ab99e33..3ae7126 100644
--- a/chromeos/network/network_state_handler.h
+++ b/chromeos/network/network_state_handler.h
@@ -179,12 +179,13 @@ class CHROMEOS_EXPORT NetworkStateHandler
   void ConnectToBestWifiNetwork();
 
   // Request an update for an existing NetworkState, e.g. after configuring
-  // a network. This is a no-op if an update request is already pending.
-  // Returns true if the network exists and an update is requested or pending.
+  // a network. This is a no-op if an update request is already pending. To
+  // ensure that a change is picked up, this must be called after Shill
+  // acknowledged it (e.g. in the callback of a SetProperties).
   // When the properties are received, NetworkPropertiesUpdated will be
   // signaled for each member of |observers_|, regardless of whether any
   // properties actually changed.
-  bool RequestUpdateForNetwork(const std::string& service_path);
+  void RequestUpdateForNetwork(const std::string& service_path);
 
   // Request an update for all existing NetworkState entries, e.g. after
   // loading an ONC configuration file that may have updated one or more
diff --git a/chromeos/network/network_state_handler_unittest.cc b/chromeos/network/network_state_handler_unittest.cc
index 419ba8b1..e32e4a5 100644
--- a/chromeos/network/network_state_handler_unittest.cc
+++ b/chromeos/network/network_state_handler_unittest.cc
@@ -376,8 +376,8 @@ TEST_F(NetworkStateHandlerTest, RequestUpdate) {
   // Request an update for kShillManagerClientStubDefaultWireless.
   EXPECT_EQ(1, test_observer_->PropertyUpdatesForService(
       kShillManagerClientStubDefaultWireless));
-  EXPECT_TRUE(network_state_handler_->RequestUpdateForNetwork(
-      kShillManagerClientStubDefaultWireless));
+  network_state_handler_->RequestUpdateForNetwork(
+      kShillManagerClientStubDefaultWireless);
   message_loop_.RunUntilIdle();
   EXPECT_EQ(2, test_observer_->PropertyUpdatesForService(
       kShillManagerClientStubDefaultWireless));