Set Crostini Ansible software dialog to observe Ansible management service

Wire up dialog to Ansible management service. States are added to modify
strings and buttons based on events that happened.

Tests for successful and unsuccessful dialog flow.

TEST: browser_tests --gtest_filter="*CrostiniAnsibleSoftware*"
BUG: 998097
Change-Id: I19d0693a068efeb5b9b6af7a0b04c4ee0ad38ec4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1789229
Commit-Queue: Christopher Gunadi <chrisgunadi@google.com>
Reviewed-by: Olya Kalitova <okalitova@chromium.org>
Reviewed-by: Alex Oldemeier <aoldemeier@chromium.org>
Reviewed-by: Nic Hollingum <hollingum@google.com>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#698444}

chrome/app/chromeos_strings.grdp

@@ -3475,6 +3475,12 @@
   <message name="IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_SUBTEXT" translateable="false" desc="Description for dialog warning user of unavailable Linux container until software configurations are applied.">
     Your app will open when configuration is finished. Configuration can take a few minutes.
   </message>
+  <message name="IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_ERROR_LABEL" translateable="false" desc="Label for Crostini software config dialog when there is an error in Ansible playbook application or installation.">
+    Error configuring Linux
+  </message>
+  <message name="IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_ERROR_SUBTEXT" translateable="false" desc="Text shown for Crostini software config dialog when there is an error in Ansible playbook application or installation.">
+    There was an error configuring Linux. Please contact your administrator.
+  </message>
   <message name="IDS_CROSTINI_SHUT_DOWN_LINUX_MENU_ITEM" desc="Text shown in the context menu for the Linux terminal app, allowing users to shut down the Linux virtual machine.">
     Shut down Linux (Beta)
   </message>

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

