Don't say an app (un)install is finished before the app list is updated

Currently we show the finished message as soon as we learn the
packagekitd transaction has completed. This can lead to a noticeable
delay before the app being (un)installed is removed/added to the
launcher. This CL delays this until all pending app list updates that
were running when the operation finished, have themselves finished.

This CL depends on changes to cros_system_api in crrev.com/c/1612877

Bug: 937748
Change-Id: I57a36a5e71c7f4e96e2d1ab825ff3f20e05c33a6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1611434
Commit-Queue: Fergus Dall <sidereal@google.com>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664153}

chrome/browser/chromeos/crostini/crostini_manager.cc

@@ -1679,6 +1679,16 @@ void CrostiniManager::RemoveLinuxPackageOperationProgressObserver(
   linux_package_operation_progress_observers_.RemoveObserver(observer);
 }
 
+void CrostiniManager::AddPendingAppListUpdatesObserver(
+    PendingAppListUpdatesObserver* observer) {
+  pending_app_list_updates_observers_.AddObserver(observer);
+}
+
+void CrostiniManager::RemovePendingAppListUpdatesObserver(
+    PendingAppListUpdatesObserver* observer) {
+  pending_app_list_updates_observers_.RemoveObserver(observer);
+}
+
 void CrostiniManager::AddExportContainerProgressObserver(
     ExportContainerProgressObserver* observer) {
   export_container_progress_observers_.AddObserver(observer);
@@ -2626,4 +2636,12 @@ void CrostiniManager::OnImportLxdContainerProgress(
   }
 }
 
+void CrostiniManager::OnPendingAppListUpdates(
+    const vm_tools::cicerone::PendingAppListUpdatesSignal& signal) {
+  for (auto& observer : pending_app_list_updates_observers_) {
+    observer.OnPendingAppListUpdates(signal.vm_name(), signal.container_name(),
+                                     signal.count());
+  }
+}
+
 }  // namespace crostini

chrome/browser/chromeos/crostini/crostini_manager.h

@@ -167,6 +167,14 @@ class LinuxPackageOperationProgressObserver {
                                           int progress_percent) = 0;
 };
 
