Support ShowCrostiniUpdateComponentView in Terminal System App

Terminal must have dedicated logic to invoke
ShowCrostiniUpdateComponentView.

Most crostini apps start crostini first and show a
spinner whilst this is happening before launching the app.
The crostini startup logic for this case detects
when to show the update component view.

Terminal System App launches immediately so that it can
show the status of crostini starting, so it requires
extra logic.

TerminalSource dummy files have been extended to include
terminal.html and terminal.js which invokes
chrome.terminalPrivate.openTerminalProcess to
test this logic.

Change-Id: I68ec7b769e038947d107024e36312e9c0689f862
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2079719
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745359}

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

@@ -47,11 +47,8 @@ std::string MoveForward(int i) {
 
 CrostiniStartupStatus::CrostiniStartupStatus(
     base::RepeatingCallback<void(const std::string&)> print,
-    bool verbose,
-    base::OnceClosure callback)
-    : print_(std::move(print)),
-      verbose_(verbose),
-      callback_(std::move(callback)) {
+    bool verbose)
+    : print_(std::move(print)), verbose_(verbose) {
   // Initialize Progress
   Print(base::StringPrintf("%s%s[%s] ", kCursorHide, kColor5Purple,
                            std::string(kMaxStage, ' ').c_str()));
@@ -62,8 +59,6 @@ CrostiniStartupStatus::~CrostiniStartupStatus() = default;
 void CrostiniStartupStatus::OnCrostiniRestarted(
     crostini::CrostiniResult result) {
   if (result != crostini::CrostiniResult::SUCCESS) {
-    LOG(ERROR) << "Error starting crostini for terminal: "
-               << static_cast<int>(result);
     PrintAfterStage(
         kColor1RedBright,
         base::StringPrintf("Error starting penguin container: %d %s\r\n",
@@ -77,8 +72,6 @@ void CrostiniStartupStatus::OnCrostiniRestarted(
   }
   Print(
       base::StringPrintf("\r%s%s%s", kEraseInLine, kColor0Normal, kCursorShow));
-  std::move(callback_).Run();
-  delete this;
 }
 
 void CrostiniStartupStatus::ShowProgressAtInterval() {

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

@@ -20,14 +20,13 @@ class CrostiniStartupStatus
     : public crostini::CrostiniManager::RestartObserver {
  public:
   CrostiniStartupStatus(base::RepeatingCallback<void(const std::string&)> print,
-                        bool verbose,
-                        base::OnceClosure callback);
+                        bool verbose);
   ~CrostiniStartupStatus() override;
 
   // Updates the progress spinner every 300ms.
   void ShowProgressAtInterval();
 
-  // Deletes this object when called.
+  // Called when startup is complete.
   void OnCrostiniRestarted(crostini::CrostiniResult result);
 
  private:
@@ -45,7 +44,6 @@ class CrostiniStartupStatus
 
   base::RepeatingCallback<void(const std::string& output)> print_;
   const bool verbose_;
-  base::OnceClosure callback_;
   int spinner_index_ = 0;
   int stage_index_ = 0;
   int end_of_line_index_ = 0;

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

@@ -27,9 +27,7 @@ class CrostiniStartupStatusTest : public testing::Test {
     return new CrostiniStartupStatus(
         base::BindRepeating(&CrostiniStartupStatusTest::Print,
                             base::Unretained(this)),
-        verbose,
-        base::BindOnce(&CrostiniStartupStatusTest::Done,
-                       base::Unretained(this)));
+        verbose);
   }
 
   void SetUp() override {}
@@ -44,8 +42,6 @@ TEST_F(CrostiniStartupStatusTest, TestNotVerbose) {
   startup_status->OnStageStarted(InstallerState::kInstallImageLoader);
   startup_status->OnCrostiniRestarted(crostini::CrostiniResult::SUCCESS);
 
-  EXPECT_TRUE(done_);
-
   EXPECT_EQ(output_.size(), 2u);
   // Hide cursor, init progress.
   EXPECT_EQ(output_[0], "\x1b[?25l\x1b[35m[         ] ");
@@ -58,7 +54,6 @@ TEST_F(CrostiniStartupStatusTest, TestVerbose) {
   startup_status->OnStageStarted(InstallerState::kStart);
   startup_status->OnStageStarted(InstallerState::kInstallImageLoader);
   startup_status->OnCrostiniRestarted(crostini::CrostiniResult::SUCCESS);
-  EXPECT_TRUE(done_);
 
   EXPECT_EQ(output_.size(), 6u);
   // Hide cursor, init progress.

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

@@ -14,11 +14,14 @@
 #include "base/json/json_writer.h"
 #include "base/lazy_instance.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/strings/stringprintf.h"
 #include "base/system/sys_info.h"
 #include "base/task/post_task.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/crostini/crostini_features.h"
+#include "chrome/browser/chromeos/crostini/crostini_manager.h"
 #include "chrome/browser/chromeos/crostini/crostini_pref_names.h"
+#include "chrome/browser/chromeos/crostini/crostini_util.h"
 #include "chrome/browser/extensions/api/terminal/crostini_startup_status.h"
 #include "chrome/browser/extensions/api/terminal/terminal_extension_helper.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -226,26 +229,27 @@ TerminalPrivateOpenTerminalProcessFunction::Run() {
                   crostini::kCrostiniDefaultContainerName);
     std::string startup_id = params_args.GetSwitchValueASCII(kSwitchStartupId);
 
-    auto open_process =
-        base::BindOnce(&TerminalPrivateOpenTerminalProcessFunction::OpenProcess,
-                       this, user_id_hash, tab_id, vmshell_cmd.argv());
     auto* mgr = crostini::CrostiniManager::GetForProfile(profile);
     bool verbose =
         !mgr->GetContainerInfo(crostini::kCrostiniDefaultVmName,
                                crostini::kCrostiniDefaultContainerName)
              .has_value();
-    auto* observer = new CrostiniStartupStatus(
+    auto observer = std::make_unique<CrostiniStartupStatus>(
         base::BindRepeating(&NotifyProcessOutput, browser_context(), tab_id,
                             startup_id,
                             api::terminal_private::ToString(
                                 api::terminal_private::OUTPUT_TYPE_STDOUT)),
-        verbose, std::move(open_process));
+        verbose);
+    // Save copy of pointer for RestartObserver before moving object.
+    CrostiniStartupStatus* observer_ptr = observer.get();
     observer->ShowProgressAtInterval();
     mgr->RestartCrostini(
         vm_name, container_name,
-        base::BindOnce(&CrostiniStartupStatus::OnCrostiniRestarted,
-                       base::Unretained(observer)),
-        observer);
+        base::BindOnce(
+            &TerminalPrivateOpenTerminalProcessFunction::OnCrostiniRestarted,
+            this, std::move(observer), user_id_hash, tab_id,
+            vmshell_cmd.argv()),
+        observer_ptr);
   } else {
     // command=[unrecognized].
     return RespondNow(Error("Invalid process name: " + params->process_name));
@@ -253,6 +257,31 @@ TerminalPrivateOpenTerminalProcessFunction::Run() {
   return RespondLater();
 }
 
+void TerminalPrivateOpenTerminalProcessFunction::OnCrostiniRestarted(
+    std::unique_ptr<CrostiniStartupStatus> startup_status,
+    const std::string& user_id_hash,
+    int tab_id,
+    const std::vector<std::string>& arguments,
+    crostini::CrostiniResult result) {
+  startup_status->OnCrostiniRestarted(result);
+  if (result == crostini::CrostiniResult::SUCCESS) {
+    OpenProcess(user_id_hash, tab_id, arguments);
+  } else {
+    const std::string msg =
+        base::StringPrintf("Error starting crostini for terminal: %d", result);
+    LOG(ERROR) << msg;
+    Respond(Error(msg));
+
+    // Special handling to update terminal component.
+    if (result == crostini::CrostiniResult::OFFLINE_WHEN_UPGRADE_REQUIRED ||
+        result == crostini::CrostiniResult::LOAD_COMPONENT_FAILED) {
+      ShowCrostiniUpdateComponentView(
+          Profile::FromBrowserContext(browser_context()),
+          crostini::CrostiniUISurface::kAppList);
+    }
+  }
+}
+
 void TerminalPrivateOpenTerminalProcessFunction::OpenProcess(
     const std::string& user_id_hash,
     int tab_id,

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

@@ -5,9 +5,11 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_API_TERMINAL_TERMINAL_PRIVATE_API_H_
 #define CHROME_BROWSER_EXTENSIONS_API_TERMINAL_TERMINAL_PRIVATE_API_H_
 
+#include <memory>
 #include <string>
 #include <vector>
 
+#include "chrome/browser/chromeos/crostini/crostini_simple_types.h"
 #include "chrome/browser/profiles/profile.h"
 #include "extensions/browser/browser_context_keyed_api_factory.h"
 #include "extensions/browser/extension_function.h"
@@ -17,6 +19,8 @@ class PrefChangeRegistrar;
 
 namespace extensions {
 
+class CrostiniStartupStatus;
+
 class TerminalPrivateAPI : public BrowserContextKeyedAPI {
  public:
   explicit TerminalPrivateAPI(content::BrowserContext* context);
@@ -50,6 +54,14 @@ class TerminalPrivateOpenTerminalProcessFunction : public ExtensionFunction {
   ExtensionFunction::ResponseAction Run() override;
 
  private:
+  // Callback for when starting crostini is complete.
+  void OnCrostiniRestarted(
+      std::unique_ptr<CrostiniStartupStatus> startup_status,
+      const std::string& user_id_hash,
+      int tab_id,
+      const std::vector<std::string>& arguments,
+      crostini::CrostiniResult result);
+
   using ProcessOutputCallback =
       base::Callback<void(const std::string& terminal_id,
                           const std::string& output_type,

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

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/views/crostini/crostini_update_component_view.h"
 
+#include "base/feature_list.h"
 #include "base/metrics/histogram_base.h"
 #include "base/run_loop.h"
 #include "base/test/metrics/histogram_tester.h"
@@ -12,13 +13,36 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/views/crostini/crostini_browser_test_util.h"
+#include "chrome/browser/ui/web_applications/system_web_app_ui_utils.h"
+#include "chrome/browser/web_applications/system_web_app_manager.h"
+#include "chrome/browser/web_applications/web_app_provider.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_concierge_client.h"
 #include "components/crx_file/id_util.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+class LoadFinishedWaiter : public content::WebContentsObserver {
+ public:
+  explicit LoadFinishedWaiter(content::WebContents* contents)
+      : content::WebContentsObserver(contents) {}
+
+  ~LoadFinishedWaiter() override = default;
+
+  void Wait() { run_loop_.Run(); }
+
+  // content::WebContentsObserver:
+  void DidFinishLoad(content::RenderFrameHost* render_frame_host,
+                     const GURL& validated_url) override {
+    run_loop_.Quit();
+  }
+
+ private:
+  base::RunLoop run_loop_;
+};
+
 class CrostiniUpdateComponentViewBrowserTest
     : public CrostiniDialogBrowserTest {
  public:
@@ -99,13 +123,20 @@ IN_PROC_BROWSER_TEST_F(CrostiniUpdateComponentViewBrowserTest,
   ExpectNoView();
 
   UnregisterTermina();
-  crostini::LaunchCrostiniApp(browser()->profile(),
-                              crostini::kCrostiniTerminalId, 0);
+  crostini::LaunchCrostiniApp(browser()->profile(), crostini::GetTerminalId(),
+                              0);
   ExpectNoView();
 }
 
 IN_PROC_BROWSER_TEST_F(CrostiniUpdateComponentViewBrowserTest,
                        LaunchAppOffline_UpgradeNeeded) {
+  // Ensure Terminal System App is installed if enabled.
+  if (base::FeatureList::IsEnabled(features::kTerminalSystemApp)) {
+    web_app::WebAppProvider::Get(browser()->profile())
+        ->system_web_app_manager()
+        .InstallSystemAppsForTesting();
+  }
+
   base::HistogramTester histogram_tester;
   SetConnectionType(network::mojom::ConnectionType::CONNECTION_NONE);
   crostini::CrostiniManager::GetForProfile(browser()->profile())
@@ -114,8 +145,18 @@ IN_PROC_BROWSER_TEST_F(CrostiniUpdateComponentViewBrowserTest,
   ExpectNoView();
 
   UnregisterTermina();
-  crostini::LaunchCrostiniApp(browser()->profile(),
-                              crostini::kCrostiniTerminalId, 0);
+  crostini::LaunchCrostiniApp(browser()->profile(), crostini::GetTerminalId(),
+                              0);
+
+  // For Terminal System App, we must wait for browser to load.
+  if (base::FeatureList::IsEnabled(features::kTerminalSystemApp)) {
+    Browser* terminal_browser = web_app::FindSystemWebAppBrowser(
+        browser()->profile(), web_app::SystemAppType::TERMINAL);
+    CHECK_NE(nullptr, terminal_browser);
+    LoadFinishedWaiter(terminal_browser->tab_strip_model()->GetWebContentsAt(0))
+        .Wait();
+  }
+
   ExpectView();
 
   ActiveView()->AcceptDialog();

chrome/browser/ui/webui/chromeos/terminal/terminal_source.cc

@@ -58,10 +58,14 @@ void ReadFile(const std::string& relative_path,
             {"manifest.json", R"({
                "name": "Test Terminal",
                "icons": [{ "src": "/icon.svg", "sizes": "any" }],
-               "start_url": "/html/pwa.html"})"},
+               "start_url": "/html/terminal.html"})"},
             {"icon.svg",
              "<svg xmlns='http://www.w3.org/2000/svg'><rect "
              "fill='red'/></svg>"},
+            {"html/terminal.html", "<script src='/js/terminal.js'></script>"},
+            {"js/terminal.js",
+             "chrome.terminalPrivate.openTerminalProcess("
+             "'vmshell', [], () => {})"},
         });
     auto it = kTestFiles->find(relative_path);
     if (it != kTestFiles->end()) {