Allow networkingPrivate to request configure UI

Currently if networkingPrivate attempts to connect
to an unconfigured network, the request fails and the
UI is expected to handle configuraiton.

Until network configuration is fully integrated into the
new Settings UI, we need Chrome to handle the configuration
UI.

This CL:
* Adds HandleConnectFailed to NetworkingPrivateDelegate::UIDelegate
* Suppresses a redundant notification when a connect to an
  unconfigured network fails.
Also:
* Updates the logging in the notification code
* Handles "expected" errors in network_summary.js to avoid "unhandled lastError" messages when requesting a connect to an unconfigured network.

BUG=515987

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

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

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

@@ -120,6 +120,10 @@ class UIDelegateStub : public NetworkingPrivateDelegate::UIDelegate {
   void ShowAccountDetails(const std::string& guid) const override {
     ++s_show_account_details_called_;
   }
+  bool HandleConnectFailed(const std::string& guid,
+                           const std::string error) const override {
+    return false;
+  }
 };
 
 // static

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

@@ -25,5 +25,17 @@ void NetworkingPrivateUIDelegateChromeOS::ShowAccountDetails(
   ui::NetworkConnect::Get()->ShowMobileSetup(network->path());
 }
 
+bool NetworkingPrivateUIDelegateChromeOS::HandleConnectFailed(
+    const std::string& guid,
+    const std::string error) const {
+  const NetworkState* network =
+      NetworkHandler::Get()->network_state_handler()->GetNetworkStateFromGuid(
+          guid);
+  if (!network || network->path().empty())
+    return false;
+  return ui::NetworkConnect::Get()->MaybeShowConfigureUI(network->path(),
+                                                         error);
+}
+
 }  // namespace extensions
 }  // namespace chromeos

chrome/browser/extensions/api/networking_private/networking_private_ui_delegate_chromeos.h

@@ -19,6 +19,8 @@ class NetworkingPrivateUIDelegateChromeOS
 
   // NetworkingPrivateDelegate::UIDelegate
   void ShowAccountDetails(const std::string& guid) const override;
+  bool HandleConnectFailed(const std::string& guid,
+                           const std::string error) const override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(NetworkingPrivateUIDelegateChromeOS);

chrome/browser/resources/settings/internet_page/network_summary.js

