AutoConnectNotifier: Improve logging and tighten logic

This also fixes a bug in AutoConnectNotifierTest, which has been "flaky"
but failing consistently for quite a while now.

For trivial removal of extra include in ash_test_helper.cc:
TBR=jamescook@chromium.org

Bug: 1015493
Change-Id: I50ed1f11c39c7385888ae4b310d686fbe4f38a63
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1864626
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708418}

ash/system/network/auto_connect_notifier.cc

@@ -13,8 +13,10 @@
 #include "base/time/time.h"
 #include "base/timer/timer.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/network_type_pattern.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/canvas_image_source.h"
@@ -79,18 +81,35 @@ void AutoConnectNotifier::ConnectToNetworkRequested(
 
 void AutoConnectNotifier::NetworkConnectionStateChanged(
     const chromeos::NetworkState* network) {
-  // No notification should be shown unless an auto-connection is underway.
-  if (!timer_->IsRunning())
+  // Ignore non WiFi networks completely.
+  if (!network->Matches(chromeos::NetworkTypePattern::WiFi()))
     return;
 
   // The notification is only shown when a connection has succeeded; if
   // |network| is not connected, there is nothing to do.
-  if (!network->IsConnectedState())
+  if (!network->IsConnectedState()) {
+    // Clear the tracked network if it is no longer connected or connecting.
+    if (!network->IsConnectingState() &&
+        network->guid() == connected_network_guid_) {
+      connected_network_guid_.clear();
+    }
+    return;
+  }
+
+  // No notification should be shown unless an auto-connection is underway.
+  if (!timer_->IsRunning()) {
+    // Track the currently connected network.
+    connected_network_guid_ = network->guid();
+    return;
+  }
+
+  // Ignore NetworkConnectionStateChanged for a previously connected network.
+  if (network->guid() == connected_network_guid_)
     return;
 
   // An auto-connected network has connected successfully. Display a
   // notification alerting the user that this has occurred.
-  DisplayNotification();
+  DisplayNotification(network);
   has_user_explicitly_requested_connection_ = false;
 }
 
@@ -125,7 +144,9 @@ void AutoConnectNotifier::OnAutoConnectedInitiated(int auto_connect_reasons) {
   timer_->Start(FROM_HERE, kNetworkConnectionTimeout, base::DoNothing());
 }
 
-void AutoConnectNotifier::DisplayNotification() {
+void AutoConnectNotifier::DisplayNotification(
+    const chromeos::NetworkState* network) {
+  NET_LOG(EVENT) << "Show AutoConnect Notification for: " << network->name();
   auto notification = ash::CreateSystemNotification(
       message_center::NotificationType::NOTIFICATION_TYPE_SIMPLE,
       kAutoConnectNotificationId,

ash/system/network/auto_connect_notifier.h

@@ -48,9 +48,10 @@ class ASH_EXPORT AutoConnectNotifier
   static const char kAutoConnectNotificationId[];
 
  private:
-  void DisplayNotification();
+  void DisplayNotification(const chromeos::NetworkState* network);
 
   bool has_user_explicitly_requested_connection_ = false;
+  std::string connected_network_guid_;
   std::unique_ptr<base::OneShotTimer> timer_;
 
   DISALLOW_COPY_AND_ASSIGN(AutoConnectNotifier);

ash/system/network/auto_connect_notifier_unittest.cc

@@ -18,6 +18,7 @@
 #include "chromeos/network/auto_connect_handler.h"
 #include "chromeos/network/network_cert_loader.h"
 #include "chromeos/network/network_handler.h"
+#include "chromeos/services/network_config/public/cpp/cros_network_config_test_helper.h"
 #include "dbus/object_path.h"
 #include "third_party/cros_system_api/dbus/shill/dbus-constants.h"
 #include "ui/message_center/message_center.h"
@@ -44,6 +45,9 @@ class AutoConnectNotifierTest : public AshTestBase {
     chromeos::shill_clients::InitializeFakes();
     chromeos::NetworkHandler::Initialize();
     CHECK(chromeos::NetworkHandler::Get()->auto_connect_handler());
+    network_config_helper_ = std::make_unique<
+        chromeos::network_config::CrosNetworkConfigTestHelper>();
+
     AshTestBase::SetUp();
 
     mock_notification_timer_ = new base::MockOneShotTimer();
@@ -54,13 +58,14 @@ class AutoConnectNotifierTest : public AshTestBase {
 
     chromeos::ShillServiceClient::Get()->GetTestInterface()->AddService(
         kTestServicePath, kTestServiceGuid, kTestServiceName, shill::kTypeWifi,
-        shill::kStateOnline, true /* visible*/);
+        shill::kStateIdle, true /* visible*/);
     // Ensure fake DBus service initialization completes.
     base::RunLoop().RunUntilIdle();
   }
 
   void TearDown() override {
     AshTestBase::TearDown();
+    network_config_helper_.reset();
     chromeos::NetworkHandler::Shutdown();
     chromeos::shill_clients::Shutdown();
     chromeos::NetworkCertLoader::Shutdown();
@@ -88,6 +93,9 @@ class AutoConnectNotifierTest : public AshTestBase {
   base::MockOneShotTimer* mock_notification_timer_;
 
  private:
+  std::unique_ptr<chromeos::network_config::CrosNetworkConfigTestHelper>
+      network_config_helper_;
+
   DISALLOW_COPY_AND_ASSIGN(AutoConnectNotifierTest);
 };
 
@@ -139,6 +147,22 @@ TEST_F(AutoConnectNotifierTest, NoConnectionBeforeTimerExpires) {
   EXPECT_FALSE(notification);
 }
 
+TEST_F(AutoConnectNotifierTest, ConnectToConnectedNetwork) {
+  SuccessfullyJoinWifiNetwork();
+
+  NotifyConnectToNetworkRequested();
+  chromeos::NetworkHandler::Get()
+      ->auto_connect_handler()
+      ->NotifyAutoConnectInitiatedForTest(
+          chromeos::AutoConnectHandler::AUTO_CONNECT_REASON_POLICY_APPLIED);
+  SuccessfullyJoinWifiNetwork();
+
+  message_center::Notification* notification =
+      message_center::MessageCenter::Get()->FindVisibleNotificationById(
+          GetNotificationId());
+  ASSERT_FALSE(notification);
+}
+
 TEST_F(AutoConnectNotifierTest, NotificationDisplayed) {
   NotifyConnectToNetworkRequested();
   chromeos::NetworkHandler::Get()

ash/test/ash_test_helper.cc

@@ -41,7 +41,6 @@
 #include "chromeos/audio/cras_audio_handler.h"
 #include "chromeos/dbus/audio/cras_audio_client.h"
 #include "chromeos/dbus/power/power_policy_controller.h"
-#include "chromeos/network/network_handler.h"
 #include "chromeos/system/fake_statistics_provider.h"
 #include "components/discardable_memory/public/mojom/discardable_shared_memory_manager.mojom.h"
 #include "components/prefs/testing_pref_service.h"

chromeos/network/auto_connect_handler.cc

@@ -258,6 +258,8 @@ void AutoConnectHandler::NotifyAutoConnectInitiatedForTest(
 }
 
 void AutoConnectHandler::NotifyAutoConnectInitiated(int auto_connect_reasons) {
+  NET_LOG(EVENT) << "AutoConnectInitiated ["
+                 << AutoConnectReasonsToString(auto_connect_reasons_) << "]";
   for (auto& observer : observer_list_)
     observer.OnAutoConnectedInitiated(auto_connect_reasons);
 }

chromeos/network/network_connection_handler_impl.cc

@@ -226,7 +226,7 @@ void NetworkConnectionHandlerImpl::ConnectToNetwork(
     const network_handler::ErrorCallback& error_callback,
     bool check_error_state,
     ConnectCallbackMode mode) {
-  NET_LOG_USER("ConnectToNetwork", service_path);
+  NET_LOG_USER("ConnectToNetworkRequested", service_path);
   for (auto& observer : observers_)
     observer.ConnectToNetworkRequested(service_path);
 

chromeos/services/network_config/public/cpp/cros_network_config_test_helper.cc

@@ -5,6 +5,7 @@
 #include "chromeos/services/network_config/public/cpp/cros_network_config_test_helper.h"
 
 #include "chromeos/network/network_device_handler.h"
+#include "chromeos/network/network_handler.h"
 #include "chromeos/services/network_config/cros_network_config.h"
 #include "chromeos/services/network_config/in_process_instance.h"
 
@@ -12,16 +13,21 @@ namespace chromeos {
 namespace network_config {
 
 CrosNetworkConfigTestHelper::CrosNetworkConfigTestHelper() {
-  network_device_handler_ =
-      chromeos::NetworkDeviceHandler::InitializeForTesting(
-          network_state_helper_.network_state_handler());
-  cros_network_config_impl_ =
-      std::make_unique<chromeos::network_config::CrosNetworkConfig>(
-          network_state_helper_.network_state_handler(),
-          network_device_handler_.get(),
-          /*network_configuration_handler=*/nullptr,
-          /*network_connection_handler=*/nullptr,
-          /*network_certificate_handler=*/nullptr);
+  if (NetworkHandler::IsInitialized()) {
+    cros_network_config_impl_ = std::make_unique<CrosNetworkConfig>();
+  } else {
+    network_state_helper_ = std::make_unique<NetworkStateTestHelper>(
+        false /* use_default_devices_and_services */);
+    network_device_handler_ =
+        chromeos::NetworkDeviceHandler::InitializeForTesting(
+            network_state_helper_->network_state_handler());
+    cros_network_config_impl_ = std::make_unique<CrosNetworkConfig>(
+        network_state_helper_->network_state_handler(),
+        network_device_handler_.get(),
+        /*network_configuration_handler=*/nullptr,
+        /*network_connection_handler=*/nullptr,
+        /*network_certificate_handler=*/nullptr);
+  }
   OverrideInProcessInstanceForTesting(cros_network_config_impl_.get());
 }
 

chromeos/services/network_config/public/cpp/cros_network_config_test_helper.h

@@ -28,12 +28,11 @@ class CrosNetworkConfigTestHelper {
   ~CrosNetworkConfigTestHelper();
 
   NetworkStateTestHelper& network_state_helper() {
-    return network_state_helper_;
+    return *network_state_helper_;
   }
 
  private:
-  NetworkStateTestHelper network_state_helper_{
-      false /* use_default_devices_and_services */};
+  std::unique_ptr<NetworkStateTestHelper> network_state_helper_;
   std::unique_ptr<NetworkDeviceHandler> network_device_handler_;
   std::unique_ptr<CrosNetworkConfig> cros_network_config_impl_;