Use networkingPrivate.startConnect

This adds checks for unconfigured or non-activated networks to internet_details.js since it no longer uses the checks in network_connect.cc.
It also moves some notification handling from network_connect.cc to network_state_notifier.cc so that it does not rely on network_connect.cc.

BUG=430115

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

Cr-Commit-Position: refs/heads/master@{#325108}

12 files changed, 99 insertions, 49 deletions

diff --git a/chromeos/dbus/fake_shill_service_client.cc b/chromeos/dbus/fake_shill_service_client.cc
index d4e789c3..fdae9db 100644
--- a/chromeos/dbus/fake_shill_service_client.cc
+++ b/chromeos/dbus/fake_shill_service_client.cc
@@ -178,8 +178,8 @@ void FakeShillServiceClient::Connect(const dbus::ObjectPath& service_path,
                                      const ErrorCallback& error_callback) {
   VLOG(1) << "FakeShillServiceClient::Connect: " << service_path.value();
   base::DictionaryValue* service_properties = NULL;
-  if (!stub_services_.GetDictionary(
-          service_path.value(), &service_properties)) {
+  if (!stub_services_.GetDictionary(service_path.value(),
+                                    &service_properties)) {
     LOG(ERROR) << "Service not found: " << service_path.value();
     error_callback.Run("Error.InvalidService", "Invalid Service");
     return;
@@ -190,21 +190,22 @@ void FakeShillServiceClient::Connect(const dbus::ObjectPath& service_path,
   // sending an update.
   SetOtherServicesOffline(service_path.value());
 
+  // Clear Error.
+  service_properties->SetStringWithoutPathExpansion(shill::kErrorProperty, "");
+
   // Set Associating.
   base::StringValue associating_value(shill::kStateAssociation);
-  SetServiceProperty(service_path.value(),
-                     shill::kStateProperty,
+  SetServiceProperty(service_path.value(), shill::kStateProperty,
                      associating_value);
 
   // Stay Associating until the state is changed again after a delay.
   base::MessageLoop::current()->PostDelayedTask(
       FROM_HERE,
       base::Bind(&FakeShillServiceClient::ContinueConnect,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 service_path.value()),
+                 weak_ptr_factory_.GetWeakPtr(), service_path.value()),
       base::TimeDelta::FromSeconds(GetInteractiveDelay()));
 
-  callback.Run();
+  base::MessageLoop::current()->PostTask(FROM_HERE, callback);
 }
 
 void FakeShillServiceClient::Disconnect(const dbus::ObjectPath& service_path,
@@ -606,8 +607,7 @@ void FakeShillServiceClient::SetCellularActivated(
               error_callback);
 }
 
-void FakeShillServiceClient::ContinueConnect(
-    const std::string& service_path) {
+void FakeShillServiceClient::ContinueConnect(const std::string& service_path) {
   VLOG(1) << "FakeShillServiceClient::ContinueConnect: " << service_path;
   base::DictionaryValue* service_properties = NULL;
   if (!stub_services_.GetDictionary(service_path, &service_properties)) {
@@ -625,26 +625,24 @@ void FakeShillServiceClient::ContinueConnect(
 
   // No custom connect behavior set, continue with the default connect behavior.
   std::string passphrase;
-  service_properties->GetStringWithoutPathExpansion(
-      shill::kPassphraseProperty, &passphrase);
+  service_properties->GetStringWithoutPathExpansion(shill::kPassphraseProperty,
+                                                    &passphrase);
   if (passphrase == "failure") {
     // Simulate a password failure.
-    SetServiceProperty(service_path,
-                       shill::kStateProperty,
+    SetServiceProperty(service_path, shill::kErrorProperty,
+                       base::StringValue(shill::kErrorBadPassphrase));
+    SetServiceProperty(service_path, shill::kStateProperty,
                        base::StringValue(shill::kStateFailure));
     base::MessageLoop::current()->PostTask(
         FROM_HERE,
         base::Bind(
             base::IgnoreResult(&FakeShillServiceClient::SetServiceProperty),
-            weak_ptr_factory_.GetWeakPtr(),
-            service_path,
-            shill::kErrorProperty,
+            weak_ptr_factory_.GetWeakPtr(), service_path, shill::kErrorProperty,
             base::StringValue(shill::kErrorBadPassphrase)));
   } else {
     // Set Online.
     VLOG(1) << "Setting state to Online " << service_path;
-    SetServiceProperty(service_path,
-                       shill::kStateProperty,
+    SetServiceProperty(service_path, shill::kStateProperty,
                        base::StringValue(shill::kStateOnline));
   }
 }
diff --git a/chromeos/network/managed_network_configuration_handler_impl.cc b/chromeos/network/managed_network_configuration_handler_impl.cc
index a92be66..f20cdcc3 100644
--- a/chromeos/network/managed_network_configuration_handler_impl.cc
+++ b/chromeos/network/managed_network_configuration_handler_impl.cc
@@ -164,9 +164,12 @@ void ManagedNetworkConfigurationHandlerImpl::SendManagedProperties(
 
   ::onc::ONCSource onc_source;
   FindPolicyByGUID(userhash, guid, &onc_source);
+  const NetworkState* network_state =
+      network_state_handler_->GetNetworkState(service_path);
   scoped_ptr<base::DictionaryValue> active_settings(
-      onc::TranslateShillServiceToONCPart(
-          *shill_properties, onc_source, &onc::kNetworkWithStateSignature));
+      onc::TranslateShillServiceToONCPart(*shill_properties, onc_source,
+                                          &onc::kNetworkWithStateSignature,
+                                          network_state));
 
   const base::DictionaryValue* network_policy = NULL;
   const base::DictionaryValue* global_policy = NULL;
@@ -213,10 +216,12 @@ void ManagedNetworkConfigurationHandlerImpl::SendProperties(
     const network_handler::ErrorCallback& error_callback,
     const std::string& service_path,
     scoped_ptr<base::DictionaryValue> shill_properties) {
+  const NetworkState* network_state =
+      network_state_handler_->GetNetworkState(service_path);
   scoped_ptr<base::DictionaryValue> onc_network(
       onc::TranslateShillServiceToONCPart(
           *shill_properties, ::onc::ONC_SOURCE_UNKNOWN,
-          &onc::kNetworkWithStateSignature));
+          &onc::kNetworkWithStateSignature, network_state));
   callback.Run(service_path, *onc_network);
 }
 
diff --git a/chromeos/network/network_state.cc b/chromeos/network/network_state.cc
index f4ad959..93c12e4 100644
--- a/chromeos/network/network_state.cc
+++ b/chromeos/network/network_state.cc
@@ -231,8 +231,6 @@ void NetworkState::GetStateProperties(base::DictionaryValue* dictionary) const {
                                             profile_path());
 
   if (visible()) {
-    if (!error().empty())
-      dictionary->SetStringWithoutPathExpansion(shill::kErrorProperty, error());
     dictionary->SetStringWithoutPathExpansion(shill::kStateProperty,
                                               connection_state());
   }
@@ -398,6 +396,12 @@ bool NetworkState::UpdateName(const base::DictionaryValue& properties) {
   return false;
 }
 
+std::string NetworkState::GetErrorState() const {
+  if (ErrorIsValid(error()))
+    return error();
+  return last_error();
+}
+
 // static
 bool NetworkState::StateIsConnected(const std::string& connection_state) {
   return (connection_state == shill::kStateReady ||
diff --git a/chromeos/network/network_state.h b/chromeos/network/network_state.h
index e4d9cce..518e568 100644
--- a/chromeos/network/network_state.h
+++ b/chromeos/network/network_state.h
@@ -126,6 +126,9 @@ class CHROMEOS_EXPORT NetworkState : public ManagedState {
   // Set the GUID. Called exclusively by NetworkStateHandler.
   void SetGuid(const std::string& guid);
 
+  // Returns |error_| if valid, otherwise returns |last_error_|.
+  std::string GetErrorState() const;
+
   // Helpers (used e.g. when a state, error, or shill dictionary is cached)
   static bool StateIsConnected(const std::string& connection_state);
   static bool StateIsConnecting(const std::string& connection_state);
diff --git a/chromeos/network/network_state_handler.cc b/chromeos/network/network_state_handler.cc
index 236789c..a8a9ebf 100644
--- a/chromeos/network/network_state_handler.cc
+++ b/chromeos/network/network_state_handler.cc
@@ -417,6 +417,16 @@ const NetworkState* NetworkStateHandler::GetEAPForEthernet(
   return list.front();
 }
 
+void NetworkStateHandler::SetLastErrorForTest(const std::string& service_path,
+                                              const std::string& error) {
+  NetworkState* network_state = GetModifiableNetworkState(service_path);
+  if (!network_state) {
+    LOG(ERROR) << "No matching NetworkState for: " << service_path;
+    return;
+  }
+  network_state->last_error_ = error;
+}
+
 //------------------------------------------------------------------------------
 // ShillPropertyHandler::Delegate overrides
 
diff --git a/chromeos/network/network_state_handler.h b/chromeos/network/network_state_handler.h
index 135d225..f032f77 100644
--- a/chromeos/network/network_state_handler.h
+++ b/chromeos/network/network_state_handler.h
@@ -224,6 +224,10 @@ class CHROMEOS_EXPORT NetworkStateHandler
     return default_network_path_;
   }
 
+  // Sets the |last_error_| property of the matching NetworkState for tests.
+  void SetLastErrorForTest(const std::string& service_path,
+                           const std::string& error);
+
   // Constructs and initializes an instance for testing.
   static NetworkStateHandler* InitializeForTest();
 
diff --git a/chromeos/network/network_util.cc b/chromeos/network/network_util.cc
index b5d50b3..5fa7de5 100644
--- a/chromeos/network/network_util.cc
+++ b/chromeos/network/network_util.cc
@@ -179,7 +179,7 @@ scoped_ptr<base::DictionaryValue> TranslateNetworkStateToONC(
 
   scoped_ptr<base::DictionaryValue> onc_dictionary =
       TranslateShillServiceToONCPart(*shill_dictionary, onc_source,
-                                     &onc::kNetworkWithStateSignature);
+                                     &onc::kNetworkWithStateSignature, network);
   return onc_dictionary.Pass();
 }
 
diff --git a/chromeos/network/onc/onc_translation_tables.cc b/chromeos/network/onc/onc_translation_tables.cc
index 285a7e1..ad7eef8 100644
--- a/chromeos/network/onc/onc_translation_tables.cc
+++ b/chromeos/network/onc/onc_translation_tables.cc
@@ -187,24 +187,23 @@ const FieldTranslationEntry cellular_fields[] = {
 const FieldTranslationEntry network_fields[] = {
     {::onc::network_config::kGUID, shill::kGuidProperty},
     {::onc::network_config::kConnectable, shill::kConnectableProperty},
-    {::onc::network_config::kErrorState, shill::kErrorProperty},
     {::onc::network_config::kPriority, shill::kPriorityProperty},
 
     // Shill doesn't allow setting the name for non-VPN networks.
     // Name is conditionally translated, see onc_translator_*.
-    // { ::onc::network_config::kName, shill::kNameProperty },
+    // {::onc::network_config::kName, shill::kNameProperty },
 
     // Type is converted during translation, see onc_translator_*.
-    // { ::onc::network_config::kType, shill::kTypeProperty },
+    // {::onc::network_config::kType, shill::kTypeProperty },
 
     // These fields are converted during translation, see
     // onc_translator_shill_to_onc.cc. They are only converted when going from
     // Shill->ONC, and ignored otherwise.
-    // { ::onc::network_config::kConnectionState, shill::kStateProperty },
-    // { ::onc::network_config::kRestrictedConnectivity,
-    //   shill::kStateProperty },
-    // { ::onc::network_config::kSource, shill::kProfileProperty },
-    // { ::onc::network_config::kMacAddress, shill::kAddressProperty },
+    // {::onc::network_config::kConnectionState, shill::kStateProperty },
+    // {::onc::network_config::kErrorState, shill::kErrorProperty},
+    // {::onc::network_config::kRestrictedConnectivity, shill::kStateProperty },
+    // {::onc::network_config::kSource, shill::kProfileProperty },
+    // {::onc::network_config::kMacAddress, shill::kAddressProperty },
     {NULL}};
 
 const FieldTranslationEntry ipconfig_fields[] = {
diff --git a/chromeos/network/onc/onc_translator.h b/chromeos/network/onc/onc_translator.h
index dc5afa3..db1cb71 100644
--- a/chromeos/network/onc/onc_translator.h
+++ b/chromeos/network/onc/onc_translator.h
@@ -14,6 +14,9 @@ class DictionaryValue;
 }
 
 namespace chromeos {
+
+class NetworkState;
+
 namespace onc {
 
 struct OncValueSignature;
@@ -39,12 +42,15 @@ scoped_ptr<base::DictionaryValue> TranslateONCObjectToShill(
 // only a subset of it and includes only the values that are actually required
 // by the UI. If |onc_source| != ONC_SOURCE_UNKNOWN then the 'Source' property
 // of the ONC dictionary will be set accordingly. Note: ONC_SOURCE_USER_IMPORT
-// is treated the same as ONC_SOURCE_NONE.
+// is treated the same as ONC_SOURCE_NONE. If |network_state| is provided it
+// will be used to set the ErrorState property. Otherwise ErrorState will not
+// be set.
 CHROMEOS_EXPORT
 scoped_ptr<base::DictionaryValue> TranslateShillServiceToONCPart(
     const base::DictionaryValue& shill_dictionary,
     ::onc::ONCSource onc_source,
-    const OncValueSignature* onc_signature);
+    const OncValueSignature* onc_signature,
+    const NetworkState* network_state);
 
 }  // namespace onc
 }  // namespace chromeos
diff --git a/chromeos/network/onc/onc_translator_shill_to_onc.cc b/chromeos/network/onc/onc_translator_shill_to_onc.cc
index d918156..aca9133 100644
--- a/chromeos/network/onc/onc_translator_shill_to_onc.cc
+++ b/chromeos/network/onc/onc_translator_shill_to_onc.cc
@@ -52,21 +52,25 @@ class ShillToONCTranslator {
  public:
   ShillToONCTranslator(const base::DictionaryValue& shill_dictionary,
                        ::onc::ONCSource onc_source,
-                       const OncValueSignature& onc_signature)
+                       const OncValueSignature& onc_signature,
+                       const NetworkState* network_state)
       : shill_dictionary_(&shill_dictionary),
         onc_source_(onc_source),
-        onc_signature_(&onc_signature) {
+        onc_signature_(&onc_signature),
+        network_state_(network_state) {
     field_translation_table_ = GetFieldTranslationTable(onc_signature);
   }
 
   ShillToONCTranslator(const base::DictionaryValue& shill_dictionary,
                        ::onc::ONCSource onc_source,
                        const OncValueSignature& onc_signature,
-                       const FieldTranslationEntry* field_translation_table)
+                       const FieldTranslationEntry* field_translation_table,
+                       const NetworkState* network_state)
       : shill_dictionary_(&shill_dictionary),
         onc_source_(onc_source),
         onc_signature_(&onc_signature),
-        field_translation_table_(field_translation_table) {}
+        field_translation_table_(field_translation_table),
+        network_state_(network_state) {}
 
   // Translates the associated Shill dictionary and creates an ONC object of the
   // given signature.
@@ -142,6 +146,7 @@ class ShillToONCTranslator {
   const OncValueSignature* onc_signature_;
   const FieldTranslationEntry* field_translation_table_;
   scoped_ptr<base::DictionaryValue> onc_object_;
+  const NetworkState* network_state_;
 
   DISALLOW_COPY_AND_ASSIGN(ShillToONCTranslator);
 };
@@ -382,9 +387,9 @@ void ShillToONCTranslator::TranslateCellularWithState() {
   if (device_dictionary) {
     // Merge the Device dictionary with this one (Cellular) using the
     // CellularDevice signature.
-    ShillToONCTranslator nested_translator(*device_dictionary, onc_source_,
-                                           kCellularWithStateSignature,
-                                           kCellularDeviceTable);
+    ShillToONCTranslator nested_translator(
+        *device_dictionary, onc_source_, kCellularWithStateSignature,
+        kCellularDeviceTable, network_state_);
     scoped_ptr<base::DictionaryValue> nested_object =
         nested_translator.CreateTranslatedONCObject();
     onc_object_->MergeDictionary(nested_object.get());
@@ -474,6 +479,17 @@ void ShillToONCTranslator::TranslateNetworkWithState() {
     }
   }
 
+  // 'ErrorState' reflects the most recent error maintained in NetworkState
+  // (which may not match Shill's Error or PreviousError properties). Non
+  // visible networks (with null network_state_) do not set ErrorState.
+  if (network_state_) {
+    std::string error_state = network_state_->GetErrorState();
+    if (!error_state.empty()) {
+      onc_object_->SetStringWithoutPathExpansion(
+          ::onc::network_config::kErrorState, error_state);
+    }
+  }
+
   std::string profile_path;
   if (onc_source_ != ::onc::ONC_SOURCE_UNKNOWN &&
       shill_dictionary_->GetStringWithoutPathExpansion(shill::kProfileProperty,
@@ -601,7 +617,8 @@ void ShillToONCTranslator::TranslateAndAddNestedObject(
     return;
   }
   ShillToONCTranslator nested_translator(dictionary, onc_source_,
-                                         *field_signature->value_signature);
+                                         *field_signature->value_signature,
+                                         network_state_);
   scoped_ptr<base::DictionaryValue> nested_object =
       nested_translator.CreateTranslatedONCObject();
   if (nested_object->empty())
@@ -642,7 +659,8 @@ void ShillToONCTranslator::TranslateAndAddListOfObjects(
       continue;
     ShillToONCTranslator nested_translator(
         *shill_value, onc_source_,
-        *field_signature->value_signature->onc_array_entry_signature);
+        *field_signature->value_signature->onc_array_entry_signature,
+        network_state_);
     scoped_ptr<base::DictionaryValue> nested_object =
         nested_translator.CreateTranslatedONCObject();
     // If the nested object couldn't be parsed, simply omit it.
@@ -728,10 +746,12 @@ std::string ShillToONCTranslator::GetName() {
 scoped_ptr<base::DictionaryValue> TranslateShillServiceToONCPart(
     const base::DictionaryValue& shill_dictionary,
     ::onc::ONCSource onc_source,
-    const OncValueSignature* onc_signature) {
+    const OncValueSignature* onc_signature,
+    const NetworkState* network_state) {
   CHECK(onc_signature != NULL);
 
-  ShillToONCTranslator translator(shill_dictionary, onc_source, *onc_signature);
+  ShillToONCTranslator translator(shill_dictionary, onc_source, *onc_signature,
+                                  network_state);
   return translator.CreateTranslatedONCObject();
 }
 
diff --git a/chromeos/network/onc/onc_translator_unittest.cc b/chromeos/network/onc/onc_translator_unittest.cc
index f637761..45701c6 100644
--- a/chromeos/network/onc/onc_translator_unittest.cc
+++ b/chromeos/network/onc/onc_translator_unittest.cc
@@ -83,7 +83,8 @@ TEST_P(ONCTranslatorShillToOncTest, Translate) {
       test_utils::ReadTestDictionary(result_onc_filename));
 
   scoped_ptr<base::DictionaryValue> translation(TranslateShillServiceToONCPart(
-      *shill_network, ::onc::ONC_SOURCE_NONE, &kNetworkWithStateSignature));
+      *shill_network, ::onc::ONC_SOURCE_NONE, &kNetworkWithStateSignature,
+      nullptr /* network_state */));
 
   EXPECT_TRUE(test_utils::Equals(expected_onc_network.get(),
                                  translation.get()));
diff --git a/chromeos/network/policy_applicator.cc b/chromeos/network/policy_applicator.cc
index 53ff817..a5dd9ff 100644
--- a/chromeos/network/policy_applicator.cc
+++ b/chromeos/network/policy_applicator.cc
@@ -131,9 +131,9 @@ void PolicyApplicator::GetEntryCallback(
           << profile_.ToDebugString();
 
   scoped_ptr<base::DictionaryValue> onc_part(
-      onc::TranslateShillServiceToONCPart(entry_properties,
-                                          ::onc::ONC_SOURCE_UNKNOWN,
-                                          &onc::kNetworkWithStateSignature));
+      onc::TranslateShillServiceToONCPart(
+          entry_properties, ::onc::ONC_SOURCE_UNKNOWN,
+          &onc::kNetworkWithStateSignature, nullptr /* network_state */));
 
   std::string old_guid;
   if (!onc_part->GetStringWithoutPathExpansion(::onc::network_config::kGUID,