Refactor CrostiniAnsibleManagementService::Observer

Changes CrostiniAnsibleManagementService::Observer to reflect only start
and finish of container configuration. In all mocks states installation
and application are reflected as one - configuring so there is no need
to disctint them yet. Also renames functions the way it is clearly
reflected what happened so that classes that would implement
CrostiniAnsibleManagementService::Observer would have more
understandable methods (for example installer).

Test: browser_tests --gtest_filter="Crostini*"
Bug: 998097
Change-Id: I32ea22c96d8987c25dfc06557596ce43561bfa93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1869416Reviewed-by: Nic Hollingum <hollingum@google.com>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Commit-Queue: Olya Kalitova <okalitova@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708567}

chrome/browser/chromeos/crostini/ansible/ansible_management_service.cc

@@ -69,6 +69,9 @@ void AnsibleManagementService::InstallAnsibleInDefaultContainer(
     base::OnceCallback<void(bool success)> callback) {
   DCHECK(ansible_installation_finished_callback_.is_null());
   ansible_installation_finished_callback_ = std::move(callback);
+  for (auto& observer : observers_) {
+    observer.OnAnsibleSoftwareConfigurationStarted();
+  }
 
   // TODO(chrisgunadi): Show Ansible software config dialog.
 
@@ -86,7 +89,7 @@ void AnsibleManagementService::OnInstallAnsibleInDefaultContainer(
     LOG(ERROR) << "Ansible installation failed";
     std::move(ansible_installation_finished_callback_).Run(/*success=*/false);
     for (auto& observer : observers_) {
-      observer.OnError();
+      observer.OnAnsibleSoftwareConfigurationFinished(false);
     }
     return;
   }
@@ -112,7 +115,7 @@ void AnsibleManagementService::OnInstallLinuxPackageProgress(
     case InstallLinuxPackageProgressStatus::FAILED:
       std::move(ansible_installation_finished_callback_).Run(/*success=*/false);
       for (auto& observer : observers_) {
-        observer.OnError();
+        observer.OnAnsibleSoftwareConfigurationFinished(false);
       }
       return;
     // TODO(okalitova): Report Ansible downloading/installation progress.
@@ -144,7 +147,7 @@ void AnsibleManagementService::ApplyAnsiblePlaybookToDefaultContainer(
         << "Attempted to apply playbook when progress signal not connected.";
     std::move(callback).Run(/*success=*/false);
     for (auto& observer : observers_) {
-      observer.OnError();
+      observer.OnAnsibleSoftwareConfigurationFinished(false);
     }
     return;
   }
@@ -158,10 +161,6 @@ void AnsibleManagementService::ApplyAnsiblePlaybookToDefaultContainer(
   request.set_container_name(std::move(kCrostiniDefaultContainerName));
   request.set_playbook(std::move(playbook));
 
-  for (auto& observer : observers_) {
-    observer.OnApplicationStarted();
-  }
-
   GetCiceroneClient()->ApplyAnsiblePlaybook(
       std::move(request),
       base::BindOnce(&AnsibleManagementService::OnApplyAnsiblePlaybook,
@@ -175,7 +174,7 @@ void AnsibleManagementService::OnApplyAnsiblePlaybook(
     std::move(ansible_playbook_application_finished_callback_)
         .Run(/*success=*/false);
     for (auto& observer : observers_) {
-      observer.OnError();
+      observer.OnAnsibleSoftwareConfigurationFinished(false);
     }
     return;
   }
@@ -187,7 +186,7 @@ void AnsibleManagementService::OnApplyAnsiblePlaybook(
     std::move(ansible_playbook_application_finished_callback_)
         .Run(/*success=*/false);
     for (auto& observer : observers_) {
-      observer.OnError();
+      observer.OnAnsibleSoftwareConfigurationFinished(false);
     }
     return;
   }
@@ -203,14 +202,14 @@ void AnsibleManagementService::OnApplyAnsiblePlaybookProgress(
       std::move(ansible_playbook_application_finished_callback_)
           .Run(/*success=*/true);
       for (auto& observer : observers_) {
-        observer.OnApplicationFinished();
+        observer.OnAnsibleSoftwareConfigurationFinished(true);
       }
       break;
     case vm_tools::cicerone::ApplyAnsiblePlaybookProgressSignal::FAILED:
       std::move(ansible_playbook_application_finished_callback_)
           .Run(/*success=*/false);
       for (auto& observer : observers_) {
-        observer.OnError();
+        observer.OnAnsibleSoftwareConfigurationFinished(false);
       }
       break;
     case vm_tools::cicerone::ApplyAnsiblePlaybookProgressSignal::IN_PROGRESS:

chrome/browser/chromeos/crostini/ansible/ansible_management_service.h

@@ -28,9 +28,8 @@ class AnsibleManagementService : public KeyedService,
   class Observer : public base::CheckedObserver {
    public:
     ~Observer() override = default;
-    virtual void OnApplicationStarted() = 0;
-    virtual void OnApplicationFinished() = 0;
-    virtual void OnError() = 0;
+    virtual void OnAnsibleSoftwareConfigurationStarted() = 0;
+    virtual void OnAnsibleSoftwareConfigurationFinished(bool success) = 0;
   };
 
   static AnsibleManagementService* GetForProfile(Profile* profile);

chrome/browser/ui/views/crostini/crostini_ansible_software_config_view.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/views/crostini/crostini_ansible_software_config_view.h"
 
+#include "base/logging.h"
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/views/chrome_layout_provider.h"
@@ -53,10 +54,6 @@ base::string16 CrostiniAnsibleSoftwareConfigView::GetWindowTitle() const {
   return l10n_util::GetStringUTF16(IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_LABEL);
 }
 
-bool CrostiniAnsibleSoftwareConfigView::ShouldShowCloseButton() const {
-  return false;
-}
-
 gfx::Size CrostiniAnsibleSoftwareConfigView::CalculatePreferredSize() const {
   const int dialog_width = ChromeLayoutProvider::Get()->GetDistanceMetric(
                                DISTANCE_STANDALONE_BUBBLE_PREFERRED_WIDTH) -
@@ -64,37 +61,34 @@ gfx::Size CrostiniAnsibleSoftwareConfigView::CalculatePreferredSize() const {
   return gfx::Size(dialog_width, GetHeightForWidth(dialog_width));
 }
 
-void CrostiniAnsibleSoftwareConfigView::OnApplicationStarted() {
-  DCHECK_EQ(state_, State::INSTALLING);
-
-  state_ = State::APPLYING;
+void CrostiniAnsibleSoftwareConfigView::
+    OnAnsibleSoftwareConfigurationStarted() {
+  NOTIMPLEMENTED();
 }
 
-void CrostiniAnsibleSoftwareConfigView::OnApplicationFinished() {
-  DCHECK_EQ(state_, State::APPLYING);
-
+void CrostiniAnsibleSoftwareConfigView::OnAnsibleSoftwareConfigurationFinished(
+    bool success) {
+  DCHECK_EQ(state_, State::CONFIGURING);
   ansible_management_service_->RemoveObserver(this);
-  // TODO(crbug.com/1005774): We should preferably add another ClosedReason
-  // instead of passing kUnspecified.
-  GetWidget()->CloseWithReason(views::Widget::ClosedReason::kUnspecified);
-}
-
-void CrostiniAnsibleSoftwareConfigView::OnError() {
-  DCHECK(state_ == State::APPLYING || state_ == State::INSTALLING);
-  state_ = State::ERROR;
 
-  ansible_management_service_->RemoveObserver(this);
+  if (!success) {
+    state_ = State::ERROR;
 
-  // Bring dialog to front.
-  GetWidget()->Show();
+    // Bring dialog to front.
+    GetWidget()->Show();
 
-  GetWidget()->UpdateWindowTitle();
-  subtext_label_->SetText(l10n_util::GetStringUTF16(
-      IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_ERROR_SUBTEXT));
+    GetWidget()->UpdateWindowTitle();
+    subtext_label_->SetText(l10n_util::GetStringUTF16(
+        IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_ERROR_SUBTEXT));
 
-  progress_bar_->SetVisible(false);
+    progress_bar_->SetVisible(false);
 
-  DialogModelChanged();
+    DialogModelChanged();
+    return;
+  }
+  // TODO(crbug.com/1005774): We should preferably add another ClosedReason
+  // instead of passing kUnspecified.
+  GetWidget()->CloseWithReason(views::Widget::ClosedReason::kUnspecified);
 }
 
 base::string16

chrome/browser/ui/views/crostini/crostini_ansible_software_config_view.h

@@ -22,13 +22,11 @@ class CrostiniAnsibleSoftwareConfigView
   // views::DialogDelegateView:
   int GetDialogButtons() const override;
   base::string16 GetWindowTitle() const override;
-  bool ShouldShowCloseButton() const override;
   gfx::Size CalculatePreferredSize() const override;
 
   // crostini::AnsibleManagementService::Observer:
-  void OnApplicationStarted() override;
-  void OnApplicationFinished() override;
-  void OnError() override;
+  void OnAnsibleSoftwareConfigurationStarted() override;
+  void OnAnsibleSoftwareConfigurationFinished(bool success) override;
 
   base::string16 GetSubtextLabelStringForTesting();
 
@@ -38,12 +36,11 @@ class CrostiniAnsibleSoftwareConfigView
 
  private:
   enum class State {
-    INSTALLING,
-    APPLYING,
+    CONFIGURING,
     ERROR,
   };
 
-  State state_ = State::INSTALLING;
+  State state_ = State::CONFIGURING;
   crostini::AnsibleManagementService* ansible_management_service_ = nullptr;
 
   views::Label* subtext_label_ = nullptr;

chrome/browser/ui/views/crostini/crostini_ansible_software_config_view_browsertest.cc

@@ -77,8 +77,7 @@ IN_PROC_BROWSER_TEST_F(CrostiniAnsibleSoftwareConfigViewBrowserTest,
   EXPECT_TRUE(HasView());
   EXPECT_TRUE(IsDefaultDialog());
 
-  ActiveView()->OnApplicationStarted();
-  ActiveView()->OnApplicationFinished();
+  ActiveView()->OnAnsibleSoftwareConfigurationFinished(true);
 
   EXPECT_TRUE(HasNoView());
 }
@@ -91,7 +90,7 @@ IN_PROC_BROWSER_TEST_F(CrostiniAnsibleSoftwareConfigViewBrowserTest,
   EXPECT_TRUE(HasView());
   EXPECT_TRUE(IsDefaultDialog());
 
-  ActiveView()->OnError();
+  ActiveView()->OnAnsibleSoftwareConfigurationFinished(false);
 
   EXPECT_NE(nullptr, ActiveView());
   EXPECT_TRUE(IsErrorDialog());