@@ -208,6 +208,13 @@ Polymer({
         chrome.networkingPrivate.getState(
             id,
             function(state) {
+              if (chrome.runtime.lastError) {
+                if (chrome.runtime.lastError != 'Error.NetworkUnavailable') {
+                  console.error('Unexpected networkingPrivate.getState error:',
+                                chrome.runtime.lastError);
+                }
+                return;
+              }
               // Async call, ensure id still exists.
               if (!this.networkIds_[id])
                 return;
@@ -228,7 +235,13 @@ Polymer({
    * @private
    */
   connectToNetwork_: function(state) {
-    chrome.networkingPrivate.startConnect(state.GUID);
+    chrome.networkingPrivate.startConnect(state.GUID, function() {
+      if (chrome.runtime.lastError &&
+          chrome.runtime.lastError != 'connecting') {
+        console.error('Unexpected networkingPrivate.startConnect error:',
+                      chrome.runtime.lastError);
+      }
+    });
   },
 
   /**

extensions/browser/api/networking_private/networking_private_chromeos.cc

@@ -166,8 +166,8 @@ NetworkingPrivateChromeOS::NetworkingPrivateChromeOS(
     content::BrowserContext* browser_context,
     scoped_ptr<VerifyDelegate> verify_delegate)
     : NetworkingPrivateDelegate(verify_delegate.Pass()),
-      browser_context_(browser_context) {
-}
+      browser_context_(browser_context),
+      weak_ptr_factory_(this) {}
 
 NetworkingPrivateChromeOS::~NetworkingPrivateChromeOS() {
 }
@@ -310,10 +310,28 @@ void NetworkingPrivateChromeOS::StartConnect(
   const bool check_error_state = false;
   NetworkHandler::Get()->network_connection_handler()->ConnectToNetwork(
       service_path, success_callback,
-      base::Bind(&NetworkHandlerFailureCallback, failure_callback),
+      base::Bind(&NetworkingPrivateChromeOS::ConnectFailureCallback,
+                 weak_ptr_factory_.GetWeakPtr(), guid, success_callback,
+                 failure_callback),
       check_error_state);
 }
 
+void NetworkingPrivateChromeOS::ConnectFailureCallback(
+    const std::string& guid,
+    const VoidCallback& success_callback,
+    const FailureCallback& failure_callback,
+    const std::string& error_name,
+    scoped_ptr<base::DictionaryValue> error_data) {
+  // TODO(stevenjb): Temporary workaround to show the configuration UI.
+  // Eventually the caller (e.g. Settings) should handle any failures and
+  // show its own configuration UI. crbug.com/380937.
+  if (ui_delegate()->HandleConnectFailed(guid, error_name)) {
+    success_callback.Run();
+    return;
+  }
+  failure_callback.Run(error_name);
+}
+
 void NetworkingPrivateChromeOS::StartDisconnect(
     const std::string& guid,
     const VoidCallback& success_callback,

extensions/browser/api/networking_private/networking_private_chromeos.h

@@ -5,6 +5,7 @@
 #ifndef EXTENSIONS_BROWSER_API_NETWORKING_PRIVATE_NETWORKING_PRIVATE_CHROMEOS_H_
 #define EXTENSIONS_BROWSER_API_NETWORKING_PRIVATE_NETWORKING_PRIVATE_CHROMEOS_H_
 
+#include "base/memory/weak_ptr.h"
 #include "extensions/browser/api/networking_private/networking_private_delegate.h"
 
 namespace context {
@@ -88,7 +89,14 @@ class NetworkingPrivateChromeOS : public NetworkingPrivateDelegate {
   bool RequestScan() override;
 
  private:
+  void ConnectFailureCallback(const std::string& guid,
+                              const VoidCallback& success_callback,
+                              const FailureCallback& failure_callback,
+                              const std::string& error_name,
+                              scoped_ptr<base::DictionaryValue> error_data);
+
   content::BrowserContext* browser_context_;
+  base::WeakPtrFactory<NetworkingPrivateChromeOS> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkingPrivateChromeOS);
 };

extensions/browser/api/networking_private/networking_private_delegate.h

@@ -90,6 +90,11 @@ class NetworkingPrivateDelegate : public KeyedService {
     // with |guid|.
     virtual void ShowAccountDetails(const std::string& guid) const = 0;
 
+    // Possibly handle a connection failure, e.g. by showing the configuration
+    // UI. Returns true if the error was handled, i.e. the UI was shown.
+    virtual bool HandleConnectFailed(const std::string& guid,
+                                     const std::string error) const = 0;
+
    private:
     DISALLOW_COPY_AND_ASSIGN(UIDelegate);
   };

ui/chromeos/network/network_connect.cc

@@ -63,6 +63,8 @@ class NetworkConnectImpl : public NetworkConnect {
 
   // NetworkConnect
   void ConnectToNetwork(const std::string& service_path) override;
+  bool MaybeShowConfigureUI(const std::string& service_path,
+                            const std::string& connect_error) override;
   void SetTechnologyEnabled(const chromeos::NetworkTypePattern& technology,
                             bool enabled_state) override;
   void ActivateCellular(const std::string& service_path) override;
@@ -83,6 +85,8 @@ class NetworkConnectImpl : public NetworkConnect {
   void OnConnectFailed(const std::string& service_path,
                        const std::string& error_name,
                        scoped_ptr<base::DictionaryValue> error_data);
+  bool MaybeShowConfigureUIImpl(const std::string& service_path,
+                                const std::string& connect_error);
   bool GetNetworkProfilePath(bool shared, std::string* profile_path);
   void OnConnectSucceeded(const std::string& service_path);
   void CallConnectToNetwork(const std::string& service_path,
@@ -198,37 +202,46 @@ bool NetworkConnectImpl::GetNetworkProfilePath(bool shared,
   return true;
 }
 
-// This handles connect failures that are a direct result of a user initiated
-// connect request and result in a new UI being shown. Note: notifications are
-// handled by NetworkStateNotifier.
 void NetworkConnectImpl::OnConnectFailed(
     const std::string& service_path,
     const std::string& error_name,
     scoped_ptr<base::DictionaryValue> error_data) {
-  NET_LOG_ERROR("Connect Failed: " + error_name, service_path);
+  MaybeShowConfigureUIImpl(service_path, error_name);
+}
+
+// This handles connect failures that are a direct result of a user initiated
+// connect request and result in a new UI being shown. Note: notifications are
+// handled by NetworkStateNotifier.
+bool NetworkConnectImpl::MaybeShowConfigureUIImpl(
+    const std::string& service_path,
+    const std::string& connect_error) {
+  NET_LOG_ERROR("Connect Failed: " + connect_error, service_path);
 
-  if (error_name == NetworkConnectionHandler::kErrorBadPassphrase ||
-      error_name == NetworkConnectionHandler::kErrorPassphraseRequired ||
-      error_name == NetworkConnectionHandler::kErrorConfigurationRequired ||
-      error_name == NetworkConnectionHandler::kErrorAuthenticationRequired) {
+  if (connect_error == NetworkConnectionHandler::kErrorBadPassphrase ||
+      connect_error == NetworkConnectionHandler::kErrorPassphraseRequired ||
+      connect_error == NetworkConnectionHandler::kErrorConfigurationRequired ||
+      connect_error == NetworkConnectionHandler::kErrorAuthenticationRequired) {
     HandleUnconfiguredNetwork(service_path);
-    return;
+    return true;
   }
 
-  if (error_name == NetworkConnectionHandler::kErrorCertificateRequired) {
+  if (connect_error == NetworkConnectionHandler::kErrorCertificateRequired) {
     if (!delegate_->ShowEnrollNetwork(service_path))
       HandleUnconfiguredNetwork(service_path);
-    return;
+    return true;
   }
 
   // Only show a configure dialog if there was a ConnectFailed error. The dialog
   // allows the user to request a new connect attempt or cancel. Note: a
   // notification may also be displayed by NetworkStateNotifier in this case.
-  if (error_name == NetworkConnectionHandler::kErrorConnectFailed)
+  if (connect_error == NetworkConnectionHandler::kErrorConnectFailed) {
     HandleUnconfiguredNetwork(service_path);
+    return true;
+  }
 
   // Notifications for other connect failures are handled by
   // NetworkStateNotifier, so no need to do anything else here.
+  return false;
 }
 
 void NetworkConnectImpl::OnConnectSucceeded(const std::string& service_path) {
@@ -377,6 +390,12 @@ void NetworkConnectImpl::ConnectToNetwork(const std::string& service_path) {
   CallConnectToNetwork(service_path, check_error_state);
 }
 
+bool NetworkConnectImpl::MaybeShowConfigureUI(
+    const std::string& service_path,
+    const std::string& connect_error) {
+  return MaybeShowConfigureUIImpl(service_path, connect_error);
+}
+
 void NetworkConnectImpl::SetTechnologyEnabled(
     const NetworkTypePattern& technology,
     bool enabled_state) {

ui/chromeos/network/network_connect.h

@@ -63,6 +63,11 @@ class UI_CHROMEOS_EXPORT NetworkConnect {
   // Requests a network connection and handles any errors and notifications.
   virtual void ConnectToNetwork(const std::string& service_path) = 0;
 
+  // Maybe show the configuration UI after a connect failure based on the
+  // network state and error name. Returns true if the UI is shown.
+  virtual bool MaybeShowConfigureUI(const std::string& service_path,
+                                    const std::string& connect_error) = 0;
+
   // Enables or disables a network technology. If |technology| refers to
   // cellular and the device cannot be enabled due to a SIM lock, this function
   // will launch the SIM unlock dialog.

ui/chromeos/network/network_state_notifier.cc

@@ -11,10 +11,10 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chromeos/network/network_configuration_handler.h"
 #include "chromeos/network/network_connection_handler.h"
-#include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/shill_property_util.h"
+#include "components/device_event_log/device_event_log.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -68,11 +68,14 @@ int GetErrorNotificationIconId(const std::string& network_type) {
   return IDR_AURA_UBER_TRAY_NETWORK_FAILED;
 }
 
-void ShowErrorNotification(const std::string& notification_id,
+void ShowErrorNotification(const std::string& service_path,
+                           const std::string& notification_id,
                            const std::string& network_type,
                            const base::string16& title,
                            const base::string16& message,
                            const base::Closure& callback) {
+  NET_LOG(ERROR) << "ShowErrorNotification: " << service_path << ": "
+                 << base::UTF16ToUTF8(title);
   const gfx::Image& icon =
       ui::ResourceBundle::GetSharedInstance().GetImageNamed(
           GetErrorNotificationIconId(network_type));
@@ -219,7 +222,7 @@ void NetworkStateNotifier::UpdateCellularOutOfCredits(
     base::string16 error_msg = l10n_util::GetStringFUTF16(
         IDS_NETWORK_OUT_OF_CREDITS_BODY, base::UTF8ToUTF16(cellular->name()));
     ShowErrorNotification(
-        kNetworkOutOfCreditsNotificationId, cellular->type(),
+        cellular->path(), kNetworkOutOfCreditsNotificationId, cellular->type(),
         l10n_util::GetStringUTF16(IDS_NETWORK_OUT_OF_CREDITS_TITLE), error_msg,
         base::Bind(&NetworkStateNotifier::ShowNetworkSettingsForPath,
                    weak_ptr_factory_.GetWeakPtr(), cellular->path()));
@@ -282,8 +285,8 @@ void NetworkStateNotifier::ShowMobileActivationError(
       NetworkHandler::Get()->network_state_handler()->GetNetworkState(
           service_path);
   if (!cellular || cellular->type() != shill::kTypeCellular) {
-    NET_LOG_ERROR("ShowMobileActivationError without Cellular network",
-                  service_path);
+    NET_LOG(ERROR) << "ShowMobileActivationError without Cellular network: "
+                   << service_path;
     return;
   }
   message_center::MessageCenter::Get()->AddNotification(
@@ -337,6 +340,9 @@ void NetworkStateNotifier::ShowConnectErrorNotification(
     const std::string& service_path,
     const base::DictionaryValue& shill_properties) {
   base::string16 error = GetConnectErrorString(error_name);
+  NET_LOG(DEBUG) << "Notify: " << service_path
+                 << ": Connect error: " << error_name << ": "
+                 << base::UTF16ToUTF8(error);
   if (error.empty()) {
     std::string shill_error;
     shill_properties.GetStringWithoutPathExpansion(shill::kErrorProperty,
@@ -344,12 +350,13 @@ void NetworkStateNotifier::ShowConnectErrorNotification(
     if (!chromeos::NetworkState::ErrorIsValid(shill_error)) {
       shill_properties.GetStringWithoutPathExpansion(
           shill::kPreviousErrorProperty, &shill_error);
-      NET_LOG_DEBUG("Notify Service.PreviousError: " + shill_error,
-                    service_path);
+      NET_LOG(DEBUG) << "Notify: " << service_path
+                     << ": Service.PreviousError: " << shill_error;
       if (!chromeos::NetworkState::ErrorIsValid(shill_error))
         shill_error.clear();
     } else {
-      NET_LOG_DEBUG("Notify Service.Error: " + shill_error, service_path);
+      NET_LOG(DEBUG) << "Notify: " << service_path
+                     << ": Service.Error: " << shill_error;
     }
 
     const NetworkState* network =
@@ -360,23 +367,32 @@ void NetworkStateNotifier::ShowConnectErrorNotification(
       // TODO(stevenjb): This shouldn't ever be necessary, but is kept here as
       // a failsafe since more information is better than less when debugging
       // and we have encountered some strange edge cases before.
-      NET_LOG_DEBUG("Notify Network.last_error: " + network->last_error(),
-                    service_path);
+      NET_LOG(DEBUG) << "Notify: " << service_path
+                     << ": Network.last_error: " << network->last_error();
       if (shill_error.empty())
         shill_error = network->last_error();
     }
 
     if (ShillErrorIsIgnored(shill_error)) {
-      NET_LOG_DEBUG("Notify Ignoring error: " + error_name, service_path);
+      NET_LOG(DEBUG) << "Notify: " << service_path
+                     << ": Ignoring error: " << error_name;
       return;
     }
 
     error = network_connect_->GetShillErrorString(shill_error, service_path);
-    if (error.empty())
+    if (error.empty()) {
+      if (error_name == NetworkConnectionHandler::kErrorConnectFailed &&
+          network && !network->connectable()) {
+        // Connect failure on non connectable network with no additional
+        // information. We expect the UI to show configuration UI so do not
+        // show an additional (and unhelpful) notification.
+        return;
+      }
       error = l10n_util::GetStringUTF16(IDS_CHROMEOS_NETWORK_ERROR_UNKNOWN);
+    }
   }
-  NET_LOG_ERROR("Notify connect error: " + base::UTF16ToUTF8(error),
-                service_path);
+  NET_LOG(ERROR) << "Notify: " << service_path
+                 << ": Connect error: " + base::UTF16ToUTF8(error);
 
   std::string network_name =
       chromeos::shill_property_util::GetNameFromProperties(service_path,
@@ -406,7 +422,7 @@ void NetworkStateNotifier::ShowConnectErrorNotification(
                                                  &network_type);
 
   ShowErrorNotification(
-      kNetworkConnectNotificationId, network_type,
+      service_path, kNetworkConnectNotificationId, network_type,
       l10n_util::GetStringUTF16(IDS_NETWORK_CONNECTION_ERROR_TITLE), error_msg,
       base::Bind(&NetworkStateNotifier::ShowNetworkSettingsForPath,
                  weak_ptr_factory_.GetWeakPtr(), service_path));
@@ -417,7 +433,7 @@ void NetworkStateNotifier::ShowVpnDisconnectedNotification(
   base::string16 error_msg = l10n_util::GetStringFUTF16(
       IDS_NETWORK_VPN_CONNECTION_LOST_BODY, base::UTF8ToUTF16(vpn->name()));
   ShowErrorNotification(
-      kNetworkConnectNotificationId, shill::kTypeVPN,
+      vpn->path(), kNetworkConnectNotificationId, shill::kTypeVPN,
       l10n_util::GetStringUTF16(IDS_NETWORK_VPN_CONNECTION_LOST_TITLE),
       error_msg, base::Bind(&NetworkStateNotifier::ShowNetworkSettingsForPath,
                             weak_ptr_factory_.GetWeakPtr(), vpn->path()));