Only install Messages PWA when supported and allowed.

Avoid installing the Android Messages PWA if prohibited by policy or if
either the phone or Chromebook does not support it. Also listen for
changes in feature state to install the app if something changes.

Bug: 884290
Change-Id: I527828e68c3578e54a1ce15cace64e4ed2c81745
Reviewed-on: https://chromium-review.googlesource.com/1250209
Commit-Queue: Jeremy Klein <jlklein@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595466}

chromeos/services/multidevice_setup/android_sms_app_installing_status_observer.cc

@@ -39,37 +39,64 @@ AndroidSmsAppInstallingStatusObserver::Factory::~Factory() = default;
 std::unique_ptr<AndroidSmsAppInstallingStatusObserver>
 AndroidSmsAppInstallingStatusObserver::Factory::BuildInstance(
     HostStatusProvider* host_status_provider,
+    FeatureStateManager* feature_state_manager,
     std::unique_ptr<AndroidSmsAppHelperDelegate>
         android_sms_app_helper_delegate) {
   return base::WrapUnique(new AndroidSmsAppInstallingStatusObserver(
-      host_status_provider, std::move(android_sms_app_helper_delegate)));
+      host_status_provider, feature_state_manager,
+      std::move(android_sms_app_helper_delegate)));
 }
 
 AndroidSmsAppInstallingStatusObserver::
     ~AndroidSmsAppInstallingStatusObserver() {
   host_status_provider_->RemoveObserver(this);
+  feature_state_manager_->RemoveObserver(this);
 }
 
 AndroidSmsAppInstallingStatusObserver::AndroidSmsAppInstallingStatusObserver(
     HostStatusProvider* host_status_provider,
+    FeatureStateManager* feature_state_manager,
     std::unique_ptr<AndroidSmsAppHelperDelegate>
         android_sms_app_helper_delegate)
     : host_status_provider_(host_status_provider),
+      feature_state_manager_(feature_state_manager),
       android_sms_app_helper_delegate_(
           std::move(android_sms_app_helper_delegate)) {
   host_status_provider_->AddObserver(this);
+  feature_state_manager_->AddObserver(this);
+  InstallPwaIfNeeded();
+}
+
+void AndroidSmsAppInstallingStatusObserver::InstallPwaIfNeeded() {
+  mojom::FeatureState feature_state =
+      feature_state_manager_->GetFeatureStates()[mojom::Feature::kMessages];
+  if (feature_state == mojom::FeatureState::kProhibitedByPolicy ||
+      feature_state == mojom::FeatureState::kNotSupportedByChromebook ||
+      feature_state == mojom::FeatureState::kNotSupportedByPhone) {
+    return;
+  }
+
+  mojom::HostStatus status(
+      host_status_provider_->GetHostWithStatus().host_status());
+  if (status !=
+          mojom::HostStatus::kHostSetLocallyButWaitingForBackendConfirmation &&
+      status != mojom::HostStatus::kHostVerified) {
+    return;
+  }
+
+  // This call is re-entrant. If the app is already installed, it will just
+  // fail silently, which is fine.
+  android_sms_app_helper_delegate_->InstallAndroidSmsApp();
 }
 
 void AndroidSmsAppInstallingStatusObserver::OnHostStatusChange(
     const HostStatusProvider::HostStatusWithDevice& host_status_with_device) {
-  mojom::HostStatus status(host_status_with_device.host_status());
-  if (status ==
-          mojom::HostStatus::kHostSetLocallyButWaitingForBackendConfirmation ||
-      status == mojom::HostStatus::kHostVerified) {
-    // This call is re-entrant. If the app is already installed, it will just
-    // fail silently, which is fine.
-    android_sms_app_helper_delegate_->InstallAndroidSmsApp();
-  }
+  InstallPwaIfNeeded();
+}
+
+void AndroidSmsAppInstallingStatusObserver::OnFeatureStatesChange(
+    const FeatureStateManager::FeatureStatesMap& feature_states_map) {
+  InstallPwaIfNeeded();
 }
 
 }  // namespace multidevice_setup