+class PendingAppListUpdatesObserver : public base::CheckedObserver {
+ public:
+  // Called whenever the kPendingAppListUpdatesMethod signal is sent.
+  virtual void OnPendingAppListUpdates(const std::string& vm_name,
+                                       const std::string& container_name,
+                                       int count) = 0;
+};
+
 class ExportContainerProgressObserver {
  public:
   // A successfully started container export will continually fire progress
@@ -566,6 +574,12 @@ class CrostiniManager : public KeyedService,
   void RemoveLinuxPackageOperationProgressObserver(
       LinuxPackageOperationProgressObserver* observer);
 
+  // Add/remove observers for pending app list updates.
+  void AddPendingAppListUpdatesObserver(
+      PendingAppListUpdatesObserver* observer);
+  void RemovePendingAppListUpdatesObserver(
+      PendingAppListUpdatesObserver* observer);
+
   // Add/remove observers for container export/import.
   void AddExportContainerProgressObserver(
       ExportContainerProgressObserver* observer);
@@ -607,6 +621,8 @@ class CrostiniManager : public KeyedService,
   void OnImportLxdContainerProgress(
       const vm_tools::cicerone::ImportLxdContainerProgressSignal& signal)
       override;
+  void OnPendingAppListUpdates(
+      const vm_tools::cicerone::PendingAppListUpdatesSignal& signal) override;
 
   void RemoveCrostini(std::string vm_name, RemoveCrostiniCallback callback);
 
@@ -885,6 +901,9 @@ class CrostiniManager : public KeyedService,
   base::ObserverList<LinuxPackageOperationProgressObserver>::Unchecked
       linux_package_operation_progress_observers_;
 
+  base::ObserverList<PendingAppListUpdatesObserver>
+      pending_app_list_updates_observers_;
+
   base::ObserverList<ExportContainerProgressObserver>::Unchecked
       export_container_progress_observers_;
   base::ObserverList<ImportContainerProgressObserver>::Unchecked

chrome/browser/chromeos/crostini/crostini_package_notification.cc

@@ -70,6 +70,10 @@ CrostiniPackageNotification::CrostiniPackageNotification(
 
 CrostiniPackageNotification::~CrostiniPackageNotification() = default;
 
+PackageOperationStatus CrostiniPackageNotification::GetOperationStatus() const {
+  return current_status_;
+}
+
 // static
 CrostiniPackageNotification::NotificationSettings
 CrostiniPackageNotification::GetNotificationSettingsForTypeAndAppName(
@@ -154,11 +158,18 @@ void CrostiniPackageNotification::UpdateProgress(PackageOperationStatus status,
           ash::kSystemNotificationColorCriticalWarning);
       break;
 
+    case PackageOperationStatus::WAITING_FOR_APP_REGISTRY_UPDATE:
+      // If a notification progress bar is set to a value outside of [0, 100],
+      // it becomes in infinite progress bar. Do that here because we have no
+      // way to know how long this will take or how close we are to completion.
+      progress_percent = -1;
+      FALLTHROUGH;
     case PackageOperationStatus::RUNNING:
       never_timeout = true;
       notification_type = message_center::NOTIFICATION_TYPE_PROGRESS;
       title = notification_settings_.progress_title;
-      if (notification_type_ == NotificationType::APPLICATION_UNINSTALL) {
+      if (notification_type_ == NotificationType::APPLICATION_UNINSTALL &&
+          progress_percent >= 0) {
         // Uninstalls have a time remaining instead of a fixed message.
         body = GetTimeRemainingMessage(running_start_time_, progress_percent);
 

chrome/browser/chromeos/crostini/crostini_package_notification.h

@@ -44,6 +44,8 @@ class CrostiniPackageNotification
 
   void ForceAllowAutoHide();
 
+  PackageOperationStatus GetOperationStatus() const;
+
   // message_center::NotificationObserver:
   void Close(bool by_user) override;
 

chrome/browser/chromeos/crostini/crostini_package_operation_status.h

@@ -11,7 +11,13 @@ namespace crostini {
 
 // Status of an operation in CrostiniPackageService &
 // CrostiniPackageNotification.
-enum class PackageOperationStatus { QUEUED, SUCCEEDED, FAILED, RUNNING };
+enum class PackageOperationStatus {
+  QUEUED,
+  SUCCEEDED,
+  FAILED,
+  RUNNING,
+  WAITING_FOR_APP_REGISTRY_UPDATE
+};
 
 }  // namespace crostini
 

chrome/browser/chromeos/crostini/crostini_package_service.cc

@@ -108,6 +108,7 @@ CrostiniPackageService::CrostiniPackageService(Profile* profile)
   CrostiniManager* manager = CrostiniManager::GetForProfile(profile);
 
   manager->AddLinuxPackageOperationProgressObserver(this);
+  manager->AddPendingAppListUpdatesObserver(this);
 }
 
 CrostiniPackageService::~CrostiniPackageService() = default;
@@ -115,6 +116,7 @@ CrostiniPackageService::~CrostiniPackageService() = default;
 void CrostiniPackageService::Shutdown() {
   CrostiniManager* manager = CrostiniManager::GetForProfile(profile_);
   manager->RemoveLinuxPackageOperationProgressObserver(this);
+  manager->RemovePendingAppListUpdatesObserver(this);
 }
 
 void CrostiniPackageService::SetNotificationStateChangeCallbackForTesting(
@@ -278,6 +280,14 @@ void CrostiniPackageService::UpdatePackageOperationStatus(
       << ContainerIdToString(container_id) << " has no notification to update";
   DCHECK(it->second) << ContainerIdToString(container_id)
                      << " has null notification pointer";
+
+  // If an operation has finished, but there are still app list updates pending,
+  // don't finish the flow yet.
+  if (status == PackageOperationStatus::SUCCEEDED &&
+      has_pending_app_list_updates_.count(container_id)) {
+    status = PackageOperationStatus::WAITING_FOR_APP_REGISTRY_UPDATE;
+  }
+
   it->second->UpdateProgress(status, progress_percent);
 
   if (status == PackageOperationStatus::SUCCEEDED ||
@@ -297,6 +307,29 @@ void CrostiniPackageService::UpdatePackageOperationStatus(
   }
 }
 
+void CrostiniPackageService::OnPendingAppListUpdates(
+    const std::string& vm_name,
+    const std::string& container_name,
+    int count) {
+  const ContainerId container_id(vm_name, container_name);
+
+  if (count != 0) {
+    has_pending_app_list_updates_.insert(container_id);
+  } else {
+    has_pending_app_list_updates_.erase(container_id);
+  }
+
+  auto it = running_notifications_.find(container_id);
+  if (it != running_notifications_.end()) {
+    if (it->second->GetOperationStatus() ==
+            PackageOperationStatus::WAITING_FOR_APP_REGISTRY_UPDATE &&
+        count == 0) {
+      UpdatePackageOperationStatus(container_id,
+                                   PackageOperationStatus::SUCCEEDED, 100);
+    }
+  }
+}
+
 void CrostiniPackageService::OnGetLinuxPackageInfo(
     const std::string& vm_name,
     const std::string& container_name,

chrome/browser/chromeos/crostini/crostini_package_service.h

@@ -25,7 +25,8 @@
 namespace crostini {
 
 class CrostiniPackageService : public KeyedService,
-                               public LinuxPackageOperationProgressObserver {
+                               public LinuxPackageOperationProgressObserver,
+                               public PendingAppListUpdatesObserver {
  public:
   using StateChangeCallback =
       base::RepeatingCallback<void(PackageOperationStatus)>;
@@ -81,6 +82,11 @@ class CrostiniPackageService : public KeyedService,
                                   UninstallPackageProgressStatus status,
                                   int progress_percent) override;
 
+  // PendingAppListUpdatesObserver:
+  void OnPendingAppListUpdates(const std::string& vm_name,
+                               const std::string& container_name,
+                               int count) override;
+
   // (Eventually) uninstall the package identified by |app_id|. If successfully
   // started, a system notification will be used to display further updates.
   void QueueUninstallApplication(const std::string& app_id);
@@ -173,6 +179,11 @@ class CrostiniPackageService : public KeyedService,
   std::vector<std::unique_ptr<CrostiniPackageNotification>>
       finished_notifications_;
 
+  // A map storing which containers have currently pending app list update
+  // operations. If a container is not present in the map, we assume no pending
+  // updates.
+  std::set<ContainerId> has_pending_app_list_updates_;
+
   // Called each time a notification is set to a new state.
   StateChangeCallback testing_state_change_callback_;
 

chromeos/dbus/cicerone_client.cc

@@ -479,6 +479,13 @@ class CiceroneClientImpl : public CiceroneClient {
             weak_ptr_factory_.GetWeakPtr()),
         base::BindOnce(&CiceroneClientImpl::OnSignalConnected,
                        weak_ptr_factory_.GetWeakPtr()));
+    cicerone_proxy_->ConnectToSignal(
+        vm_tools::cicerone::kVmCiceroneInterface,
+        vm_tools::cicerone::kPendingAppListUpdatesSignal,
+        base::BindRepeating(&CiceroneClientImpl::OnPendingAppListUpdatesSignal,
+                            weak_ptr_factory_.GetWeakPtr()),
+        base::BindOnce(&CiceroneClientImpl::OnSignalConnected,
+                       weak_ptr_factory_.GetWeakPtr()));
   }
 
  private:
@@ -631,6 +638,18 @@ class CiceroneClientImpl : public CiceroneClient {
     }
   }
 
+  void OnPendingAppListUpdatesSignal(dbus::Signal* signal) {
+    vm_tools::cicerone::PendingAppListUpdatesSignal proto;
+    dbus::MessageReader reader(signal);
+    if (!reader.PopArrayOfBytesAsProto(&proto)) {
+      LOG(ERROR) << "Failed to parse proto from DBus Signal";
+      return;
+    }
+    for (auto& observer : observer_list_) {
+      observer.OnPendingAppListUpdates(proto);
+    }
+  }
+
   void OnSignalConnected(const std::string& interface_name,
                          const std::string& signal_name,
                          bool is_connected) {

chromeos/dbus/cicerone_client.h

@@ -78,6 +78,11 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) CiceroneClient : public DBusClient {
     virtual void OnImportLxdContainerProgress(
         const vm_tools::cicerone::ImportLxdContainerProgressSignal& signal) = 0;
 
+    // OnPendingAppListUpdates is signalled from Cicerone when the number of
+    // pending app list updates changes.
+    virtual void OnPendingAppListUpdates(
+        const vm_tools::cicerone::PendingAppListUpdatesSignal& signal) = 0;
+
    protected:
     virtual ~Observer() = default;
   };

chromeos/dbus/fake_cicerone_client.cc

@@ -320,4 +320,11 @@ void FakeCiceroneClient::UninstallPackageProgress(
   }
 }
 
+void FakeCiceroneClient::NotifyPendingAppListUpdates(
+    const vm_tools::cicerone::PendingAppListUpdatesSignal& proto) {
+  for (auto& observer : observer_list_) {
+    observer.OnPendingAppListUpdates(proto);
+  }
+}
+
 }  // namespace chromeos

chromeos/dbus/fake_cicerone_client.h

@@ -340,6 +340,8 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeCiceroneClient
       const vm_tools::cicerone::InstallLinuxPackageProgressSignal& signal);
   void UninstallPackageProgress(
       const vm_tools::cicerone::UninstallPackageProgressSignal& signal);
+  void NotifyPendingAppListUpdates(
+      const vm_tools::cicerone::PendingAppListUpdatesSignal& signal);
 
  protected:
   void Init(dbus::Bus* bus) override {}