Update terminal startup status to be less verbose

Using mocks as provided by @benwells

Bug: 1016680
Change-Id: I4f94640a6aa60c99ae3c7fb417bc498b7db2dfa9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2040799
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Jason Lin <lxj@google.com>
Auto-Submit: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739291}

chrome/browser/extensions/api/terminal/crostini_startup_status.cc

@@ -12,10 +12,13 @@
 #include "base/strings/stringprintf.h"
 #include "base/system/sys_info.h"
 #include "base/task/post_task.h"
-#include "base/task/task_traits.h"
 #include "base/time/time.h"
 #include "chromeos/dbus/util/version_loader.h"
 #include "components/version_info/version_info.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/browser_thread.h"
+
+using crostini::mojom::InstallerState;
 
 namespace extensions {
 
@@ -24,19 +27,22 @@ namespace {
 const char kCursorHide[] = "\x1b[?25l";
 const char kCursorShow[] = "\x1b[?25h";
 const char kColor0Normal[] = "\x1b[0m";  // Default.
-const char kColor1Red[] = "\x1b[31m";
-const char kColor2Green[] = "\x1b[32m";
-const char kColor4Blue[] = "\x1b[34m";
+const char kColor1RedBright[] = "\x1b[1;31m";
+const char kColor2GreenBright[] = "\x1b[1;32m";
+const char kColor3Yellow[] = "\x1b[33m";
 const char kColor5Purple[] = "\x1b[35m";
-const char kProgressStart[] = "\x1b[7m";  // Invert color.
-const char kProgressEnd[] = "\x1b[27m";   // Revert color.
+const char kEraseInLine[] = "\x1b[K";
 const char kSpinner[] = "|/-\\";
-const int kTimestampLength = 25;
-const int kMaxProgress = 9;
-const base::NoDestructor<std::vector<std::string>> kSuccessEmoji(
-    {"😀", "😉", "🤩", "🤪", "😎", "🥳", "👍"});
-const base::NoDestructor<std::vector<std::string>> kErrorEmoji({"🤕", "😠",
-                                                                "😧", "😢", "😞"});
+const int kMaxStage = 9;
+const char kErrorSymbol[] = "☠️";
+const base::NoDestructor<std::vector<std::string>> kSuccessSymbol({"😺", "🐹",
+                                                                   "🐼", "🐸",
+                                                                   "🐶", "🐬"});
+
+std::string MoveForward(int i) {
+  return base::StringPrintf("\x1b[%dC", i);
+}
+
 }  // namespace
 
 CrostiniStartupStatus::CrostiniStartupStatus(
@@ -46,13 +52,9 @@ CrostiniStartupStatus::CrostiniStartupStatus(
     : print_(std::move(print)),
       verbose_(verbose),
       callback_(std::move(callback)) {
-  Print(kCursorHide);
-  if (verbose_) {
-    PrintWithTimestamp("Chrome OS " + version_info::GetVersionNumber() + " " +
-                       chromeos::version_loader::GetVersion(
-                           chromeos::version_loader::VERSION_FULL) +
-                       "\r\n");
-  }
+  // Initialize Progress
+  Print(base::StringPrintf("%s%s[%s] ", kCursorHide, kColor5Purple,
+                           std::string(kMaxStage, ' ').c_str()));
 }
 
 CrostiniStartupStatus::~CrostiniStartupStatus() = default;
@@ -62,160 +64,92 @@ void CrostiniStartupStatus::OnCrostiniRestarted(
   if (result != crostini::CrostiniResult::SUCCESS) {
     LOG(ERROR) << "Error starting crostini for terminal: "
                << static_cast<int>(result);
-    PrintWithTimestamp(base::StringPrintf(
-        "Error starting penguin container: %d %s\r\n", result,
-        (*kErrorEmoji)[rand() % kErrorEmoji->size()].c_str()));
+    PrintAfterStage(
+        kColor1RedBright,
+        base::StringPrintf("Error starting penguin container: %d %s\r\n",
+                           result, kErrorSymbol));
   } else {
     if (verbose_) {
-      PrintWithTimestamp(base::StringPrintf(
-          "Ready %s\r\n",
-          (*kSuccessEmoji)[rand() % kSuccessEmoji->size()].c_str()));
+      stage_index_ = kMaxStage + 1;  // done.
+      PrintStage(kColor2GreenBright, "done");
+      Print((*kSuccessSymbol)[rand() % kSuccessSymbol->size()] + "\r\n");
     }
-    Print(kCursorShow);
   }
+  Print(
+      base::StringPrintf("\r%s%s%s", kEraseInLine, kColor0Normal, kCursorShow));
   std::move(callback_).Run();
   delete this;
 }
 
-void CrostiniStartupStatus::ShowStatusLineAtInterval() {
+void CrostiniStartupStatus::ShowProgressAtInterval() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
   ++spinner_index_;
-  PrintStatusLine();
+  PrintProgress();
   base::PostDelayedTask(
-      FROM_HERE,
-      base::BindOnce(&CrostiniStartupStatus::ShowStatusLineAtInterval,
+      FROM_HERE, {content::BrowserThread::UI},
+      base::BindOnce(&CrostiniStartupStatus::ShowProgressAtInterval,
                      weak_factory_.GetWeakPtr()),
       base::TimeDelta::FromMilliseconds(300));
 }
 
 void CrostiniStartupStatus::OnStageStarted(InstallerState stage) {
   stage_ = stage;
-  progress_index_++;
+  if (stage_index_ < kMaxStage) {
+    ++stage_index_;
+  }
   if (!verbose_) {
     return;
   }
   static base::NoDestructor<base::flat_map<InstallerState, std::string>>
       kStartStrings({
-          {InstallerState::kStart, "Starting... 🤔"},
+          {InstallerState::kStart, "Initializing"},
           {InstallerState::kInstallImageLoader,
-           "Checking cros-termina component..."},
-          {InstallerState::kStartConcierge, "Starting VM controller..."},
-          {InstallerState::kCreateDiskImage, "Creating termina VM image..."},
-          {InstallerState::kStartTerminaVm, "Starting termina VM..."},
-          {InstallerState::kCreateContainer, "Creating penguin container..."},
-          {InstallerState::kSetupContainer,
-           "Checking penguin container setup..."},
-          {InstallerState::kStartContainer, "Starting penguin container..."},
+           "Checking cros-termina component"},
+          {InstallerState::kStartConcierge, "Starting VM controller"},
+          {InstallerState::kCreateDiskImage, "Creating termina VM image"},
+          {InstallerState::kStartTerminaVm, "Starting termina VM"},
+          {InstallerState::kCreateContainer, "Creating penguin container"},
+          {InstallerState::kSetupContainer, "Checking penguin container setup"},
+          {InstallerState::kStartContainer, "Starting penguin container"},
           {InstallerState::kFetchSshKeys,
-           "Fetching penguin container ssh keys..."},
-          {InstallerState::kMountContainer,
-           "Mounting penguin container sshfs..."},
+           "Fetching penguin container ssh keys"},
+          {InstallerState::kMountContainer, "Mounting penguin container sshfs"},
       });
-  const std::string& start_string = (*kStartStrings)[stage];
-  cursor_position_ = kTimestampLength + start_string.length();
-  PrintWithTimestamp(start_string + "\r\n");
-  PrintStatusLine();
-}
-
-void CrostiniStartupStatus::OnComponentLoaded(crostini::CrostiniResult result) {
-  PrintCrostiniResult(result);
-}
-
-void CrostiniStartupStatus::OnConciergeStarted(bool success) {
-  PrintSuccess(success);
-}
-
-void CrostiniStartupStatus::OnDiskImageCreated(
-    bool success,
-    vm_tools::concierge::DiskImageStatus status,
-    int64_t disk_size_available) {
-  PrintSuccess(success);
-}
-
-void CrostiniStartupStatus::OnVmStarted(bool success) {
-  PrintSuccess(success);
+  const std::string& stage_string = (*kStartStrings)[stage];
+  PrintStage(kColor3Yellow, stage_string);
 }
 
 void CrostiniStartupStatus::OnContainerDownloading(int32_t download_percent) {
   if (download_percent % 8 == 0) {
-    PrintResult(".");
+    PrintAfterStage(kColor3Yellow, ".");
   }
 }
 
-void CrostiniStartupStatus::OnContainerCreated(
-    crostini::CrostiniResult result) {
-  PrintCrostiniResult(result);
-}
-
-void CrostiniStartupStatus::OnContainerSetup(bool success) {
-  PrintSuccess(success);
-}
-
-void CrostiniStartupStatus::OnContainerStarted(
-    crostini::CrostiniResult result) {
-  PrintCrostiniResult(result);
-}
-
-void CrostiniStartupStatus::OnSshKeysFetched(bool success) {
-  PrintSuccess(success);
-}
-
-void CrostiniStartupStatus::OnContainerMounted(bool success) {
-  PrintSuccess(success);
-}
-
-void CrostiniStartupStatus::PrintStatusLine() {
-  std::string progress(progress_index_, ' ');
-  std::string dots(std::max(kMaxProgress - progress_index_, 0), '.');
-  Print(base::StringPrintf("[%s%s%s%s%s%s] %s%c%s\r", kProgressStart,
-                           progress.c_str(), kProgressEnd, kColor5Purple,
-                           dots.c_str(), kColor0Normal, kColor4Blue,
-                           kSpinner[spinner_index_ & 0x3], kColor0Normal));
-}
-
 void CrostiniStartupStatus::Print(const std::string& output) {
   print_.Run(output);
 }
 
-void CrostiniStartupStatus::PrintWithTimestamp(const std::string& output) {
-  base::Time::Exploded exploded;
-  base::Time::Now().LocalExplode(&exploded);
-  Print(base::StringPrintf("%04d-%02d-%02d %02d:%02d:%02d.%03d %s",
-                           exploded.year, exploded.month, exploded.day_of_month,
-                           exploded.hour, exploded.minute, exploded.second,
-                           exploded.millisecond, output.c_str()));
+void CrostiniStartupStatus::PrintProgress() {
+  Print(base::StringPrintf("\r%s%s%c", MoveForward(stage_index_).c_str(),
+                           kColor5Purple, kSpinner[spinner_index_ & 0x3]));
 }
 
-void CrostiniStartupStatus::PrintResult(const std::string& output) {
-  if (!verbose_) {
-    return;
-  }
-
-  std::string cursor_move = "\x1b[A";  // cursor up.
-  for (int i = 0; i < cursor_position_; ++i) {
-    cursor_move += "\x1b[C";  // cursor forward.
-  }
-  Print(cursor_move + output + "\r\n");
-  cursor_position_ += output.length();
-  PrintStatusLine();
-}
-
-void CrostiniStartupStatus::PrintCrostiniResult(
-    crostini::CrostiniResult result) {
-  if (result == crostini::CrostiniResult::SUCCESS) {
-    PrintSuccess(true);
-  } else {
-    PrintResult(base::StringPrintf("%serror=%d%s ❌", kColor1Red, result,
-                                   kColor0Normal));
-  }
+void CrostiniStartupStatus::PrintStage(const char* color,
+                                       const std::string& output) {
+  DCHECK_GE(stage_index_, 1);
+  std::string progress(stage_index_ - 1, '=');
+  Print(base::StringPrintf("\r%s[%s%s%s%s%s ", kColor5Purple, progress.c_str(),
+                           MoveForward(3 + (kMaxStage - stage_index_)).c_str(),
+                           kEraseInLine, color, output.c_str()));
+  end_of_line_index_ = 4 + kMaxStage + output.size();
 }
 
-void CrostiniStartupStatus::PrintSuccess(bool success) {
-  if (success) {
-    PrintResult(
-        base::StringPrintf("%sdone%s ✔️", kColor2Green, kColor0Normal));
-  } else {
-    PrintResult(base::StringPrintf("%serror%s ❌", kColor1Red, kColor0Normal));
-  }
+void CrostiniStartupStatus::PrintAfterStage(const char* color,
+                                            const std::string& output) {
+  Print(base::StringPrintf("\r%s%s%s", MoveForward(end_of_line_index_).c_str(),
+                           color, output.c_str()));
+  end_of_line_index_ += output.size();
 }
 
 }  // namespace extensions

chrome/browser/extensions/api/terminal/crostini_startup_status.h

@@ -13,8 +13,6 @@
 #include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/crostini/crostini_simple_types.h"
 
-using crostini::mojom::InstallerState;
-
 namespace extensions {
 
 // Displays startup status to the crostini terminal.
@@ -26,8 +24,8 @@ class CrostiniStartupStatus
                         base::OnceClosure callback);
   ~CrostiniStartupStatus() override;
 
-  // Updates the status line every 300ms.
-  void ShowStatusLineAtInterval();
+  // Updates the progress spinner every 300ms.
+  void ShowProgressAtInterval();
 
   // Deletes this object when called.
   void OnCrostiniRestarted(crostini::CrostiniResult result);
@@ -37,37 +35,22 @@ class CrostiniStartupStatus
   FRIEND_TEST_ALL_PREFIXES(CrostiniStartupStatusTest, TestVerbose);
 
   // crostini::CrostiniManager::RestartObserver
-  void OnStageStarted(InstallerState stage) override;
-  void OnComponentLoaded(crostini::CrostiniResult result) override;
-  void OnConciergeStarted(bool success) override;
-  void OnDiskImageCreated(bool success,
-                          vm_tools::concierge::DiskImageStatus status,
-                          int64_t disk_size_available) override;
-  void OnVmStarted(bool success) override;
+  void OnStageStarted(crostini::mojom::InstallerState stage) override;
   void OnContainerDownloading(int32_t download_percent) override;
-  void OnContainerCreated(crostini::CrostiniResult result) override;
-  void OnContainerSetup(bool success) override;
-  void OnContainerStarted(crostini::CrostiniResult result) override;
-  void OnSshKeysFetched(bool success) override;
-  void OnContainerMounted(bool success) override;
 
-  void PrintStatusLine();
   void Print(const std::string& output);
-  void PrintWithTimestamp(const std::string& output);
-  // Moves cursor up and to the right to previous line before status line before
-  // printing output.
-  void PrintResult(const std::string& output);
-  void PrintCrostiniResult(crostini::CrostiniResult result);
-  void PrintSuccess(bool success);
+  void PrintProgress();
+  void PrintStage(const char* color, const std::string& output);
+  void PrintAfterStage(const char* color, const std::string& output);
 
   base::RepeatingCallback<void(const std::string& output)> print_;
   const bool verbose_;
   base::OnceClosure callback_;
   int spinner_index_ = 0;
-  int progress_index_ = 0;
-  // Position of cursor on line above status line.
-  int cursor_position_ = 0;
-  InstallerState stage_ = InstallerState::kStart;
+  int stage_index_ = 0;
+  int end_of_line_index_ = 0;
+  crostini::mojom::InstallerState stage_ =
+      crostini::mojom::InstallerState::kStart;
 
   base::WeakPtrFactory<CrostiniStartupStatus> weak_factory_{this};
 };

chrome/browser/extensions/api/terminal/crostini_startup_status_unittest.cc

@@ -11,6 +11,8 @@
 #include "chrome/browser/chromeos/crostini/crostini_simple_types.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using crostini::mojom::InstallerState;
+
 namespace extensions {
 
 class CrostiniStartupStatusTest : public testing::Test {
@@ -40,45 +42,41 @@ TEST_F(CrostiniStartupStatusTest, TestNotVerbose) {
   auto* startup_status = NewStartupStatus(false);
   startup_status->OnStageStarted(InstallerState::kStart);
   startup_status->OnStageStarted(InstallerState::kInstallImageLoader);
-  startup_status->OnComponentLoaded(crostini::CrostiniResult::SUCCESS);
   startup_status->OnCrostiniRestarted(crostini::CrostiniResult::SUCCESS);
 
   EXPECT_TRUE(done_);
 
-  // Hides cursor, shows cursor.
   EXPECT_EQ(output_.size(), 2u);
-  EXPECT_EQ(output_[0], "\x1b[?25l");
-  EXPECT_EQ(output_[1], "\x1b[?25h");
+  // Hide cursor, init progress.
+  EXPECT_EQ(output_[0], "\x1b[?25l\x1b[35m[         ] ");
+  // CR, delete line, default color, show cursor.
+  EXPECT_EQ(output_[1], "\r\x1b[K\x1b[0m\x1b[?25h");
 }
 
 TEST_F(CrostiniStartupStatusTest, TestVerbose) {
   auto* startup_status = NewStartupStatus(true);
   startup_status->OnStageStarted(InstallerState::kStart);
   startup_status->OnStageStarted(InstallerState::kInstallImageLoader);
-  startup_status->OnComponentLoaded(crostini::CrostiniResult::SUCCESS);
   startup_status->OnCrostiniRestarted(crostini::CrostiniResult::SUCCESS);
   EXPECT_TRUE(done_);
 
-  // Hides cursor, version, start, status, component, status, done, status,
-  // ready, shows cursor.
-  EXPECT_EQ(output_.size(), 10u);
-  EXPECT_EQ(output_[0], "\x1b[?25l");
-  EXPECT_EQ(output_[1].find("Chrome OS "), 24u);
-  EXPECT_EQ(output_[2].substr(24), "Starting... 🤔\r\n");
-  EXPECT_EQ(output_[3],
-            "[\x1b[7m \x1b[27m\x1b[35m........\x1b[0m] \x1b[34m|\x1b[0m\r");
-  EXPECT_EQ(output_[4].substr(24), "Checking cros-termina component...\r\n");
-  EXPECT_EQ(output_[5],
-            "[\x1b[7m  \x1b[27m\x1b[35m.......\x1b[0m] \x1b[34m|\x1b[0m\r");
-  std::string expected = "\x1b[A";
-  for (int i = 0; i < 59; ++i)
-    expected += "\x1b[C";
-  expected += "\x1b[32mdone\x1b[0m \xE2\x9C\x94\xEF\xb8\x8F\r\n";
-  EXPECT_EQ(output_[6], expected);
-  EXPECT_EQ(output_[7],
-            "[\x1b[7m  \x1b[27m\x1b[35m.......\x1b[0m] \x1b[34m|\x1b[0m\r");
-  EXPECT_EQ(output_[8].find("Ready"), 24u);
-  EXPECT_EQ(output_[9], "\x1b[?25h");
+  EXPECT_EQ(output_.size(), 6u);
+  // Hide cursor, init progress.
+  EXPECT_EQ(output_[0], "\x1b[?25l\x1b[35m[         ] ");
+
+  // CR, purple, forward 11, yellow, stage.
+  EXPECT_EQ(output_[1], "\r\x1b[35m[\x1b[11C\x1b[K\x1b[33mInitializing ");
+
+  // CR, purple, progress, forward 10, erase, yellow, stage.
+  EXPECT_EQ(
+      output_[2],
+      "\r\x1b[35m[=\x1b[10C\x1b[K\x1b[33mChecking cros-termina component ");
+
+  // CR, purple, progress, forward 2, erase, green, done, symbol, CRLF.
+  EXPECT_EQ(output_[3], "\r\x1b[35m[=========\x1b[2C\x1b[K\x1b[1;32mdone ");
+
+  // CR, delete line, default color, show cursor;
+  EXPECT_EQ(output_[5], "\r\x1b[K\x1b[0m\x1b[?25h");
 }
 
 }  // namespace extensions

chrome/browser/extensions/api/terminal/terminal_private_api.cc

@@ -240,7 +240,7 @@ TerminalPrivateOpenTerminalProcessFunction::Run() {
                             api::terminal_private::ToString(
                                 api::terminal_private::OUTPUT_TYPE_STDOUT)),
         verbose, std::move(open_process));
-    observer->ShowStatusLineAtInterval();
+    observer->ShowProgressAtInterval();
     mgr->RestartCrostini(
         vm_name, container_name,
         base::BindOnce(&CrostiniStartupStatus::OnCrostiniRestarted,