Don't treat Shill ClearProperty errors as failures.

chromeos/network/network_configuration_handler.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_configuration_handler.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 382 matching lines @@
 void NetworkConfigurationHandler::ClearPropertiesSuccessCallback(
     const std::string& service_path,
     const std::vector<std::string>& names,
     const base::Closure& callback,
     const network_handler::ErrorCallback& error_callback,
     const base::ListValue& result) {
   const std::string kClearPropertiesFailedError("Error.ClearPropertiesFailed");
   DCHECK(names.size() == result.GetSize())
       << "Incorrect result size from ClearProperties.";
 
-  bool some_failed = false;
   for (size_t i = 0; i < result.GetSize(); ++i) {
     bool success = false;
     result.GetBoolean(i, &success);
     if (!success) {
+      // If we try to clear a property that hasn't been set the clear will

[pneubeck (no reviews), 2014/04/22 07:16:41]

can this error be distinguished from other errors?
if so, you might want to set some_failed in the other cases.

[stevenjb, 2014/04/22 19:06:23]

I'm not sure what you mean by "this error". We do not keep track of which
properties have been explicitly set (and I don't think we should try to). 
This is really a "warning", but I intentionally didn't distinguish between
"error" and "warning" in NET_LOG since they are effectively the same.

[pneubeck (no reviews), 2014/04/22 19:12:11]

Oh, I meant whether the error 'tried to clear a not-existent property' reported
by Shill can be distinguished from other Shill errors like 'service unknown'.
If so, you might want to log the error only in the former case and set
'some_failed = true' in the latter cases.

+      // fail, so just log the error.
       NET_LOG_ERROR("ClearProperties Failed: " + names[i], service_path);
-      some_failed = true;
     }
   }
 
-  if (some_failed) {
-    if (!error_callback.is_null()) {
-      scoped_ptr<base::DictionaryValue> error_data(
-          network_handler::CreateErrorData(
-              service_path, kClearPropertiesFailedError,
-              base::StringPrintf("Errors: %" PRIuS, result.GetSize())));
-      error_data->Set("errors", result.DeepCopy());
-      scoped_ptr<base::ListValue> name_list(new base::ListValue);
-      name_list->AppendStrings(names);
-      error_data->Set("names", name_list.release());
-      error_callback.Run(kClearPropertiesFailedError, error_data.Pass());
-    }
-  } else if (!callback.is_null()) {
-    callback.Run();
-  }
+  callback.Run();

[pneubeck (no reviews), 2014/04/22 07:16:41]

seems unintentional to remove 
 if (!callback.is_null())


depending on your answer to the above question, you should remove error_callback
if its never being used.

[stevenjb, 2014/04/22 19:06:23]

Good catch, and you are correct, will do.

   network_state_handler_->RequestUpdateForNetwork(service_path);
 }
 
 void NetworkConfigurationHandler::ClearPropertiesErrorCallback(
     const std::string& service_path,
     const network_handler::ErrorCallback& error_callback,
     const std::string& dbus_error_name,
     const std::string& dbus_error_message) {
   network_handler::ShillErrorCallbackFunction(
       "Config.ClearProperties Failed",
       service_path, error_callback,
       dbus_error_name, dbus_error_message);
   // Some properties may have changed so request an update regardless.
   network_state_handler_->RequestUpdateForNetwork(service_path);
 }
 
 // static
 NetworkConfigurationHandler* NetworkConfigurationHandler::InitializeForTest(
     NetworkStateHandler* network_state_handler) {
   NetworkConfigurationHandler* handler = new NetworkConfigurationHandler();
   handler->Init(network_state_handler);
   return handler;
 }
 
 }  // namespace chromeos