Revert of Use networkingPrivate.startConnect (patchset chromium#12 id…

…:240001 of https://codereview.chromium.org/1043343002/)

Reason for revert:
Causes browser_test failure on Win7 bot.
https://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20(1)

[ RUN      ] WebNavigationApiTest.Failures
[4444:4508:0414/144708:WARNING:data_reduction_proxy_config.cc(319)] SPDY proxy OFF at startup
[4444:2028:0414/144708:ERROR:bluetooth_adapter_win.cc(102)] NOT IMPLEMENTED
[180:3848:0414/144709:WARNING:raw_channel_win.cc(473)] WriteFile: Error (0x5) while retrieving error. (0xE8)
[180:3848:0414/144709:WARNING:channel.cc(549)] Failed to send message to ack remove remote endpoint (local ID 2147483650, remote ID 3)
[180:3848:0414/144709:WARNING:channel.cc(315)] RawChannel write error
[180:3848:0414/144709:WARNING:raw_channel_win.cc(473)] WriteFile: Error (0x3B01) while retrieving error. (0xE8)
[180:3848:0414/144709:WARNING:channel.cc(549)] Failed to send message to ack remove remote endpoint (local ID 1, remote ID 1)
[180:3848:0414/144709:WARNING:channel.cc(315)] RawChannel write error
[4444:2028:0414/144709:INFO:CONSOLE(0)] "[SUCCESS] nonExistentIframe", source: chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/test_failures.html (0)
[4444:2028:0414/144710:INFO:CONSOLE(0)] "[SUCCESS] nonExistentIframeNavigation", source: chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/test_failures.html (0)
[4444:2028:0414/144710:INFO:CONSOLE(0)] "[FAIL] cancel: Received unexpected event 'onCompleted':{"frameId":0,"processId":0,"tabId":0,"timeStamp":0,"url":"chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/e.html"}
Error
    at captureEvent (chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/framework.js:194:17)
    at chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/framework.js:219:5
    at EventImpl.dispatchToListener (extensions::event_bindings:395:22)
    at Event.publicClass.(anonymous function) [as dispatchToListener] (extensions::utils:94:26)
    at EventImpl.dispatch_ (extensions::event_bindings:379:35)
    at dispatchArgs (extensions::event_bindings:247:26)
    at dispatchEvent (extensions::event_bindings:256:7)", source: chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/test_failures.html (0)
[4444:2028:0414/144710:INFO:CONSOLE(0)] "[SUCCESS] nonExistent", source: chrome-extension://ollhgkgdokpphndfogkepkmblokamlkf/test_failures.html (0)
c:\b\build\slave\win_x64_builder\build\src\chrome\browser\extensions\api\web_navigation\web_navigation_apitest.cc(468): error: Value of: RunExtensionTest("webnavigation/failures")
  Actual: false
Expected: true
Failed 1 of 4 tests
[4444:2028:0414/144710:ERROR:browser_thread.h(263)] DeleteSoon failed on thread 0
[  FAILED  ] WebNavigationApiTest.Failures, where TypeParam =  and GetParam() =  (2465 ms)

Original issue's description:
> 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
>
> Committed: https://crrev.com/e22e5bcbad2cea941993b32e6cc3b082822a42fc
> Cr-Commit-Position: refs/heads/master@{#325108}

TBR=armansito@chromium.org,michaelpg@chromium.org,pneubeck@chromium.org,stevenjb@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=430115

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

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

chrome/browser/chromeos/options/network_config_view.cc

@@ -25,7 +25,6 @@
 #include "chromeos/login/login_state.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
-#include "components/device_event_log/device_event_log.h"
 #include "components/user_manager/user.h"
 #include "ui/accessibility/ax_view_state.h"
 #include "ui/aura/window_event_dispatcher.h"
@@ -137,7 +136,6 @@ void NetworkConfigView::Show(const std::string& service_path,
     delete view;
     return;
   }
-  NET_LOG(USER) << "NetworkConfigView::Show: " << service_path;
   view->ShowDialog(parent);
 }
 
@@ -153,7 +151,6 @@ void NetworkConfigView::ShowForType(const std::string& type,
     delete view;
     return;
   }
-  NET_LOG(USER) << "NetworkConfigView::ShowForType: " << type;
   view->ShowDialog(parent);
 }
 

chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc

@@ -20,8 +20,6 @@
 #include "chromeos/dbus/shill_profile_client.h"
 #include "chromeos/dbus/shill_service_client.h"
 #include "chromeos/login/user_names.h"