chromeos/services/multidevice_setup/android_sms_app_installing_status_observer.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "chromeos/services/multidevice_setup/feature_state_manager.h"
 #include "chromeos/services/multidevice_setup/host_status_provider.h"
 
 namespace chromeos {
@@ -17,11 +18,9 @@ class AndroidSmsAppHelperDelegate;
 
 // Listens for status changes in multidevice state and installs the Android
 // Messages PWA if needed.
-//
-// TODO(crbug.com/884290): Also observe FeatureStateManager to make sure
-// Messages is supported.
 class AndroidSmsAppInstallingStatusObserver
-    : public HostStatusProvider::Observer {
+    : public HostStatusProvider::Observer,
+      public FeatureStateManager::Observer {
  public:
   class Factory {
    public:
@@ -30,6 +29,7 @@ class AndroidSmsAppInstallingStatusObserver
     virtual ~Factory();
     virtual std::unique_ptr<AndroidSmsAppInstallingStatusObserver>
     BuildInstance(HostStatusProvider* host_status_provider,
+                  FeatureStateManager* feature_state_manager,
                   std::unique_ptr<AndroidSmsAppHelperDelegate>
                       android_sms_app_helper_delegate);
 
@@ -42,6 +42,7 @@ class AndroidSmsAppInstallingStatusObserver
  private:
   AndroidSmsAppInstallingStatusObserver(
       HostStatusProvider* host_status_provider,
+      FeatureStateManager* feature_state_manager,
       std::unique_ptr<AndroidSmsAppHelperDelegate>
           android_sms_app_helper_delegate);
 
@@ -49,7 +50,14 @@ class AndroidSmsAppInstallingStatusObserver
   void OnHostStatusChange(const HostStatusProvider::HostStatusWithDevice&
                               host_status_with_device) override;
 
+  // FeatureStateManager:;Observer:
+  void OnFeatureStatesChange(
+      const FeatureStateManager::FeatureStatesMap& feature_states_map) override;
+
+  void InstallPwaIfNeeded();
+
   HostStatusProvider* host_status_provider_;
+  FeatureStateManager* feature_state_manager_;
   std::unique_ptr<AndroidSmsAppHelperDelegate> android_sms_app_helper_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(AndroidSmsAppInstallingStatusObserver);

chromeos/services/multidevice_setup/android_sms_app_installing_status_observer_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <string>
 
+#include "chromeos/services/multidevice_setup/fake_feature_state_manager.h"
 #include "chromeos/services/multidevice_setup/fake_host_status_provider.h"
 #include "chromeos/services/multidevice_setup/public/cpp/fake_android_sms_app_helper_delegate.h"
 #include "chromeos/services/multidevice_setup/public/mojom/multidevice_setup.mojom.h"
@@ -37,11 +38,15 @@ class MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest
     fake_android_sms_app_helper_delegate_ =
         fake_android_sms_app_helper_delegate.get();
     fake_host_status_provider_ = std::make_unique<FakeHostStatusProvider>();
+    fake_feature_state_manager_ = std::make_unique<FakeFeatureStateManager>();
     android_sms_app_installing_status_observer_ =
         AndroidSmsAppInstallingStatusObserver::Factory::Get()->BuildInstance(
-            fake_host_status_provider_.get(),
+            fake_host_status_provider_.get(), fake_feature_state_manager_.get(),
             std::move(fake_android_sms_app_helper_delegate));
 
+    SetMessagesFeatureState(mojom::FeatureState::kEnabledByUser);
+    SetHostWithStatus(mojom::HostStatus::kHostVerified, GetFakePhone());
+    fake_app_helper_delegate()->Reset();
     EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
   }
 
@@ -62,8 +67,14 @@ class MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest
         .Build();
   }
 
+  void SetMessagesFeatureState(mojom::FeatureState feature_state) {
+    fake_feature_state_manager_->SetFeatureState(mojom::Feature::kMessages,
+                                                 feature_state);
+  }
+
  private:
   std::unique_ptr<FakeHostStatusProvider> fake_host_status_provider_;
+  std::unique_ptr<FakeFeatureStateManager> fake_feature_state_manager_;
   FakeAndroidSmsAppHelperDelegate* fake_android_sms_app_helper_delegate_;
 
   std::unique_ptr<AndroidSmsAppInstallingStatusObserver>
@@ -75,6 +86,10 @@ class MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest
 
 TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
        InstallsAfterHostPending) {
+  SetMessagesFeatureState(mojom::FeatureState::kUnavailableNoVerifiedHost);
+  fake_app_helper_delegate()->Reset();
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+
   SetHostWithStatus(mojom::HostStatus::kEligibleHostExistsButNoHostSet,
                     base::nullopt /* host_device */);
   EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
@@ -94,6 +109,73 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
   SetHostWithStatus(mojom::HostStatus::kHostVerified, GetFakePhone());
   EXPECT_TRUE(fake_app_helper_delegate()->HasInstalledApp());
 }
+
+TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
+       DoesNotInstallsAfterHostVerifiedIfNotAllowed) {
+  SetMessagesFeatureState(mojom::FeatureState::kProhibitedByPolicy);
+  fake_app_helper_delegate()->Reset();
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+
+  SetHostWithStatus(mojom::HostStatus::kNoEligibleHosts,
+                    base::nullopt /* host_device */);
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+
+  SetHostWithStatus(mojom::HostStatus::kHostVerified, GetFakePhone());
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+}
+
+TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
+       DoesNotInstallsAfterHostVerifiedIfNotSupportedByPhone) {
+  SetMessagesFeatureState(mojom::FeatureState::kNotSupportedByPhone);
+  fake_app_helper_delegate()->Reset();
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+
+  SetHostWithStatus(mojom::HostStatus::kNoEligibleHosts,
+                    base::nullopt /* host_device */);
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+
+  SetHostWithStatus(mojom::HostStatus::kHostVerified, GetFakePhone());
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+}
+
+TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
+       DoesNotInstallsAfterHostVerifiedIfNotSupportedByChromebook) {
+  SetMessagesFeatureState(mojom::FeatureState::kNotSupportedByChromebook);
+  fake_app_helper_delegate()->Reset();
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+
+  SetHostWithStatus(mojom::HostStatus::kNoEligibleHosts,
+                    base::nullopt /* host_device */);
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+
+  SetHostWithStatus(mojom::HostStatus::kHostVerified, GetFakePhone());
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+}
+
+TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
+       InstallsWhenFeatureBecomesEnabled) {
+  SetMessagesFeatureState(mojom::FeatureState::kNotSupportedByChromebook);
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+  SetMessagesFeatureState(mojom::FeatureState::kEnabledByUser);
+  EXPECT_TRUE(fake_app_helper_delegate()->HasInstalledApp());
+}
+
+TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
+       InstallsEvenIfFeatureIsDisabledByUser) {
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+  SetMessagesFeatureState(mojom::FeatureState::kDisabledByUser);
+  EXPECT_TRUE(fake_app_helper_delegate()->HasInstalledApp());
+}
+
+TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
+       DoesNotInstallIfNotVerified) {
+  SetHostWithStatus(mojom::HostStatus::kNoEligibleHosts,
+                    base::nullopt /* host_device */);
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+  SetMessagesFeatureState(mojom::FeatureState::kUnavailableNoVerifiedHost);
+  EXPECT_FALSE(fake_app_helper_delegate()->HasInstalledApp());
+}
+
 }  // namespace multidevice_setup
 
 }  // namespace chromeos