@@ -70,6 +70,8 @@ void AnsibleManagementService::InstallAnsibleInDefaultContainer(
   DCHECK(ansible_installation_finished_callback_.is_null());
   ansible_installation_finished_callback_ = std::move(callback);
 
+  // TODO(chrisgunadi): Show Ansible software config dialog.
+
   CrostiniManager::GetForProfile(profile_)->InstallLinuxPackageFromApt(
       kCrostiniDefaultVmName, kCrostiniDefaultContainerName,
       kCrostiniDefaultAnsibleVersion,
@@ -83,6 +85,9 @@ void AnsibleManagementService::OnInstallAnsibleInDefaultContainer(
   if (result == CrostiniResult::INSTALL_LINUX_PACKAGE_FAILED) {
     LOG(ERROR) << "Ansible installation failed";
     std::move(ansible_installation_finished_callback_).Run(/*success=*/false);
+    for (auto& observer : observers_) {
+      observer.OnError();
+    }
     return;
   }
 
@@ -107,6 +112,9 @@ void AnsibleManagementService::OnInstallLinuxPackageProgress(
       return;
     case InstallLinuxPackageProgressStatus::FAILED:
       std::move(ansible_installation_finished_callback_).Run(/*success=*/false);
+      for (auto& observer : observers_) {
+        observer.OnError();
+      }
       return;
     // TODO(okalitova): Report Ansible downloading/installation progress.
     case InstallLinuxPackageProgressStatus::DOWNLOADING:
@@ -137,6 +145,9 @@ void AnsibleManagementService::ApplyAnsiblePlaybookToDefaultContainer(
     LOG(ERROR)
         << "Attempted to apply playbook when progress signal not connected.";
     std::move(callback).Run(/*success=*/false);
+    for (auto& observer : observers_) {
+      observer.OnError();
+    }
     return;
   }
 
@@ -149,6 +160,10 @@ 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,
@@ -161,6 +176,9 @@ void AnsibleManagementService::OnApplyAnsiblePlaybook(
     LOG(ERROR) << "Failed to apply Ansible playbook. Empty response.";
     std::move(ansible_playbook_application_finished_callback_)
         .Run(/*success=*/false);
+    for (auto& observer : observers_) {
+      observer.OnError();
+    }
     return;
   }
 
@@ -170,6 +188,9 @@ void AnsibleManagementService::OnApplyAnsiblePlaybook(
                << response->failure_reason();
     std::move(ansible_playbook_application_finished_callback_)
         .Run(/*success=*/false);
+    for (auto& observer : observers_) {
+      observer.OnError();
+    }
     return;
   }
 
@@ -183,10 +204,16 @@ void AnsibleManagementService::OnApplyAnsiblePlaybookProgress(
     case vm_tools::cicerone::ApplyAnsiblePlaybookProgressSignal::SUCCEEDED:
       std::move(ansible_playbook_application_finished_callback_)
           .Run(/*success=*/true);
+      for (auto& observer : observers_) {
+        observer.OnApplicationFinished();
+      }
       break;
     case vm_tools::cicerone::ApplyAnsiblePlaybookProgressSignal::FAILED:
       std::move(ansible_playbook_application_finished_callback_)
           .Run(/*success=*/false);
+      for (auto& observer : observers_) {
+        observer.OnError();
+      }
       break;
     case vm_tools::cicerone::ApplyAnsiblePlaybookProgressSignal::IN_PROGRESS:
       // TODO(okalitova): Report Ansible playbook application progress.
@@ -196,4 +223,13 @@ void AnsibleManagementService::OnApplyAnsiblePlaybookProgress(
   }
 }
 
+void AnsibleManagementService::AddObserver(Observer* observer) {
+  DCHECK(observer);
+  observers_.AddObserver(observer);
+}
+
+void AnsibleManagementService::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
 }  // namespace crostini

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

@@ -24,6 +24,15 @@ constexpr char kCrostiniDefaultAnsibleVersion[] =
 class AnsibleManagementService : public KeyedService,
                                  public LinuxPackageOperationProgressObserver {
  public:
+  // Observer class for Ansible Management Service related events.
+  class Observer : public base::CheckedObserver {
+   public:
+    ~Observer() override = default;
+    virtual void OnApplicationStarted() = 0;
+    virtual void OnApplicationFinished() = 0;
+    virtual void OnError() = 0;
+  };
+
   static AnsibleManagementService* GetForProfile(Profile* profile);
 
   explicit AnsibleManagementService(Profile* profile);
@@ -51,6 +60,9 @@ class AnsibleManagementService : public KeyedService,
   void OnApplyAnsiblePlaybookProgress(
       vm_tools::cicerone::ApplyAnsiblePlaybookProgressSignal::Status status);
 
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
  private:
   void OnInstallAnsibleInDefaultContainer(CrostiniResult result);
 
@@ -61,6 +73,7 @@ class AnsibleManagementService : public KeyedService,
           response);
 
   Profile* profile_;
+  base::ObserverList<Observer> observers_;
   base::OnceCallback<void(bool success)>
       ansible_installation_finished_callback_;
   base::OnceCallback<void(bool success)>

chrome/browser/chromeos/crostini/crostini_util.h

@@ -154,7 +154,7 @@ void CloseCrostiniUpgradeContainerView();
 
 // Show the Crostini Software Config Upgrade dialog (for installing Ansible and
 // applying an Ansible playbook in the container).
-void ShowCrostiniAnsibleSoftwareConfigView();
+void ShowCrostiniAnsibleSoftwareConfigView(Profile* profile);
 
 // We use an arbitrary well-formed extension id for the Terminal app, this
 // is equal to GenerateId("Terminal").

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

@@ -23,26 +23,38 @@ CrostiniAnsibleSoftwareConfigView*
 
 }  // namespace
 
-void crostini::ShowCrostiniAnsibleSoftwareConfigView() {
+namespace crostini {
+
+void ShowCrostiniAnsibleSoftwareConfigView(Profile* profile) {
   if (!g_crostini_ansible_software_configuration_view) {
     g_crostini_ansible_software_configuration_view =
-        new CrostiniAnsibleSoftwareConfigView();
+        new CrostiniAnsibleSoftwareConfigView(profile);
     views::DialogDelegate::CreateDialogWidget(
         g_crostini_ansible_software_configuration_view, nullptr, nullptr);
   }
+
   g_crostini_ansible_software_configuration_view->GetWidget()->Show();
 }
 
+}  // namespace crostini
+
 int CrostiniAnsibleSoftwareConfigView::GetDialogButtons() const {
+  if (state_ == State::ERROR) {
+    return ui::DIALOG_BUTTON_OK;
+  }
   return ui::DIALOG_BUTTON_NONE;
 }
 
 base::string16 CrostiniAnsibleSoftwareConfigView::GetWindowTitle() const {
+  if (state_ == State::ERROR) {
+    return l10n_util::GetStringUTF16(
+        IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_ERROR_LABEL);
+  }
   return l10n_util::GetStringUTF16(IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_LABEL);
 }
 
 bool CrostiniAnsibleSoftwareConfigView::ShouldShowCloseButton() const {
-  return true;
+  return false;
 }
 
 gfx::Size CrostiniAnsibleSoftwareConfigView::CalculatePreferredSize() const {
@@ -52,7 +64,56 @@ gfx::Size CrostiniAnsibleSoftwareConfigView::CalculatePreferredSize() const {
   return gfx::Size(dialog_width, GetHeightForWidth(dialog_width));
 }
 
-CrostiniAnsibleSoftwareConfigView::CrostiniAnsibleSoftwareConfigView() {
+void CrostiniAnsibleSoftwareConfigView::OnApplicationStarted() {
+  DCHECK_EQ(state_, State::INSTALLING);
+
+  state_ = State::APPLYING;
+}
+
+void CrostiniAnsibleSoftwareConfigView::OnApplicationFinished() {
+  DCHECK_EQ(state_, State::APPLYING);
+
+  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);
+
+  // Bring dialog to front.
+  GetWidget()->Show();
+
+  GetWidget()->UpdateWindowTitle();
+  subtext_label_->SetText(l10n_util::GetStringUTF16(
+      IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_ERROR_SUBTEXT));
+
+  progress_bar_->SetVisible(false);
+
+  DialogModelChanged();
+}
+
+base::string16
+CrostiniAnsibleSoftwareConfigView::GetSubtextLabelStringForTesting() {
+  return subtext_label_->GetText();
+}
+
+// static
+CrostiniAnsibleSoftwareConfigView*
+CrostiniAnsibleSoftwareConfigView::GetActiveViewForTesting() {
+  return g_crostini_ansible_software_configuration_view;
+}
+
+CrostiniAnsibleSoftwareConfigView::CrostiniAnsibleSoftwareConfigView(
+    Profile* profile)
+    : ansible_management_service_(
+          crostini::AnsibleManagementService::GetForProfile(profile)) {
+  ansible_management_service_->AddObserver(this);
+
   views::LayoutProvider* provider = views::LayoutProvider::Get();
   SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::Orientation::kVertical,
@@ -61,21 +122,20 @@ CrostiniAnsibleSoftwareConfigView::CrostiniAnsibleSoftwareConfigView() {
   set_margins(provider->GetDialogInsetsForContentType(
       views::DialogContentType::TEXT, views::DialogContentType::CONTROL));
 
-  const base::string16 message =
-      l10n_util::GetStringUTF16(IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_SUBTEXT);
-  auto message_label = std::make_unique<views::Label>(message);
-  message_label->SetMultiLine(true);
-  message_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
-  AddChildView(std::move(message_label));
+  subtext_label_ = new views::Label(
+      l10n_util::GetStringUTF16(IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_SUBTEXT));
+  subtext_label_->SetMultiLine(true);
+  subtext_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  AddChildView(subtext_label_);
 
   // Add infinite progress bar.
   // TODO(crbug.com/1000173): add progress reporting and display text above
   // progress bar indicating current process.
-  auto progress_bar = std::make_unique<views::ProgressBar>();
-  progress_bar->SetVisible(true);
+  progress_bar_ = new views::ProgressBar();
+  progress_bar_->SetVisible(true);
   // Values outside the range [0,1] display an infinite loading animation.
-  progress_bar->SetValue(-1);
-  AddChildView(std::move(progress_bar));
+  progress_bar_->SetValue(-1);
+  AddChildView(progress_bar_);
 
   chrome::RecordDialogCreation(
       chrome::DialogIdentifier::CROSTINI_ANSIBLE_SOFTWARE_CONFIG);

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

@@ -5,13 +5,19 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_CROSTINI_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_VIEW_H_
 #define CHROME_BROWSER_UI_VIEWS_CROSTINI_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_VIEW_H_
 
+#include "chrome/browser/chromeos/crostini/ansible/ansible_management_service.h"
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/controls/progress_bar.h"
+
+class Profile;
 
 // The Ansible software configuration is shown to let the user know that an
 // Ansible playbook is being applied and their app might take longer than
 // usual to launch.
 class CrostiniAnsibleSoftwareConfigView
-    : public views::BubbleDialogDelegateView {
+    : public views::BubbleDialogDelegateView,
+      public crostini::AnsibleManagementService::Observer {
  public:
   // views::DialogDelegateView:
   int GetDialogButtons() const override;
@@ -19,10 +25,33 @@ class CrostiniAnsibleSoftwareConfigView
   bool ShouldShowCloseButton() const override;
   gfx::Size CalculatePreferredSize() const override;
 
-  CrostiniAnsibleSoftwareConfigView();
+  // crostini::AnsibleManagementService::Observer:
+  void OnApplicationStarted() override;
+  void OnApplicationFinished() override;
+  void OnError() override;
+
+  base::string16 GetSubtextLabelStringForTesting();
+
+  static CrostiniAnsibleSoftwareConfigView* GetActiveViewForTesting();
+
+  explicit CrostiniAnsibleSoftwareConfigView(Profile* profile);
 
  private:
+  enum class State {
+    INSTALLING,
+    APPLYING,
+    ERROR,
+  };
+
+  State state_ = State::INSTALLING;
+  crostini::AnsibleManagementService* ansible_management_service_ = nullptr;
+
+  views::Label* subtext_label_ = nullptr;
+  views::ProgressBar* progress_bar_ = nullptr;
+
   ~CrostiniAnsibleSoftwareConfigView() override;
+
+  DISALLOW_COPY_AND_ASSIGN(CrostiniAnsibleSoftwareConfigView);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_CROSTINI_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_VIEW_H_

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

@@ -4,24 +4,95 @@
 
 #include "chrome/browser/ui/views/crostini/crostini_ansible_software_config_view.h"
 
+#include "base/bind_helpers.h"
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/views/crostini/crostini_browser_test_util.h"
+#include "chrome/grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/views/window/dialog_client_view.h"
 
-class CrostiniAnsibleSoftwareConfigBrowserTest
+class CrostiniAnsibleSoftwareConfigViewBrowserTest
     : public CrostiniDialogBrowserTest {
  public:
-  CrostiniAnsibleSoftwareConfigBrowserTest()
+  CrostiniAnsibleSoftwareConfigViewBrowserTest()
       : CrostiniDialogBrowserTest(true /*register_termina*/) {}
 
   // CrostiniDialogBrowserTest:
   void ShowUi(const std::string& name) override {
-    crostini::ShowCrostiniAnsibleSoftwareConfigView();
+    crostini::ShowCrostiniAnsibleSoftwareConfigView(browser()->profile());
+  }
+
+  CrostiniAnsibleSoftwareConfigView* ActiveView() {
+    return CrostiniAnsibleSoftwareConfigView::GetActiveViewForTesting();
+  }
+
+ protected:
+  // A new Widget was created in ShowUi() or since the last VerifyUi().
+  bool HasView() { return VerifyUi() && ActiveView() != nullptr; }
+
+  // No new Widget was created in ShowUi() or since last VerifyUi().
+  bool HasNoView() {
+    base::RunLoop().RunUntilIdle();
+    return !VerifyUi() && ActiveView() == nullptr;
+  }
+
+  bool IsDefaultDialog() { return !HasAcceptButton() && HasDefaultStrings(); }
+
+  bool IsErrorDialog() { return HasAcceptButton() && HasErrorStrings(); }
+
+ private:
+  bool HasAcceptButton() {
+    return ActiveView()->GetDialogClientView()->ok_button() != nullptr;
+  }
+
+  bool HasDefaultStrings() {
+    return (ActiveView()->GetWindowTitle().compare(l10n_util::GetStringUTF16(
+                IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_LABEL)) == 0) &&
+           (ActiveView()->GetSubtextLabelStringForTesting().compare(
+                l10n_util::GetStringUTF16(
+                    IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_SUBTEXT)) == 0);
+  }
+
+  bool HasErrorStrings() {
+    return (ActiveView()->GetWindowTitle().compare(l10n_util::GetStringUTF16(
+                IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_ERROR_LABEL)) == 0) &&
+           (ActiveView()->GetSubtextLabelStringForTesting().compare(
+                l10n_util::GetStringUTF16(
+                    IDS_CROSTINI_ANSIBLE_SOFTWARE_CONFIG_ERROR_SUBTEXT)) == 0);
   }
 };
 
-// Test the dialog is actually launched.
-IN_PROC_BROWSER_TEST_F(CrostiniAnsibleSoftwareConfigBrowserTest,
+// Test if dialog is actually launched.
+IN_PROC_BROWSER_TEST_F(CrostiniAnsibleSoftwareConfigViewBrowserTest,
                        InvokeUi_default) {
   ShowAndVerifyUi();
 }
+
+// Test if dialog behaviour is correct during a successful flow.
+IN_PROC_BROWSER_TEST_F(CrostiniAnsibleSoftwareConfigViewBrowserTest,
+                       SuccessfulFlow) {
+  ShowUi("default");
+
+  EXPECT_TRUE(HasView());
+  EXPECT_TRUE(IsDefaultDialog());
+
+  ActiveView()->OnApplicationStarted();
+  ActiveView()->OnApplicationFinished();
+
+  EXPECT_TRUE(HasNoView());
+}
+
+// Test if dialog behaviour is correct during an unsuccessful flow.
+IN_PROC_BROWSER_TEST_F(CrostiniAnsibleSoftwareConfigViewBrowserTest,
+                       UnsuccessfulFlow) {
+  ShowUi("default");
+
+  EXPECT_TRUE(HasView());
+  EXPECT_TRUE(IsDefaultDialog());
+
+  ActiveView()->OnError();
+
+  EXPECT_NE(nullptr, ActiveView());
+  EXPECT_TRUE(IsErrorDialog());
+}