Check for network connectivity to decide if the Assistant should run

Previously we checked for network connections, which gave a false
positive if the connected network had no internet connection.

The network connectivity is checked using the (new) CrosNetworkConfig service.

TBR=msarda@chromium.org

Bug: 960956
Change-Id: Id315812b3db1f5445ffdfb95011638e48abafd19
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1643953
Commit-Queue: Jeroen Dhollander <jeroendh@google.com>
Reviewed-by: Tao Wu <wutao@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672240}

chrome/browser/profiles/profile_impl.cc

@@ -1341,8 +1341,7 @@ std::unique_ptr<service_manager::Service> ProfileImpl::HandleServiceRequest(
 #if BUILDFLAG(ENABLE_CROS_ASSISTANT)
   if (service_name == chromeos::assistant::mojom::kServiceName) {
     return std::make_unique<chromeos::assistant::Service>(
-        std::move(request), content::GetNetworkConnectionTracker(),
-        GetURLLoaderFactory()->Clone());
+        std::move(request), GetURLLoaderFactory()->Clone());
   }
 #endif  // BUILDFLAG(ENABLE_CROS_ASSISTANT)
 

chromeos/services/assistant/BUILD.gn

@@ -102,6 +102,7 @@ component("lib") {
       "//chromeos/assistant/internal/proto/google3",
       "//chromeos/dbus",
       "//chromeos/resources",
+      "//chromeos/services/network_config/public/mojom",
       "//chromeos/strings",
       "//libassistant/shared/internal_api:fuchsia_api_helper",
       "//libassistant/shared/internal_api/c:api_wrappers_entrypoint",
@@ -151,12 +152,14 @@ source_set("tests") {
   if (enable_cros_libassistant) {
     sources += [
       "platform/audio_output_provider_impl_unittest.cc",
+      "platform/network_provider_impl_unittest.cc",
       "platform/power_manager_provider_impl_unittest.cc",
       "platform/system_provider_impl_unittest.cc",
     ]
 
     deps += [
       "//chromeos/dbus",
+      "//chromeos/services/network_config/public/mojom",
       "//libassistant/shared/public",
       "//services/device/public/cpp:test_support",
     ]

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -133,7 +133,6 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
     service_manager::Connector* connector,
     device::mojom::BatteryMonitorPtr battery_monitor,
     Service* service,
-    network::NetworkConnectionTracker* network_connection_tracker,
     std::unique_ptr<network::SharedURLLoaderFactoryInfo>
         url_loader_factory_info)
     : media_session_(std::make_unique<AssistantMediaSession>(connector, this)),
@@ -153,7 +152,7 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
   platform_api_ = std::make_unique<PlatformApiImpl>(
       connector, media_session_.get(), std::move(battery_monitor),
       service_->main_task_runner(), background_thread_.task_runner(),
-      network_connection_tracker, service->assistant_state()->locale().value());
+      service->assistant_state()->locale().value());
 
   media_session::mojom::MediaControllerManagerPtr controller_manager_ptr;
   connector->BindInterface(media_session::mojom::kServiceName,

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -95,7 +95,6 @@ class AssistantManagerServiceImpl
       service_manager::Connector* connector,
       device::mojom::BatteryMonitorPtr battery_monitor,
       Service* service,
-      network::NetworkConnectionTracker* network_connection_tracker,
       std::unique_ptr<network::SharedURLLoaderFactoryInfo>
           url_loader_factory_info);
 

chromeos/services/assistant/platform/network_provider_impl.cc

@@ -3,53 +3,78 @@
 // found in the LICENSE file.
 
 #include "chromeos/services/assistant/platform/network_provider_impl.h"
+
+#include <algorithm>
+
 #include "base/bind.h"
+#include "chromeos/services/network_config/public/mojom/constants.mojom.h"
+#include "chromeos/services/network_config/public/mojom/cros_network_config.mojom-forward.h"
+#include "services/service_manager/public/cpp/connector.h"
 
-using assistant_client::NetworkProvider;
 using ConnectionStatus = assistant_client::NetworkProvider::ConnectionStatus;
+using NetworkStatePropertiesPtr =
+    chromeos::network_config::mojom::NetworkStatePropertiesPtr;
+using ConnectionStateType =
+    chromeos::network_config::mojom::ConnectionStateType;
 
 namespace chromeos {
 namespace assistant {
 
-NetworkProviderImpl::NetworkProviderImpl(
-    network::NetworkConnectionTracker* network_connection_tracker)
-    : network_connection_tracker_(network_connection_tracker),
-      weak_factory_(this) {
-  if (network_connection_tracker_) {
-    network_connection_tracker_->AddNetworkConnectionObserver(this);
-    network_connection_tracker_->GetConnectionType(
-        &connection_type_,
-        base::BindOnce(&NetworkProviderImpl::OnConnectionChanged,
-                       weak_factory_.GetWeakPtr()));
-  }
+NetworkProviderImpl::NetworkProviderImpl(service_manager::Connector* connector)
+    : connection_status_(ConnectionStatus::UNKNOWN), binding_(this) {
+  // |connector| can be null for the unittests
+  if (connector)
+    Init(connector);
+}
+
+NetworkProviderImpl::~NetworkProviderImpl() = default;
+
+network_config::mojom::CrosNetworkConfigObserverPtr
+NetworkProviderImpl::BindAndGetPtr() {
+  DCHECK(!binding_.is_bound());
+  network_config::mojom::CrosNetworkConfigObserverPtr observer_ptr;
+  binding_.Bind(mojo::MakeRequest(&observer_ptr));
+  return observer_ptr;
+}
+
+void NetworkProviderImpl::OnActiveNetworksChanged(
+    std::vector<network_config::mojom::NetworkStatePropertiesPtr> networks) {
+  const bool is_any_network_online =
+      std::any_of(networks.begin(), networks.end(), [](const auto& network) {
+        return network->connection_state == ConnectionStateType::kOnline;
+      });
+
+  if (is_any_network_online)
+    connection_status_ = ConnectionStatus::CONNECTED;
+  else
+    connection_status_ = ConnectionStatus::DISCONNECTED_FROM_INTERNET;
+}
+
+void NetworkProviderImpl::Init(service_manager::Connector* connector) {
+  BindCrosNetworkConfig(connector);
+  AddAndFireCrosNetworkConfigObserver();
 }
 
-NetworkProviderImpl::~NetworkProviderImpl() {
-  if (network_connection_tracker_)
-    network_connection_tracker_->RemoveNetworkConnectionObserver(this);
+void NetworkProviderImpl::BindCrosNetworkConfig(
+    service_manager::Connector* connector) {
+  DCHECK(!cros_network_config_ptr_.is_bound());
+  connector->BindInterface(chromeos::network_config::mojom::kServiceName,
+                           &cros_network_config_ptr_);
 }
 
-void NetworkProviderImpl::OnConnectionChanged(
-    network::mojom::ConnectionType type) {
-  connection_type_ = type;
+void NetworkProviderImpl::AddAndFireCrosNetworkConfigObserver() {
+  cros_network_config_ptr_->AddObserver(BindAndGetPtr());
+  cros_network_config_ptr_->GetNetworkStateList(
+      network_config::mojom::NetworkFilter::New(
+          network_config::mojom::FilterType::kActive,
+          network_config::mojom::NetworkType::kAll,
+          network_config::mojom::kNoLimit),
+      base::BindOnce(&NetworkProviderImpl::OnActiveNetworksChanged,
+                     base::Unretained(this)));
 }
 
 ConnectionStatus NetworkProviderImpl::GetConnectionStatus() {
-  // TODO(updowndota): Check actual internect connectivity in addition to the
-  // physical connectivity.
-  switch (connection_type_) {
-    case network::mojom::ConnectionType::CONNECTION_UNKNOWN:
-      return ConnectionStatus::UNKNOWN;
-    case network::mojom::ConnectionType::CONNECTION_ETHERNET:
-    case network::mojom::ConnectionType::CONNECTION_WIFI:
-    case network::mojom::ConnectionType::CONNECTION_2G:
-    case network::mojom::ConnectionType::CONNECTION_3G:
-    case network::mojom::ConnectionType::CONNECTION_4G:
-    case network::mojom::ConnectionType::CONNECTION_BLUETOOTH:
-      return ConnectionStatus::CONNECTED;
-    case network::mojom::ConnectionType::CONNECTION_NONE:
-      return ConnectionStatus::DISCONNECTED_FROM_INTERNET;
-  }
+  return connection_status_;
 }
 
 // Mdns responder is not supported in ChromeOS.

chromeos/services/assistant/platform/network_provider_impl.h

@@ -6,33 +6,46 @@
 #define CHROMEOS_SERVICES_ASSISTANT_PLATFORM_NETWORK_PROVIDER_IMPL_H_
 
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
+#include "chromeos/services/network_config/public/mojom/cros_network_config.mojom.h"
 #include "libassistant/shared/public/platform_net.h"
-#include "services/network/public/cpp/network_connection_tracker.h"
-#include "services/network/public/mojom/network_change_manager.mojom.h"
+#include "mojo/public/cpp/bindings/binding.h"
+
+namespace service_manager {
+class Connector;
+}  // namespace service_manager
 
 namespace chromeos {
 namespace assistant {
 
-class NetworkProviderImpl
+class COMPONENT_EXPORT(ASSISTANT_SERVICE) NetworkProviderImpl
     : public assistant_client::NetworkProvider,
-      public network::NetworkConnectionTracker::NetworkConnectionObserver {
+      public network_config::mojom::CrosNetworkConfigObserver {
  public:
-  explicit NetworkProviderImpl(
-      network::NetworkConnectionTracker* network_connection_tracker);
+  NetworkProviderImpl(service_manager::Connector* connector);
   ~NetworkProviderImpl() override;
 
-  // network::NetworkConnectionTracker::NetworkConnectionObserver:
-  void OnConnectionChanged(network::mojom::ConnectionType type) override;
-
-  // assistant_client::NetworkProvider::NetworkChangeObserver overrides:
+  // assistant_client::NetworkProvider:
   ConnectionStatus GetConnectionStatus() override;
   assistant_client::MdnsResponder* GetMdnsResponder() override;
 
+  network_config::mojom::CrosNetworkConfigObserverPtr BindAndGetPtr();
+
+  // network_config::mojom::CrosNetworkConfigObserver:
+  void OnActiveNetworksChanged(
+      std::vector<network_config::mojom::NetworkStatePropertiesPtr> networks)
+      override;
+  void OnNetworkStateListChanged() override {}
+  void OnDeviceStateListChanged() override {}
+
  private:
-  network::NetworkConnectionTracker* network_connection_tracker_;
-  network::mojom::ConnectionType connection_type_;
-  base::WeakPtrFactory<NetworkProviderImpl> weak_factory_;
+  void Init(service_manager::Connector* connector);
+
+  void BindCrosNetworkConfig(service_manager::Connector* connector);
+  void AddAndFireCrosNetworkConfigObserver();
+
+  ConnectionStatus connection_status_;
+  mojo::Binding<network_config::mojom::CrosNetworkConfigObserver> binding_;
+  network_config::mojom::CrosNetworkConfigPtr cros_network_config_ptr_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkProviderImpl);
 };

chromeos/services/assistant/platform/network_provider_impl_unittest.cc

+// Copyright 2019 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/services/assistant/platform/network_provider_impl.h"
+
+#include <utility>
+#include <vector>
+
+#include "base/macros.h"
+#include "chromeos/services/network_config/public/mojom/cros_network_config.mojom-forward.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace assistant {
+
+using network_config::mojom::ConnectionStateType;
+using network_config::mojom::NetworkStatePropertiesPtr;
+using ConnectionStatus = NetworkProviderImpl::ConnectionStatus;
+
+class NetworkProviderImplTest : public ::testing::Test {
+ public:
+  NetworkProviderImplTest() : network_provider_(nullptr) {}
+  ~NetworkProviderImplTest() override {}
+
+  void PublishConnectionStateType(ConnectionStateType connection_type) {
+    std::vector<NetworkStatePropertiesPtr> active_networks;
+    active_networks.push_back(CreateNetworkState(connection_type));
+
+    PublishActiveNetworks(std::move(active_networks));
+  }
+
+  NetworkStatePropertiesPtr CreateNetworkState(
+      ConnectionStateType connection_type) const {
+    NetworkStatePropertiesPtr network_state =
+        network_config::mojom::NetworkStateProperties::New();
+    network_state->connection_state = connection_type;
+    return network_state;
+  }
+
+  void PublishActiveNetworks(
+      std::vector<NetworkStatePropertiesPtr> active_networks) {
+    network_provider_.OnActiveNetworksChanged(std::move(active_networks));
+  }
+
+  std::vector<std::pair<ConnectionStateType, ConnectionStatus>>
+  GetStatusPairs() {
+    return {
+        {ConnectionStateType::kOnline, ConnectionStatus::CONNECTED},
+        {ConnectionStateType::kConnected,
+         ConnectionStatus::DISCONNECTED_FROM_INTERNET},
+        {ConnectionStateType::kPortal,
+         ConnectionStatus::DISCONNECTED_FROM_INTERNET},
+        {ConnectionStateType::kNotConnected,
+         ConnectionStatus::DISCONNECTED_FROM_INTERNET},
+    };
+  }
+
+ protected:
+  NetworkProviderImpl network_provider_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkProviderImplTest);
+};
+
+TEST_F(NetworkProviderImplTest, StartWithStatusUnknown) {
+  EXPECT_EQ(ConnectionStatus::UNKNOWN, network_provider_.GetConnectionStatus());
+}
+
+TEST_F(NetworkProviderImplTest, ChangeStateBasedOnConnectionStateType) {
+  for (const auto& test : GetStatusPairs()) {
+    ConnectionStateType input = test.first;
+    ConnectionStatus expected = test.second;
+
+    PublishConnectionStateType(input);
+
+    EXPECT_EQ(expected, network_provider_.GetConnectionStatus())
+        << "Failure with input " << input;
+  }
+}
+
+TEST_F(NetworkProviderImplTest, IsOnlineIfOneOfTheActiveNetworksIsOnline) {
+  std::vector<NetworkStatePropertiesPtr> active_networks{};
+  active_networks.push_back(
+      CreateNetworkState(ConnectionStateType::kNotConnected));
+  active_networks.push_back(CreateNetworkState(ConnectionStateType::kOnline));
+
+  PublishActiveNetworks(std::move(active_networks));
+
+  EXPECT_EQ(ConnectionStatus::CONNECTED,
+            network_provider_.GetConnectionStatus());
+}
+
+TEST_F(NetworkProviderImplTest, IsOfflineIfThereAreNoNetworks) {
+  PublishActiveNetworks({});
+
+  EXPECT_EQ(ConnectionStatus::DISCONNECTED_FROM_INTERNET,
+            network_provider_.GetConnectionStatus());
+}
+
+}  // namespace assistant
+}  // namespace chromeos

chromeos/services/assistant/platform_api_impl.cc

@@ -82,7 +82,6 @@ PlatformApiImpl::PlatformApiImpl(
     device::mojom::BatteryMonitorPtr battery_monitor,
     scoped_refptr<base::SequencedTaskRunner> main_thread_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> background_task_runner,
-    network::NetworkConnectionTracker* network_connection_tracker,
     std::string pref_locale)
     : audio_input_provider_(connector,
                             media::AudioDeviceDescription::kDefaultDeviceId,
@@ -91,7 +90,7 @@ PlatformApiImpl::PlatformApiImpl(
                              media_session,
                              background_task_runner,
                              media::AudioDeviceDescription::kDefaultDeviceId),
-      network_provider_(network_connection_tracker),
+      network_provider_(connector),
       pref_locale_(pref_locale) {
   // Only enable native power features if they are supported by the UI.
   std::unique_ptr<PowerManagerProviderImpl> provider;

chromeos/services/assistant/platform_api_impl.h

@@ -40,7 +40,6 @@ class PlatformApiImpl : public assistant_client::PlatformApi,
       device::mojom::BatteryMonitorPtr battery_monitor,
       scoped_refptr<base::SequencedTaskRunner> main_thread_task_runner,
       scoped_refptr<base::SingleThreadTaskRunner> background_task_runner,
-      network::NetworkConnectionTracker* network_connection_tracker,
       std::string pref_locale);
   ~PlatformApiImpl() override;
 
@@ -102,6 +101,9 @@ class PlatformApiImpl : public assistant_client::PlatformApi,
   std::unique_ptr<SystemProviderImpl> system_provider_;
   std::string pref_locale_;
 
+  chromeos::network_config::mojom::CrosNetworkConfigPtr
+      cros_network_config_ptr_;
+
   DISALLOW_COPY_AND_ASSIGN(PlatformApiImpl);
 };
 

chromeos/services/assistant/public/cpp/BUILD.gn

@@ -11,6 +11,7 @@ source_set("manifest") {
   deps = [
     "//base",
     "//chromeos/services/assistant/public/mojom",
+    "//chromeos/services/network_config/public/mojom",
     "//services/service_manager/public/cpp",
   ]
 }

chromeos/services/assistant/public/cpp/manifest.cc

@@ -8,6 +8,7 @@
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
 #include "chromeos/services/assistant/public/mojom/constants.mojom.h"
 #include "chromeos/services/assistant/public/mojom/settings.mojom.h"
+#include "chromeos/services/network_config/public/mojom/constants.mojom.h"
 #include "services/service_manager/public/cpp/manifest_builder.h"
 
 namespace chromeos {
@@ -31,6 +32,9 @@ const service_manager::Manifest& GetManifest() {
           .RequireCapability("device", "device:wake_lock")
           .RequireCapability("identity", "identity_accessor")
           .RequireCapability("media_session", "app")
+          .RequireCapability(
+              chromeos::network_config::mojom::kServiceName,
+              chromeos::network_config::mojom::kNetworkConfigCapability)
 
           .Build()};
   return *manifest;

chromeos/services/assistant/service.cc

@@ -59,7 +59,6 @@ constexpr base::TimeDelta kMaxTokenRefreshDelay =
 }  // namespace
 
 Service::Service(service_manager::mojom::ServiceRequest request,
-                 network::NetworkConnectionTracker* network_connection_tracker,
                  std::unique_ptr<network::SharedURLLoaderFactoryInfo>
                      url_loader_factory_info)
     : service_binding_(this, std::move(request)),
@@ -67,7 +66,6 @@ Service::Service(service_manager::mojom::ServiceRequest request,
       token_refresh_timer_(std::make_unique<base::OneShotTimer>()),
       main_task_runner_(base::SequencedTaskRunnerHandle::Get()),
       power_manager_observer_(this),
-      network_connection_tracker_(network_connection_tracker),
       url_loader_factory_info_(std::move(url_loader_factory_info)),
       weak_ptr_factory_(this) {
   registry_.AddInterface<mojom::AssistantPlatform>(base::BindRepeating(
@@ -361,7 +359,7 @@ void Service::CreateAssistantManagerService() {
   DCHECK(url_loader_factory_info_);
   assistant_manager_service_ = std::make_unique<AssistantManagerServiceImpl>(
       service_binding_.GetConnector(), std::move(battery_monitor), this,
-      network_connection_tracker_, std::move(url_loader_factory_info_));
+      std::move(url_loader_factory_info_));
 #else
   assistant_manager_service_ =
       std::make_unique<FakeAssistantManagerServiceImpl>();

chromeos/services/assistant/service.h

@@ -40,7 +40,6 @@ class OneShotTimer;
 }  // namespace base
 
 namespace network {
-class NetworkConnectionTracker;
 class SharedURLLoaderFactoryInfo;
 }  // namespace network
 
@@ -61,7 +60,6 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) Service
       public ash::DefaultVoiceInteractionObserver {
  public:
   Service(service_manager::mojom::ServiceRequest request,
-          network::NetworkConnectionTracker* network_connection_tracker,
           std::unique_ptr<network::SharedURLLoaderFactoryInfo>
               url_loader_factory_info);
   ~Service() override;
@@ -211,7 +209,6 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) Service
       assistant_screen_context_controller_;
   ash::AssistantStateProxy assistant_state_;
 
-  network::NetworkConnectionTracker* network_connection_tracker_;
   // non-null until |assistant_manager_service_| is created.
   std::unique_ptr<network::SharedURLLoaderFactoryInfo> url_loader_factory_info_;
 

chromeos/services/assistant/service_unittest.cc

@@ -32,7 +32,7 @@ namespace assistant {
 namespace {
 constexpr base::TimeDelta kDefaultTokenExpirationDelay =
     base::TimeDelta::FromMilliseconds(1000);
-}
+}  // namespace
 
 class FakeIdentityAccessor : identity::mojom::IdentityAccessor {
  public:
@@ -170,7 +170,6 @@ class AssistantServiceTest : public testing::Test {
 
     service_ = std::make_unique<Service>(
         test_connector_factory_.RegisterInstance(mojom::kServiceName),
-        nullptr /* network_connection_tracker */,
         shared_url_loader_factory_->Clone());
 
     mock_task_runner_ = base::MakeRefCounted<base::TestMockTimeTaskRunner>(