chromeos/services/multidevice_setup/multidevice_setup_impl.cc

@@ -115,6 +115,7 @@ MultiDeviceSetupImpl::MultiDeviceSetupImpl(
       android_sms_app_installing_host_observer_(
           AndroidSmsAppInstallingStatusObserver::Factory::Get()->BuildInstance(
               host_status_provider_.get(),
+              feature_state_manager_.get(),
               std::move(android_sms_app_helper_delegate))),
       auth_token_validator_(auth_token_validator) {
   host_status_provider_->AddObserver(this);

chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc

@@ -398,8 +398,10 @@ class FakeAndroidSmsAppInstallingStatusObserverFactory
  public:
   FakeAndroidSmsAppInstallingStatusObserverFactory(
       FakeHostStatusProviderFactory* fake_host_status_provider_factory,
+      FakeFeatureStateManagerFactory* fake_feature_state_manager_factory,
       AndroidSmsAppHelperDelegate* expected_android_sms_app_helper_delegate)
       : fake_host_status_provider_factory_(fake_host_status_provider_factory),
+        fake_feature_state_manager_factory_(fake_feature_state_manager_factory),
         expected_android_sms_app_helper_delegate_(
             expected_android_sms_app_helper_delegate) {}
 
@@ -409,10 +411,13 @@ class FakeAndroidSmsAppInstallingStatusObserverFactory
   // AndroidSmsAppInstallingStatusObserver::Factory:
   std::unique_ptr<AndroidSmsAppInstallingStatusObserver> BuildInstance(
       HostStatusProvider* host_status_provider,
+      FeatureStateManager* feature_state_manager,
       std::unique_ptr<AndroidSmsAppHelperDelegate>
           android_sms_app_helper_delegate) override {
     EXPECT_EQ(fake_host_status_provider_factory_->instance(),
               host_status_provider);
+    EXPECT_EQ(fake_feature_state_manager_factory_->instance(),
+              feature_state_manager);
     EXPECT_EQ(expected_android_sms_app_helper_delegate_,
               android_sms_app_helper_delegate.get());
     // Only check inputs and return nullptr. We do not want to trigger the
@@ -421,6 +426,7 @@ class FakeAndroidSmsAppInstallingStatusObserverFactory
   }
 
   FakeHostStatusProviderFactory* fake_host_status_provider_factory_;
+  FakeFeatureStateManagerFactory* fake_feature_state_manager_factory_;
   AndroidSmsAppHelperDelegate* expected_android_sms_app_helper_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeAndroidSmsAppInstallingStatusObserverFactory);
@@ -514,6 +520,7 @@ class MultiDeviceSetupImplTest : public testing::Test {
     fake_android_sms_app_installing_status_observer_factory_ =
         std::make_unique<FakeAndroidSmsAppInstallingStatusObserverFactory>(
             fake_host_status_provider_factory_.get(),
+            fake_feature_state_manager_factory_.get(),
             fake_android_sms_app_helper_delegate.get());
     AndroidSmsAppInstallingStatusObserver::Factory::SetFactoryForTesting(
         fake_android_sms_app_installing_status_observer_factory_.get());