-#include "chromeos/network/network_handler.h"
-#include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/onc/onc_utils.h"
 #include "chromeos/network/portal_detector/network_portal_detector.h"
 #include "components/onc/onc_constants.h"
@@ -548,12 +546,6 @@ IN_PROC_BROWSER_TEST_F(NetworkingPrivateChromeOSApiTest, GetManagedProperties) {
   EXPECT_TRUE(RunNetworkingSubtest("getManagedProperties")) << message_;
 }
 
-IN_PROC_BROWSER_TEST_F(NetworkingPrivateChromeOSApiTest, GetErrorState) {
-  chromeos::NetworkHandler::Get()->network_state_handler()->SetLastErrorForTest(
-      kWifi1ServicePath, "TestErrorState");
-  EXPECT_TRUE(RunNetworkingSubtest("getErrorState")) << message_;
-}
-
 IN_PROC_BROWSER_TEST_F(NetworkingPrivateChromeOSApiTest,
                        OnNetworksChangedEventConnect) {
   EXPECT_TRUE(RunNetworkingSubtest("onNetworksChangedEventConnect"))

chrome/browser/resources/options/chromeos/internet_detail.js

@@ -1189,62 +1189,14 @@ cr.define('options.internet', function() {
   };
 
   DetailsInternetPage.loginFromDetails = function() {
-    DetailsInternetPage.configureOrConnect();
-    PageManager.closeOverlay();
-  };
-
-  /**
-   * This function identifies unconfigured networks and networks that are
-   * likely to fail (e.g. due to a bad passphrase on a previous connect
-   * attempt). For such networks a configure dialog will be opened. Otherwise
-   * a connection will be attempted.
-   */
-  DetailsInternetPage.configureOrConnect = function() {
     var detailsPage = DetailsInternetPage.getInstance();
     if (detailsPage.type_ == 'WiFi')
       sendChromeMetricsAction('Options_NetworkConnectToWifi');
     else if (detailsPage.type_ == 'VPN')
       sendChromeMetricsAction('Options_NetworkConnectToVPN');
-
-    var onc = detailsPage.onc_;
-    var guid = onc.guid();
-    var type = onc.getActiveValue('Type');
-
-    // VPNs do not correctly set 'Connectable', so we always show the
-    // configuration UI.
-    if (type == 'VPN') {
-      chrome.send('configureNetwork', [guid]);
-      return;
-    }
-
-    // If 'Connectable' is false for WiFi or WiMAX, Shill requires
-    // additional configuration to connect, so show the configuration UI.
-    if ((type == 'WiFi' || type == 'WiMAX') &&
-        !onc.getActiveValue('Connectable')) {
-      chrome.send('configureNetwork', [guid]);
-      return;
-    }
-
-    // Secure WiFi networks with ErrorState set most likely require
-    // configuration (e.g. a correct passphrase) before connecting.
-    if (type == 'WiFi' && onc.getWiFiSecurity() != 'None') {
-      var errorState = onc.getActiveValue('ErrorState');
-      if (errorState && errorState != 'Unknown') {
-        chrome.send('configureNetwork', [guid]);
-        return;
-      }
-    }
-
-    // Cellular networks need to be activated before they can be connected to.
-    if (type == 'Cellular') {
-      var activationState = onc.getActiveValue('Cellular.ActivationState');
-      if (activationState != 'Activated' && activationState != 'Unknown') {
-        DetailsInternetPage.activateCellular(guid);
-        return;
-      }
-    }
-
-    chrome.networkingPrivate.startConnect(guid);
+    // TODO(stevenjb): chrome.networkingPrivate.startConnect
+    chrome.send('startConnect', [detailsPage.onc_.guid()]);
+    PageManager.closeOverlay();
   };
 
   DetailsInternetPage.disconnectNetwork = function() {

chrome/browser/ui/webui/options/chromeos/internet_options_handler.cc

@@ -91,6 +91,10 @@ const char kUpdateConnectionDataFunction[] =
 const char kShowMorePlanInfoMessage[] = "showMorePlanInfo";
 const char kSimOperationMessage[] = "simOperation";
 
+// TODO(stevenjb): Replace these with the matching networkingPrivate methods.
+// crbug.com/279351.
+const char kStartConnectMessage[] = "startConnect";
+
 // TODO(stevenjb): Deprecate this once we handle events in the JS.
 const char kSetNetworkGuidMessage[] = "setNetworkGuid";
 
@@ -256,6 +260,11 @@ void InternetOptionsHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(kSetNetworkGuidMessage,
       base::Bind(&InternetOptionsHandler::SetNetworkGuidCallback,
                  base::Unretained(this)));
+
+  // networkingPrivate methods
+  web_ui()->RegisterMessageCallback(kStartConnectMessage,
+      base::Bind(&InternetOptionsHandler::StartConnectCallback,
+                 base::Unretained(this)));
 }
 
 void InternetOptionsHandler::OnExtensionLoaded(
@@ -337,6 +346,23 @@ void InternetOptionsHandler::SetNetworkGuidCallback(
   details_guid_ = guid;
 }
 
+
+////////////////////////////////////////////////////////////////////////////////
+// networkingPrivate implementation methods. TODO(stevenjb): Use the
+// networkingPrivate API directly in the settings JS and deprecate these
+// methods. crbug.com/279351.
+
+void InternetOptionsHandler::StartConnectCallback(const base::ListValue* args) {
+  std::string guid;
+  if (!args->GetString(0, &guid)) {
+    NOTREACHED();
+    return;
+  }
+  std::string service_path = ServicePathFromGuid(guid);
+  if (!service_path.empty())
+    ui::NetworkConnect::Get()->ConnectToNetwork(service_path);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 
 void InternetOptionsHandler::UpdateVPNProviders() {

chrome/browser/ui/webui/options/chromeos/internet_options_handler.h

@@ -68,6 +68,8 @@ class InternetOptionsHandler : public ::options::OptionsPageUIHandler,
 
   // networkingPrvate callbacks
   void GetManagedPropertiesCallback(const base::ListValue* args);
+  void StartConnectCallback(const base::ListValue* args);
+  void SetPropertiesCallback(const base::ListValue* args);
 
   // Updates the list of VPN providers enabled in the primary user's profile.
   void UpdateVPNProviders();

chrome/test/data/extensions/api_test/networking_private/chromeos/test.js

@@ -616,19 +616,6 @@ var availableTests = [
       'non_existent',
       callbackFail('Error.InvalidNetworkGuid'));
   },
-  function getErrorState() {
-    // Both getState and getProperties should have ErrorState set.
-    chrome.networkingPrivate.getState(
-      'stub_wifi1_guid',
-      function(result) {
-        assertEq('TestErrorState', result.ErrorState);
-        chrome.networkingPrivate.getProperties(
-          'stub_wifi1_guid',
-          callbackPass(function(result2) {
-            assertEq('TestErrorState', result2.ErrorState);
-          }));
-      });
-  },
   function onNetworksChangedEventConnect() {
     var network = 'stub_wifi2_guid';
     var done = chrome.test.callbackAdded();

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,22 +190,21 @@ 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()));
 
-  base::MessageLoop::current()->PostTask(FROM_HERE, callback);
+  callback.Run();
 }
 
 void FakeShillServiceClient::Disconnect(const dbus::ObjectPath& service_path,
@@ -607,7 +606,8 @@ 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,24 +625,26 @@ void FakeShillServiceClient::ContinueConnect(const std::string& service_path) {
 
   // 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::kErrorProperty,
-                       base::StringValue(shill::kErrorBadPassphrase));
-    SetServiceProperty(service_path, shill::kStateProperty,
+    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));
   }
 }

chromeos/network/managed_network_configuration_handler_impl.cc

@@ -164,12 +164,9 @@ 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,
-                                          network_state));
+      onc::TranslateShillServiceToONCPart(
+          *shill_properties, onc_source, &onc::kNetworkWithStateSignature));
 
   const base::DictionaryValue* network_policy = NULL;
   const base::DictionaryValue* global_policy = NULL;
@@ -216,12 +213,10 @@ 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, network_state));
+          &onc::kNetworkWithStateSignature));
   callback.Run(service_path, *onc_network);
 }
 

chromeos/network/network_state.cc

@@ -231,6 +231,8 @@ 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());
   }
@@ -396,12 +398,6 @@ 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 ||

chromeos/network/network_state.h

@@ -126,9 +126,6 @@ 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);

chromeos/network/network_state_handler.cc

@@ -417,16 +417,6 @@ 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
 

chromeos/network/network_state_handler.h

@@ -224,10 +224,6 @@ 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();
 

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, network);
+                                     &onc::kNetworkWithStateSignature);
   return onc_dictionary.Pass();
 }