Fix 'Share with Parallels Desktop' not appearing in Files app

crrev.com/c/4019076 moved the logic for observing changes from the file
manager code to the PluginVmManager, but incorrectly used 'allowed'
state (allowed by policy) rather than 'enabled' state (allowed +
installed) to decide whether to show the share option. It also only
triggers when this state changes, so under normal operation there is
no change and the share option will not show. This CL fixes these.

Rename PluginVmPolicySubscrtiption to PluginVmAvailabilitySubscription
and also use it to track 'configured' state (whether it's installed or
not). The other consumer of this class currently separately monitors
configured state, so it's convenient to do it here too.

Bug: b/269231606
Change-Id: Ib87e5120c18661f5080f7b11b8a4758f67c540c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4273106
Commit-Queue: Timothy Loh <timloh@chromium.org>
Reviewed-by: Austin Tankiang <austinct@chromium.org>
Reviewed-by: Maggie Cai <mxcai@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108712}

chrome/browser/apps/app_service/publishers/plugin_vm_apps.cc

@@ -154,15 +154,12 @@ PluginVmApps::PluginVmApps(AppServiceProxy* proxy)
   // Register for Plugin VM changes to policy and installed state, so that we
   // can update the availability and status of the Plugin VM app. Unretained is
   // safe as these are cleaned up upon destruction.
-  policy_subscription_ =
-      std::make_unique<plugin_vm::PluginVmPolicySubscription>(
-          profile_, base::BindRepeating(&PluginVmApps::OnPluginVmAllowedChanged,
-                                        base::Unretained(this)));
+  availability_subscription_ =
+      std::make_unique<plugin_vm::PluginVmAvailabilitySubscription>(
+          profile_,
+          base::BindRepeating(&PluginVmApps::OnPluginVmAvailabilityChanged,
+                              base::Unretained(this)));
   pref_registrar_.Init(profile_->GetPrefs());
