Cleanup error handling in network handlers.

Also fixes two small bugs (missing returns after invoking the error callback) in ManagedNetworkConfigurationHandler.

BUG=NONE

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

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

5 files changed, 76 insertions, 111 deletions

diff --git a/chromeos/network/managed_network_configuration_handler_impl.cc b/chromeos/network/managed_network_configuration_handler_impl.cc
index 73e5e07..9a7af0f 100644
--- a/chromeos/network/managed_network_configuration_handler_impl.cc
+++ b/chromeos/network/managed_network_configuration_handler_impl.cc
@@ -43,22 +43,13 @@ typedef std::map<std::string, const base::DictionaryValue*> GuidToPolicyMap;
 
 // These are error strings used for error callbacks. None of these error
 // messages are user-facing: they should only appear in logs.
-const char kInvalidUserSettingsMessage[] = "User settings are invalid.";
-const char kInvalidUserSettings[] = "Error.InvalidUserSettings";
-const char kNetworkAlreadyConfiguredMessage[] =
-    "Network is already configured.";
-const char kNetworkAlreadyConfigured[] = "Error.NetworkAlreadyConfigured";
-const char kPoliciesNotInitializedMessage[] = "Policies not initialized.";
-const char kPoliciesNotInitialized[] = "Error.PoliciesNotInitialized";
-const char kProfileNotInitializedMessage[] = "Profile not initialized.";
-const char kProfileNotInitialized[] = "Error.ProflieNotInitialized";
-const char kSetOnUnconfiguredNetworkMessage[] =
-    "Unable to modify properties of an unconfigured network.";
-const char kSetOnUnconfiguredNetwork[] = "Error.SetCalledOnUnconfiguredNetwork";
-const char kUnknownProfilePathMessage[] = "Profile path is unknown.";
-const char kUnknownProfilePath[] = "Error.UnknownProfilePath";
-const char kUnknownServicePathMessage[] = "Service path is unknown.";
-const char kUnknownServicePath[] = "Error.UnknownServicePath";
+const char kInvalidUserSettings[] = "InvalidUserSettings";
+const char kNetworkAlreadyConfigured[] = "NetworkAlreadyConfigured";
+const char kPoliciesNotInitialized[] = "PoliciesNotInitialized";
+const char kProfileNotInitialized[] = "ProflieNotInitialized";
+const char kSetOnUnconfiguredNetwork[] = "SetCalledOnUnconfiguredNetwork";
+const char kUnknownProfilePath[] = "UnknownProfilePath";
+const char kUnknownServicePath[] = "UnknownServicePath";
 
 std::string ToDebugString(::onc::ONCSource source,
                           const std::string& userhash) {
@@ -66,17 +57,13 @@ std::string ToDebugString(::onc::ONCSource source,
       ("user policy of " + userhash) : "device policy";
 }
 
-void RunErrorCallback(const std::string& service_path,
-                      const std::string& error_name,
-                      const std::string& error_message,
-                      const network_handler::ErrorCallback& error_callback) {
-  NET_LOG_ERROR(error_name, error_message);
-  error_callback.Run(
-      error_name,
-      make_scoped_ptr(
-          network_handler::CreateErrorData(service_path,
-                                           error_name,
-                                           error_message)));
+void InvokeErrorCallback(const std::string& service_path,
+                         const network_handler::ErrorCallback& error_callback,
+                         const std::string& error_name) {
+  std::string error_msg = "ManagedConfig Error: " + error_name;
+  NET_LOG_ERROR(error_msg, service_path);
+  network_handler::RunErrorCallback(
+      error_callback, service_path, error_name, error_msg);
 }
 
 void LogErrorWithDict(const tracked_objects::Location& from_where,
@@ -133,10 +120,7 @@ void ManagedNetworkConfigurationHandlerImpl::GetManagedProperties(
     const network_handler::DictionaryResultCallback& callback,
     const network_handler::ErrorCallback& error_callback) {
   if (!GetPoliciesForUser(userhash) || !GetPoliciesForUser(std::string())) {
-    RunErrorCallback(service_path,
-                     kPoliciesNotInitialized,
-                     kPoliciesNotInitializedMessage,
-                     error_callback);
+    InvokeErrorCallback(service_path, error_callback, kPoliciesNotInitialized);
     return;
   }
   network_configuration_handler_->GetProperties(
@@ -199,10 +183,8 @@ void ManagedNetworkConfigurationHandlerImpl::GetManagedPropertiesCallback(
   if (!guid.empty() && profile) {
     const Policies* policies = GetPoliciesForProfile(*profile);
     if (!policies) {
-      RunErrorCallback(service_path,
-                       kPoliciesNotInitialized,
-                       kPoliciesNotInitializedMessage,
-                       error_callback);
+      InvokeErrorCallback(
+          service_path, error_callback, kPoliciesNotInitialized);
       return;
     }
     const base::DictionaryValue* policy =
@@ -246,10 +228,7 @@ void ManagedNetworkConfigurationHandlerImpl::SetProperties(
       network_state_handler_->GetNetworkState(service_path);
 
   if (!state) {
-    RunErrorCallback(service_path,
-                     kUnknownServicePath,
-                     kUnknownServicePathMessage,
-                     error_callback);
+    InvokeErrorCallback(service_path, error_callback, kUnknownServicePath);
     return;
   }
 
@@ -258,10 +237,8 @@ void ManagedNetworkConfigurationHandlerImpl::SetProperties(
     // TODO(pneubeck): create an initial configuration in this case. As for
     // CreateConfiguration, user settings from older ChromeOS versions have to
     // determined here.
-    RunErrorCallback(service_path,
-                     kSetOnUnconfiguredNetwork,
-                     kSetOnUnconfiguredNetworkMessage,
-                     error_callback);
+    InvokeErrorCallback(
+        service_path, error_callback, kSetOnUnconfiguredNetwork);
     return;
   }
 
@@ -269,10 +246,7 @@ void ManagedNetworkConfigurationHandlerImpl::SetProperties(
   const NetworkProfile *profile =
       network_profile_handler_->GetProfileForPath(profile_path);
   if (!profile) {
-    RunErrorCallback(service_path,
-                     kUnknownProfilePath,
-                     kUnknownProfilePathMessage,
-                     error_callback);
+    InvokeErrorCallback(service_path, error_callback, kUnknownProfilePath);
     return;
   }
 
@@ -281,10 +255,7 @@ void ManagedNetworkConfigurationHandlerImpl::SetProperties(
 
   const Policies* policies = GetPoliciesForProfile(*profile);
   if (!policies) {
-    RunErrorCallback(service_path,
-                     kPoliciesNotInitialized,
-                     kPoliciesNotInitializedMessage,
-                     error_callback);
+    InvokeErrorCallback(service_path, error_callback, kPoliciesNotInitialized);
     return;
   }
 
@@ -303,10 +274,7 @@ void ManagedNetworkConfigurationHandlerImpl::SetProperties(
           &validation_result);
 
   if (validation_result == onc::Validator::INVALID) {
-    RunErrorCallback(service_path,
-                     kInvalidUserSettings,
-                     kInvalidUserSettingsMessage,
-                     error_callback);
+    InvokeErrorCallback(service_path, error_callback, kInvalidUserSettings);
     return;
   }
   if (validation_result == onc::Validator::VALID_WITH_WARNINGS)
@@ -331,28 +299,21 @@ void ManagedNetworkConfigurationHandlerImpl::CreateConfiguration(
     const network_handler::ErrorCallback& error_callback) const {
   const Policies* policies = GetPoliciesForUser(userhash);
   if (!policies) {
-    RunErrorCallback("",
-                     kPoliciesNotInitialized,
-                     kPoliciesNotInitializedMessage,
-                     error_callback);
+    InvokeErrorCallback("", error_callback, kPoliciesNotInitialized);
     return;
   }
 
   if (policy_util::FindMatchingPolicy(policies->per_network_config,
                                       properties)) {
-    RunErrorCallback("",
-                     kNetworkAlreadyConfigured,
-                     kNetworkAlreadyConfiguredMessage,
-                     error_callback);
+    InvokeErrorCallback("", error_callback, kNetworkAlreadyConfigured);
+    return;
   }
 
   const NetworkProfile* profile =
       network_profile_handler_->GetProfileForUserhash(userhash);
   if (!profile) {
-    RunErrorCallback("",
-                     kProfileNotInitialized,
-                     kProfileNotInitializedMessage,
-                     error_callback);
+    InvokeErrorCallback("", error_callback, kProfileNotInitialized);
+    return;
   }
 
   // TODO(pneubeck): In case of WiFi, check that no other configuration for the
diff --git a/chromeos/network/network_configuration_handler.cc b/chromeos/network/network_configuration_handler.cc
index f464dee..750c33f 100644
--- a/chromeos/network/network_configuration_handler.cc
+++ b/chromeos/network/network_configuration_handler.cc
@@ -38,15 +38,13 @@ std::string StripQuotations(const std::string& in_str) {
   return in_str;
 }
 
-void InvokeErrorCallback(const std::string& error,
-                         const std::string& path,
-                         const network_handler::ErrorCallback& error_callback) {
-  NET_LOG_ERROR(error, path);
-  if (error_callback.is_null())
-    return;
-  scoped_ptr<base::DictionaryValue> error_data(
-      network_handler::CreateErrorData(path, error, ""));
-  error_callback.Run(error, error_data.Pass());
+void InvokeErrorCallback(const std::string& service_path,
+                         const network_handler::ErrorCallback& error_callback,
+                         const std::string& error_name) {
+  std::string error_msg = "Config Error: " + error_name;
+  NET_LOG_ERROR(error_msg, service_path);
+  network_handler::RunErrorCallback(
+      error_callback, service_path, error_name, error_msg);
 }
 
 void GetPropertiesCallback(
@@ -65,14 +63,10 @@ void GetPropertiesCallback(
   if (call_status != DBUS_METHOD_CALL_SUCCESS) {
     // Because network services are added and removed frequently, we will see
     // failures regularly, so don't log these.
-    if (!error_callback.is_null()) {
-      scoped_ptr<base::DictionaryValue> error_data(
-          network_handler::CreateErrorData(
-              service_path,
-              network_handler::kDBusFailedError,
-              network_handler::kDBusFailedErrorMessage));
-      error_callback.Run(network_handler::kDBusFailedError, error_data.Pass());
-    }
+    network_handler::RunErrorCallback(error_callback,
+                                      service_path,
+                                      network_handler::kDBusFailedError,
+                                      network_handler::kDBusFailedErrorMessage);
   } else if (!callback.is_null()) {
     callback.Run(service_path, *properties_copy.get());
   }
@@ -147,7 +141,7 @@ class NetworkConfigurationHandler::ProfileEntryDeleter
       const base::DictionaryValue& profile_entries) {
     if (call_status != DBUS_METHOD_CALL_SUCCESS) {
       InvokeErrorCallback(
-          "GetLoadableProfileEntries Failed", service_path_, error_callback_);
+          service_path_, error_callback_, "GetLoadableProfileEntriesFailed");
       owner_->ProfileEntryDeleterCompleted(service_path_);  // Deletes this.
       return;
     }
@@ -311,9 +305,9 @@ void NetworkConfigurationHandler::RemoveConfiguration(
     const network_handler::ErrorCallback& error_callback) {
   // Service.Remove is not reliable. Instead, request the profile entries
   // for the service and remove each entry.
-  if (profile_entry_deleters_.count(service_path)) {
+  if (ContainsKey(profile_entry_deleters_,service_path)) {
     InvokeErrorCallback(
-        "RemoveConfiguration In-Progress", service_path, error_callback);
+        service_path, error_callback, "RemoveConfigurationInProgress");
     return;
   }
   NET_LOG_USER("Remove Configuration", service_path);
diff --git a/chromeos/network/network_connection_handler.cc b/chromeos/network/network_connection_handler.cc
index 1f127c6..1418bfa 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -33,11 +33,8 @@ void InvokeErrorCallback(const std::string& service_path,
                          const std::string& error_name) {
   std::string error_msg = "Connect Error: " + error_name;
   NET_LOG_ERROR(error_msg, service_path);
-  if (error_callback.is_null())
-    return;
-  scoped_ptr<base::DictionaryValue> error_data(
-      network_handler::CreateErrorData(service_path, error_name, error_msg));
-  error_callback.Run(error_name, error_data.Pass());
+  network_handler::RunErrorCallback(
+      error_callback, service_path, error_name, error_msg);
 }
 
 bool IsAuthenticationError(const std::string& error) {
@@ -609,9 +606,8 @@ void NetworkConnectionHandler::CheckPendingRequest(
   pending_requests_.erase(service_path);
   if (error_callback.is_null())
     return;
-  scoped_ptr<base::DictionaryValue> error_data(
-      network_handler::CreateErrorData(service_path, error_name, shill_error));
-  error_callback.Run(error_name, error_data.Pass());
+  network_handler::RunErrorCallback(
+      error_callback, service_path, error_name, shill_error);
 }
 
 void NetworkConnectionHandler::CheckAllPendingRequests() {
diff --git a/chromeos/network/network_handler_callbacks.cc b/chromeos/network/network_handler_callbacks.cc
index e34d549..6b54074 100644
--- a/chromeos/network/network_handler_callbacks.cc
+++ b/chromeos/network/network_handler_callbacks.cc
@@ -22,10 +22,21 @@ const char kDbusErrorName[] = "dbusErrorName";
 const char kDbusErrorMessage[] = "dbusErrorMessage";
 const char kPath[] = "path";
 
-base::DictionaryValue* CreateErrorData(const std::string& service_path,
+base::DictionaryValue* CreateErrorData(const std::string& path,
                                        const std::string& error_name,
                                        const std::string& error_detail) {
-  return CreateDBusErrorData(service_path, error_name, error_detail, "", "");
+  return CreateDBusErrorData(path, error_name, error_detail, "", "");
+}
+
+void RunErrorCallback(const ErrorCallback& error_callback,
+                      const std::string& path,
+                      const std::string& error_name,
+                      const std::string& error_detail) {
+  if (error_callback.is_null())
+    return;
+  error_callback.Run(
+      error_name,
+      make_scoped_ptr(CreateErrorData(path, error_name, error_detail)));
 }
 
 base::DictionaryValue* CreateDBusErrorData(
@@ -65,22 +76,17 @@ void ShillErrorCallbackFunction(const std::string& error_name,
   error_callback.Run(error_name, error_data.Pass());
 }
 
-void GetPropertiesCallback(
-    const network_handler::DictionaryResultCallback& callback,
-    const network_handler::ErrorCallback& error_callback,
-    const std::string& path,
-    DBusMethodCallStatus call_status,
-    const base::DictionaryValue& value) {
+void GetPropertiesCallback(const DictionaryResultCallback& callback,
+                           const ErrorCallback& error_callback,
+                           const std::string& path,
+                           DBusMethodCallStatus call_status,
+                           const base::DictionaryValue& value) {
   if (call_status != DBUS_METHOD_CALL_SUCCESS) {
-    scoped_ptr<base::DictionaryValue> error_data(
-        network_handler::CreateErrorData(path,
-                                         kDBusFailedError,
-                                         kDBusFailedErrorMessage));
     NET_LOG_ERROR(
         base::StringPrintf("GetProperties failed. Status: %d", call_status),
         path);
-    if (!error_callback.is_null())
-      error_callback.Run(kDBusFailedError, error_data.Pass());
+    RunErrorCallback(
+        error_callback, path, kDBusFailedError, kDBusFailedErrorMessage);
   } else if (!callback.is_null()) {
     callback.Run(path, value);
   }
diff --git a/chromeos/network/network_handler_callbacks.h b/chromeos/network/network_handler_callbacks.h
index f939ef3..4bdd687 100644
--- a/chromeos/network/network_handler_callbacks.h
+++ b/chromeos/network/network_handler_callbacks.h
@@ -9,6 +9,7 @@
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/dbus/dbus_method_call_status.h"
 
@@ -45,6 +46,13 @@ CHROMEOS_EXPORT base::DictionaryValue* CreateErrorData(
     const std::string& error_name,
     const std::string& error_detail);
 
+// If not NULL, runs |error_callback| with an ErrorData dictionary created from
+// the other arguments.
+CHROMEOS_EXPORT void RunErrorCallback(const ErrorCallback& error_callback,
+                                      const std::string& path,
+                                      const std::string& error_name,
+                                      const std::string& error_detail);
+
 CHROMEOS_EXPORT base::DictionaryValue* CreateDBusErrorData(
     const std::string& path,
     const std::string& error_name,
@@ -69,8 +77,8 @@ CHROMEOS_EXPORT void ShillErrorCallbackFunction(
 // the DBus Dictionary callback into one that calls the error callback
 // if |call_status| != DBUS_METHOD_CALL_SUCCESS.
 CHROMEOS_EXPORT void GetPropertiesCallback(
-    const network_handler::DictionaryResultCallback& callback,
-    const network_handler::ErrorCallback& error_callback,
+    const DictionaryResultCallback& callback,
+    const ErrorCallback& error_callback,
     const std::string& path,
     DBusMethodCallStatus call_status,
     const base::DictionaryValue& value);