Make dlcservice_client use per-DLC signal

We want to move away from OnInstallStatus signal and instead move to a
per-DLC signal DlcStateChanged. This new signal is designed to make it
much easier for clients to monitor the state of a DLC.

BUG=chromium:1071260
TEST=./testing/xvfb.py ./out/Default/unit_tests --gtest_filter=Dlc*

Change-Id: Ibc042670afb5ec1fa9183bb20833d12c911ec94f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2213892
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Auto-Submit: Amin Hassani <ahassani@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772252}

chromeos/dbus/dlcservice/dlcservice_client.cc

@@ -122,7 +122,7 @@ class DlcserviceClientImpl : public DlcserviceClient {
 
     progress_callback_holder_ = std::move(progress_callback);
     install_callback_holder_ = std::move(install_callback);
-    install_field_holder_ = dlc_id;
+    dlc_id_holder_ = dlc_id;
 
     VLOG(1) << "Requesting to install DLC(s).";
     dlcservice_proxy_->CallMethodWithErrorResponse(
@@ -171,8 +171,8 @@ class DlcserviceClientImpl : public DlcserviceClient {
                        weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
   }
 
-  void OnInstallStatusForTest(dbus::Signal* signal) override {
-    OnInstallStatus(signal);
+  void DlcStateChangedForTest(dbus::Signal* signal) override {
+    DlcStateChanged(signal);
   }
 
   void Init(dbus::Bus* bus) {
@@ -180,10 +180,10 @@ class DlcserviceClientImpl : public DlcserviceClient {
         dlcservice::kDlcServiceServiceName,
         dbus::ObjectPath(dlcservice::kDlcServiceServicePath));
     dlcservice_proxy_->ConnectToSignal(
-        dlcservice::kDlcServiceInterface, dlcservice::kOnInstallStatusSignal,
-        base::BindRepeating(&DlcserviceClientImpl::OnInstallStatus,
+        dlcservice::kDlcServiceInterface, dlcservice::kDlcStateChangedSignal,
+        base::BindRepeating(&DlcserviceClientImpl::DlcStateChanged,
                             weak_ptr_factory_.GetWeakPtr()),
-        base::BindOnce(&DlcserviceClientImpl::OnInstallStatusConnected,
+        base::BindOnce(&DlcserviceClientImpl::DlcStateChangedConnected,
                        weak_ptr_factory_.GetWeakPtr()));
   }
 
@@ -198,7 +198,7 @@ class DlcserviceClientImpl : public DlcserviceClient {
     // |Install()|
     install_callback_holder_.reset();
     progress_callback_holder_.reset();
-    install_field_holder_.reset();
+    dlc_id_holder_.reset();
   }
 
   void EnqueueTask(base::OnceClosure task) {
@@ -213,57 +213,59 @@ class DlcserviceClientImpl : public DlcserviceClient {
     }
   }
 
-  void SendProgress(const dlcservice::InstallStatus& install_status) {
-    const double progress = install_status.progress();
+  void SendProgress(double progress) {
     VLOG(2) << "Install in progress: " << progress;
     if (progress_callback_holder_.has_value())
       progress_callback_holder_.value().Run(progress);
   }
 
-  void SendCompleted(const dlcservice::InstallStatus& install_status) {
-    // TODO(crbug.com/1078556): We should not be getting the DLC ID from the
-    // module list returned by the signal.
-    std::string dlc_id, root_path;
-    if (install_status.dlc_module_list().dlc_module_infos_size() > 0) {
-      auto dlc_info = install_status.dlc_module_list().dlc_module_infos(0);
-      dlc_id = dlc_info.dlc_id();
-      root_path = dlc_info.dlc_root();
+  void SendCompleted(const dlcservice::DlcState& dlc_state) {
+    if (!install_callback_holder_.has_value() || !dlc_id_holder_.has_value())
+      return;
+
+    if (dlc_id_holder_ != dlc_state.id()) {
+      // We are not interested in this DLC. Some other process might be
+      // installing it.
+      return;
     }
 
-    InstallResult install_result = {
-        .error = install_status.error_code(),
-        .dlc_id = dlc_id,
-        .root_path = root_path,
-    };
-    std::move(install_callback_holder_.value()).Run(install_result);
+    if (dlc_state.state() == dlcservice::DlcState::NOT_INSTALLED) {
+      LOG(ERROR) << "Failed to install DLC " << dlc_state.id()
+                 << " with error code: " << dlc_state.last_error_code();
+
+    } else {
+      VLOG(1) << "DLC " << dlc_state.id() << " installed successfully.";
+      if (dlc_state.last_error_code() != dlcservice::kErrorNone) {
+        LOG(WARNING) << "DLC installation was sucessful but non-success "
+                     << "error code: " << dlc_state.last_error_code();
+      }
     }
 
-  void OnInstallStatus(dbus::Signal* signal) {
-    if (!install_callback_holder_.has_value())
-      return;
+    InstallResult result = {
+        .error = dlc_state.last_error_code(),
+        .dlc_id = dlc_state.id(),
+        .root_path = dlc_state.root_path(),
+    };
+    std::move(install_callback_holder_.value()).Run(result);
+  }
 
-    dlcservice::InstallStatus install_status;
-    if (!dbus::MessageReader(signal).PopArrayOfBytesAsProto(&install_status)) {
+  void DlcStateChanged(dbus::Signal* signal) {
+    dlcservice::DlcState dlc_state;
+    if (!dbus::MessageReader(signal).PopArrayOfBytesAsProto(&dlc_state)) {
       LOG(ERROR) << "Failed to parse proto as install status.";
       return;
     }
 
-    switch (install_status.status()) {
-      case dlcservice::Status::COMPLETED:
-        VLOG(1) << "DLC(s) install successful.";
-        SendCompleted(install_status);
+    switch (dlc_state.state()) {
+      case dlcservice::DlcState::NOT_INSTALLED:
+      case dlcservice::DlcState::INSTALLED:
+        SendCompleted(dlc_state);
         break;
-      case dlcservice::Status::RUNNING: {
-        SendProgress(install_status);
+      case dlcservice::DlcState::INSTALLING:
+        SendProgress(dlc_state.progress());
         // Need to return here since we don't want to try starting another
         // pending install from the queue (would waste time checking).
         return;
-      }
-      case dlcservice::Status::FAILED:
-        LOG(ERROR) << "Failed to install with error code: "
-                   << install_status.error_code();
-        SendCompleted(install_status);
-        break;
       default:
         NOTREACHED();
     }
@@ -274,10 +276,10 @@ class DlcserviceClientImpl : public DlcserviceClient {
     CheckAndRunPendingTask();
   }
 
-  void OnInstallStatusConnected(const std::string& interface,
+  void DlcStateChangedConnected(const std::string& interface,
                                 const std::string& signal,
                                 bool success) {
-    LOG_IF(ERROR, !success) << "Failed to connect to install status signal.";
+    LOG_IF(ERROR, !success) << "Failed to connect to DlcStateChanged signal.";
   }
 
   void OnInstall(dbus::Response* response, dbus::ErrorResponse* err_response) {
@@ -286,7 +288,7 @@ class DlcserviceClientImpl : public DlcserviceClient {
 
     // Perform DCHECKs only when an error occurs, platform dlcservice currently
     // sends a signal prior to DBus method callback on quick install scenarios.
-    DCHECK(install_field_holder_.has_value());
+    DCHECK(dlc_id_holder_.has_value());
     DCHECK(install_callback_holder_.has_value());
     DCHECK(progress_callback_holder_.has_value());
 
@@ -294,13 +296,14 @@ class DlcserviceClientImpl : public DlcserviceClient {
     if (err == dlcservice::kErrorBusy) {
       EnqueueTask(base::BindOnce(&DlcserviceClientImpl::Install,
                                  weak_ptr_factory_.GetWeakPtr(),
-                                 std::move(install_field_holder_.value()),
+                                 std::move(dlc_id_holder_.value()),
                                  std::move(install_callback_holder_.value()),
                                  std::move(progress_callback_holder_.value())));
     } else {
-      dlcservice::InstallStatus install_status;
-      install_status.set_error_code(err);
-      SendCompleted(install_status);
+      dlcservice::DlcState dlc_state;
+      dlc_state.set_id(dlc_id_holder_.value());
+      dlc_state.set_last_error_code(err);
+      SendCompleted(dlc_state);
     }
     CheckAndRunPendingTask();
   }
@@ -347,8 +350,8 @@ class DlcserviceClientImpl : public DlcserviceClient {
   // The cached callback to call on during progress of |Install()|.
   base::Optional<ProgressCallback> progress_callback_holder_;
 
-  // The cached field of string (DLC ID) for retrying call to install.
-  base::Optional<std::string> install_field_holder_;
+  // The cached DLC ID for retrying call to install.
+  base::Optional<std::string> dlc_id_holder_;
 
   // A list of postponed installs to dlcservice.
   std::deque<base::OnceClosure> pending_tasks_;

chromeos/dbus/dlcservice/dlcservice_client.h

@@ -99,7 +99,7 @@ class COMPONENT_EXPORT(DLCSERVICE_CLIENT) DlcserviceClient {
   virtual void GetExistingDlcs(GetExistingDlcsCallback callback) = 0;
 
   // During testing, can be used to mimic signals received back from dlcservice.
-  virtual void OnInstallStatusForTest(dbus::Signal* signal) = 0;
+  virtual void DlcStateChangedForTest(dbus::Signal* signal) = 0;
 
   // Creates and initializes the global instance. |bus| must not be nullptr.
   static void Initialize(dbus::Bus* bus);

chromeos/dbus/dlcservice/dlcservice_client_unittest.cc

@@ -303,13 +303,13 @@ TEST_F(DlcserviceClientTest, InstallProgressTest) {
   EXPECT_EQ(0u, counter.load());
 
   dbus::Signal signal("com.example.Interface", "SomeSignal");
-  dlcservice::InstallStatus install_status;
-  install_status.set_status(dlcservice::Status::RUNNING);
+  dlcservice::DlcState dlc_state;
+  dlc_state.set_state(dlcservice::DlcState::INSTALLING);
 
   dbus::MessageWriter writer(&signal);
-  writer.AppendProtoAsArrayOfBytes(install_status);
+  writer.AppendProtoAsArrayOfBytes(dlc_state);
 
-  client_->OnInstallStatusForTest(&signal);
+  client_->DlcStateChangedForTest(&signal);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1u, counter.load());
 }
@@ -358,14 +358,14 @@ TEST_F(DlcserviceClientTest, PendingTaskTest) {
   EXPECT_EQ(0u, counter.load());
 
   dbus::Signal signal("com.example.Interface", "SomeSignal");
-  dlcservice::InstallStatus install_status;
-  install_status.set_status(dlcservice::Status::COMPLETED);
+  dlcservice::DlcState dlc_state;
+  dlc_state.set_state(dlcservice::DlcState::INSTALLED);
 
   dbus::MessageWriter writer(&signal);
-  writer.AppendProtoAsArrayOfBytes(install_status);
+  writer.AppendProtoAsArrayOfBytes(dlc_state);
 
   for (size_t i = 1; i < 100; ++i) {
-    client_->OnInstallStatusForTest(&signal);
+    client_->DlcStateChangedForTest(&signal);
     base::RunLoop().RunUntilIdle();
     EXPECT_EQ(i <= kLoopCount ? i : kLoopCount, counter.load());
   }

chromeos/dbus/dlcservice/fake_dlcservice_client.cc

@@ -48,7 +48,7 @@ void FakeDlcserviceClient::GetExistingDlcs(GetExistingDlcsCallback callback) {
                                 dlcs_with_content_));
 }
 
-void FakeDlcserviceClient::OnInstallStatusForTest(dbus::Signal* signal) {
+void FakeDlcserviceClient::DlcStateChangedForTest(dbus::Signal* signal) {
   NOTREACHED();
 }
 

chromeos/dbus/dlcservice/fake_dlcservice_client.h

@@ -30,7 +30,7 @@ class COMPONENT_EXPORT(DLCSERVICE_CLIENT) FakeDlcserviceClient
   // Purging removes the DLC entirely from disk.
   void Purge(const std::string& dlc_id, PurgeCallback callback) override;
   void GetExistingDlcs(GetExistingDlcsCallback callback) override;
-  void OnInstallStatusForTest(dbus::Signal* signal) override;
+  void DlcStateChangedForTest(dbus::Signal* signal) override;
 
   // Setters:
   // TODO(hsuregan/kimjae): Convert setters and at tests that use them to