-  pref_registrar_.Add(
-      plugin_vm::prefs::kPluginVmImageExists,
-      base::BindRepeating(&PluginVmApps::OnPluginVmConfiguredChanged,
-                          base::Unretained(this)));
   for (const PermissionInfo& info : permission_infos) {
     pref_registrar_.Add(info.pref_name,
                         base::BindRepeating(&PluginVmApps::OnPermissionChanged,
@@ -377,23 +374,16 @@ AppPtr PluginVmApps::CreateApp(
   return app;
 }
 
-void PluginVmApps::OnPluginVmAllowedChanged(bool is_allowed) {
+void PluginVmApps::OnPluginVmAvailabilityChanged(bool is_allowed,
+                                                 bool is_configured) {
   // Republish the Plugin VM app when policy changes have changed
   // its availability. Only changed fields need to be republished.
   is_allowed_ = is_allowed;
 
   auto app =
       std::make_unique<App>(AppType::kPluginVm, plugin_vm::kPluginVmShelfAppId);
   SetAppAllowed(is_allowed, *app);
-  AppPublisher::Publish(std::move(app));
-}
-
-void PluginVmApps::OnPluginVmConfiguredChanged() {
-  // Only changed fields need to be republished.
-  auto app =
-      std::make_unique<App>(AppType::kPluginVm, plugin_vm::kPluginVmShelfAppId);
-  app->show_in_management =
-      plugin_vm::PluginVmFeatures::Get()->IsConfigured(profile_);
+  app->show_in_management = is_configured;
   AppPublisher::Publish(std::move(app));
 }
 

chrome/browser/apps/app_service/publishers/plugin_vm_apps.h

@@ -92,8 +92,7 @@ class PluginVmApps : public AppPublisher,
       const guest_os::GuestOsRegistryService::Registration& registration,
       bool generate_new_icon_key);
 
-  void OnPluginVmAllowedChanged(bool is_allowed);
-  void OnPluginVmConfiguredChanged();
+  void OnPluginVmAvailabilityChanged(bool is_allowed, bool is_configured);
   void OnPermissionChanged();
 
   Profile* const profile_;
@@ -104,7 +103,8 @@ class PluginVmApps : public AppPublisher,
   // Whether the Plugin VM app is allowed by policy.
   bool is_allowed_ = false;
 
-  std::unique_ptr<plugin_vm::PluginVmPolicySubscription> policy_subscription_;
+  std::unique_ptr<plugin_vm::PluginVmAvailabilitySubscription>
+      availability_subscription_;
   PrefChangeRegistrar pref_registrar_;
 };
 

chrome/browser/ash/extensions/file_manager/event_router.cc

@@ -48,8 +48,6 @@
 #include "chrome/browser/ash/guest_os/public/guest_os_service.h"
 #include "chrome/browser/ash/login/lock/screen_locker.h"
 #include "chrome/browser/ash/login/ui/login_display_host.h"
-#include "chrome/browser/ash/plugin_vm/plugin_vm_features.h"
-#include "chrome/browser/ash/plugin_vm/plugin_vm_pref_names.h"
 #include "chrome/browser/ash/plugin_vm/plugin_vm_util.h"
 #include "chrome/browser/extensions/api/file_system/chrome_file_system_delegate_ash.h"
 #include "chrome/browser/extensions/extension_service.h"

chrome/browser/ash/extensions/file_manager/event_router.h

@@ -29,7 +29,6 @@
 #include "chrome/browser/ash/guest_os/guest_os_share_path.h"
 #include "chrome/browser/ash/guest_os/public/guest_os_mount_provider.h"
 #include "chrome/browser/ash/guest_os/public/guest_os_mount_provider_registry.h"
-#include "chrome/browser/ash/plugin_vm/plugin_vm_util.h"
 #include "chrome/common/extensions/api/file_manager_private.h"
 #include "chromeos/ash/components/disks/disk_mount_manager.h"
 #include "chromeos/ash/components/settings/timezone_settings.h"
@@ -283,8 +282,6 @@ class EventRouter
       const file_manager_private::ProgressStatus& event_status);
 
   std::map<base::FilePath, std::unique_ptr<FileWatcher>> file_watchers_;
-  std::unique_ptr<plugin_vm::PluginVmPolicySubscription>
-      plugin_vm_subscription_;
   std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
   Profile* profile_;
 

chrome/browser/ash/plugin_vm/plugin_vm_manager_impl.cc

@@ -124,9 +124,15 @@ PluginVmManagerImpl::PluginVmManagerImpl(Profile* profile)
     : profile_(profile),
       owner_id_(ash::ProfileHelper::GetUserIdHashFromProfile(profile)) {
   ash::VmPluginDispatcherClient::Get()->AddObserver(this);
-  plugin_vm_subscription_ = std::make_unique<PluginVmPolicySubscription>(
-      profile_, base::BindRepeating(&PluginVmManagerImpl::OnPluginVmChanged,
-                                    weak_ptr_factory_.GetWeakPtr()));
+  availability_subscription_ =
+      std::make_unique<PluginVmAvailabilitySubscription>(
+          profile_,
+          base::BindRepeating(&PluginVmManagerImpl::OnAvailabilityChanged,
+                              weak_ptr_factory_.GetWeakPtr()));
+
+  if (PluginVmFeatures::Get()->IsEnabled(profile_)) {
+    OnAvailabilityChanged(true, true);
+  }
 }
 
 PluginVmManagerImpl::~PluginVmManagerImpl() {
@@ -793,10 +799,12 @@ void PluginVmManagerImpl::UninstallFailed(
   uninstaller_notification_.reset();
 }
 
-void PluginVmManagerImpl::OnPluginVmChanged(bool is_allowed) {
+void PluginVmManagerImpl::OnAvailabilityChanged(bool is_allowed,
+                                                bool is_configured) {
+  bool is_enabled = is_allowed && is_configured;
   auto* share_path = guest_os::GuestOsSharePath::GetForProfile(profile_);
   guest_os::GuestId id{guest_os::VmType::PLUGIN_VM, kPluginVmName, ""};
-  if (is_allowed) {
+  if (is_enabled) {
     share_path->RegisterGuest(id);
   } else {
     share_path->UnregisterGuest(id);

chrome/browser/ash/plugin_vm/plugin_vm_manager_impl.h

@@ -142,9 +142,9 @@ class PluginVmManagerImpl : public PluginVmManager,
       PluginVmUninstallerNotification::FailedReason reason =
           PluginVmUninstallerNotification::FailedReason::kUnknown);
 
-  // Called when pluginvm changes availability e.g. installed, uninstalled,
+  // Called when Plugin VM changes availability e.g. installed, uninstalled,
   // policy changes.
-  void OnPluginVmChanged(bool is_allowed);
+  void OnAvailabilityChanged(bool is_allowed, bool is_configured);
 
   Profile* profile_;
   std::string owner_id_;
@@ -192,8 +192,8 @@ class PluginVmManagerImpl : public PluginVmManager,
   // suspending, so delay until an in progress operation finishes.
   bool pending_destroy_disk_image_ = false;
 
-  // We subscribe to events which change our availability.
-  std::unique_ptr<PluginVmPolicySubscription> plugin_vm_subscription_;
+  // For notifying the GuestOsSharePath.
+  std::unique_ptr<PluginVmAvailabilitySubscription> availability_subscription_;
 
   base::WeakPtrFactory<PluginVmManagerImpl> weak_ptr_factory_{this};
 };

chrome/browser/ash/plugin_vm/plugin_vm_util.cc

@@ -142,9 +142,9 @@ bool IsPluginvmWindowId(const std::string& window_id) {
   return base::StartsWith(window_id, kPluginVmWindowId);
 }
 
-PluginVmPolicySubscription::PluginVmPolicySubscription(
+PluginVmAvailabilitySubscription::PluginVmAvailabilitySubscription(
     Profile* profile,
-    base::RepeatingCallback<void(bool)> callback)
+    AvailabilityChangeCallback callback)
     : profile_(profile), callback_(callback) {
   DCHECK(ash::CrosSettings::IsInitialized());
   ash::CrosSettings* cros_settings = ash::CrosSettings::Get();
@@ -153,35 +153,45 @@ PluginVmPolicySubscription::PluginVmPolicySubscription(
   pref_change_registrar_->Init(profile->GetPrefs());
   pref_change_registrar_->Add(
       plugin_vm::prefs::kPluginVmAllowed,
-      base::BindRepeating(&PluginVmPolicySubscription::OnPolicyChanged,
+      base::BindRepeating(&PluginVmAvailabilitySubscription::OnPolicyChanged,
                           base::Unretained(this)));
   pref_change_registrar_->Add(
       plugin_vm::prefs::kPluginVmUserId,
-      base::BindRepeating(&PluginVmPolicySubscription::OnPolicyChanged,
+      base::BindRepeating(&PluginVmAvailabilitySubscription::OnPolicyChanged,
                           base::Unretained(this)));
   pref_change_registrar_->Add(
       plugin_vm::prefs::kPluginVmImageExists,
-      base::BindRepeating(&PluginVmPolicySubscription::OnPolicyChanged,
-                          base::Unretained(this)));
+      base::BindRepeating(
+          &PluginVmAvailabilitySubscription::OnImageExistsChanged,
+          base::Unretained(this)));
   device_allowed_subscription_ = cros_settings->AddSettingsObserver(
       ash::kPluginVmAllowed,
-      base::BindRepeating(&PluginVmPolicySubscription::OnPolicyChanged,
+      base::BindRepeating(&PluginVmAvailabilitySubscription::OnPolicyChanged,
                           base::Unretained(this)));
   fake_license_subscription_ = GetFakeLicenseKeyListeners().Add(
-      base::BindRepeating(&PluginVmPolicySubscription::OnPolicyChanged,
+      base::BindRepeating(&PluginVmAvailabilitySubscription::OnPolicyChanged,
                           base::Unretained(this)));
 
   is_allowed_ = PluginVmFeatures::Get()->IsAllowed(profile);
+  is_configured_ = PluginVmFeatures::Get()->IsConfigured(profile_);
 }
 
-void PluginVmPolicySubscription::OnPolicyChanged() {
+void PluginVmAvailabilitySubscription::OnPolicyChanged() {
   bool allowed = PluginVmFeatures::Get()->IsAllowed(profile_);
   if (allowed != is_allowed_) {
     is_allowed_ = allowed;
-    callback_.Run(allowed);
+    callback_.Run(is_allowed_, is_configured_);
+  }
+}
+
+void PluginVmAvailabilitySubscription::OnImageExistsChanged() {
+  bool configured = PluginVmFeatures::Get()->IsConfigured(profile_);
+  if (configured != is_configured_) {
+    is_configured_ = configured;
+    callback_.Run(is_allowed_, is_configured_);
   }
 }
 
-PluginVmPolicySubscription::~PluginVmPolicySubscription() = default;
+PluginVmAvailabilitySubscription::~PluginVmAvailabilitySubscription() = default;
 
 }  // namespace plugin_vm

chrome/browser/ash/plugin_vm/plugin_vm_util.h

@@ -24,7 +24,7 @@ class GURL;
 
 namespace plugin_vm {
 
-class PluginVmPolicySubscription;
+class PluginVmAvailabilitySubscription;
 
 // Name of the pita DLC.
 extern const char kPitaDlc[];
@@ -99,29 +99,34 @@ absl::optional<std::string> GetIdFromDriveUrl(const GURL& url);
 // Returns true if window is PluginVM.
 bool IsPluginvmWindowId(const std::string& window_id);
 
-// A subscription for changes to PluginVm policy that may affect
-// PluginVmFeatures::Get()->IsAllowed.
-class PluginVmPolicySubscription {
+// A subscription for changes to Plugin VM's availability. The callback is
+// called whenever there are changes that would affect either
+// PluginVmFeatures::Get()->IsAllowed() or IsConfigured().
+class PluginVmAvailabilitySubscription {
  public:
-  using PluginVmAllowedChanged = base::RepeatingCallback<void(bool is_allowed)>;
-  PluginVmPolicySubscription(Profile* profile, PluginVmAllowedChanged callback);
-  ~PluginVmPolicySubscription();
+  using AvailabilityChangeCallback =
+      base::RepeatingCallback<void(bool is_allowed, bool is_configured)>;
+  PluginVmAvailabilitySubscription(Profile* profile,
+                                   AvailabilityChangeCallback callback);
+  ~PluginVmAvailabilitySubscription();
 
-  PluginVmPolicySubscription(const PluginVmPolicySubscription&) = delete;
-  PluginVmPolicySubscription& operator=(const PluginVmPolicySubscription&) =
+  PluginVmAvailabilitySubscription(const PluginVmAvailabilitySubscription&) =
       delete;
+  PluginVmAvailabilitySubscription& operator=(
+      const PluginVmAvailabilitySubscription&) = delete;
 
  private:
-  // Internal callback for policy changes.
   void OnPolicyChanged();
+  void OnImageExistsChanged();
 
   Profile* profile_;
 
   // Whether Plugin VM was previously allowed for the profile.
   bool is_allowed_;
+  bool is_configured_;
 
   // The user-provided callback method.
-  PluginVmAllowedChanged callback_;
+  AvailabilityChangeCallback callback_;
 
   std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
   base::CallbackListSubscription device_allowed_subscription_;

chrome/browser/ash/plugin_vm/plugin_vm_util_unittest.cc

@@ -28,7 +28,7 @@ class PluginVmUtilTest : public testing::Test {
   PluginVmUtilTest(const PluginVmUtilTest&) = delete;
   PluginVmUtilTest& operator=(const PluginVmUtilTest&) = delete;
 
-  MOCK_METHOD(void, OnPolicyChanged, (bool));
+  MOCK_METHOD(void, OnAvailabilityChanged, (bool, bool));
 
  protected:
   struct ScopedDBusClients {
@@ -80,45 +80,56 @@ TEST_F(PluginVmUtilTest, PluginVmShouldBeConfiguredOnceAllConditionsAreMet) {
   EXPECT_TRUE(PluginVmFeatures::Get()->IsConfigured(testing_profile_.get()));
 }
 
-TEST_F(PluginVmUtilTest, AddPluginVmPolicyObserver) {
-  const std::unique_ptr<PluginVmPolicySubscription> subscription =
-      std::make_unique<plugin_vm::PluginVmPolicySubscription>(
-          testing_profile_.get(),
-          base::BindRepeating(&PluginVmUtilTest::OnPolicyChanged,
-                              base::Unretained(this)));
+TEST_F(PluginVmUtilTest, AvailabilitySubscription) {
+  PluginVmAvailabilitySubscription subscription(
+      testing_profile_.get(),
+      base::BindRepeating(&PluginVmUtilTest::OnAvailabilityChanged,
+                          base::Unretained(this)));
+
+  // Callback args are: (is_allowed, is_configured).
 
   EXPECT_FALSE(PluginVmFeatures::Get()->IsAllowed(testing_profile_.get()));
 
-  EXPECT_CALL(*this, OnPolicyChanged(true));
+  EXPECT_CALL(*this, OnAvailabilityChanged(true, false));
   test_helper_->AllowPluginVm();
   testing::Mock::VerifyAndClearExpectations(this);
 
-  EXPECT_CALL(*this, OnPolicyChanged(false));
+  EXPECT_CALL(*this, OnAvailabilityChanged(false, false));
   testing_profile_->ScopedCrosSettingsTestHelper()->SetBoolean(
       ash::kPluginVmAllowed, false);
   testing::Mock::VerifyAndClearExpectations(this);
 
-  EXPECT_CALL(*this, OnPolicyChanged(true));
+  EXPECT_CALL(*this, OnAvailabilityChanged(true, false));
   testing_profile_->ScopedCrosSettingsTestHelper()->SetBoolean(
       ash::kPluginVmAllowed, true);
   testing::Mock::VerifyAndClearExpectations(this);
 
-  EXPECT_CALL(*this, OnPolicyChanged(false));
+  EXPECT_CALL(*this, OnAvailabilityChanged(true, true));
+  testing_profile_->GetPrefs()->SetBoolean(
+      plugin_vm::prefs::kPluginVmImageExists, true);
+  testing::Mock::VerifyAndClearExpectations(this);
+
+  EXPECT_CALL(*this, OnAvailabilityChanged(false, true));
   testing_profile_->GetPrefs()->SetBoolean(plugin_vm::prefs::kPluginVmAllowed,
                                            false);
   testing::Mock::VerifyAndClearExpectations(this);
 
-  EXPECT_CALL(*this, OnPolicyChanged(true));
+  EXPECT_CALL(*this, OnAvailabilityChanged(true, true));
   testing_profile_->GetPrefs()->SetBoolean(plugin_vm::prefs::kPluginVmAllowed,
                                            true);
   testing::Mock::VerifyAndClearExpectations(this);
 
-  EXPECT_CALL(*this, OnPolicyChanged(false));
+  EXPECT_CALL(*this, OnAvailabilityChanged(false, true));
   testing_profile_->GetPrefs()->SetString(plugin_vm::prefs::kPluginVmUserId,
                                           "");
   testing::Mock::VerifyAndClearExpectations(this);
 
-  EXPECT_CALL(*this, OnPolicyChanged(true));
+  EXPECT_CALL(*this, OnAvailabilityChanged(false, false));
+  testing_profile_->GetPrefs()->SetBoolean(
+      plugin_vm::prefs::kPluginVmImageExists, false);
+  testing::Mock::VerifyAndClearExpectations(this);
+
+  EXPECT_CALL(*this, OnAvailabilityChanged(true, false));
   const std::string kPluginVmUserId = "fancy-user-id";
   testing_profile_->GetPrefs()->SetString(plugin_vm::prefs::kPluginVmUserId,
                                           kPluginVmUserId);