Split AppInstall and AppInstallController.

This is an iteration to make the implementation of --install
cross-platform.

SplashScreen and AppInstallController are injected by MakeAppInstall
implementation using two closures. The Windows code has working
objects for the two, while the other platforms use do-nothing stubs.

There is a small change in the integration test to invoke Wake for
the Windows execution path. The tests pass without the change and
removing this platform difference in the test execution seems useful.

Change-Id: Ie3c75d52742aafc7f2f96e16cc64583f501018f7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2385896
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804958}

chrome/updater/BUILD.gn

@@ -55,6 +55,7 @@ if (is_win || is_mac) {
       "registration_data.cc",
       "registration_data.h",
       "service_scope.h",
+      "splash_screen.h",
       "unzipper.cc",
       "unzipper.h",
       "update_service.cc",
@@ -83,6 +84,8 @@ if (is_win || is_mac) {
     sources = [
       "app/app.cc",
       "app/app.h",
+      "app/app_install.cc",
+      "app/app_install.h",
       "app/app_register.cc",
       "app/app_register.h",
       "app/app_server.cc",
@@ -164,6 +167,7 @@ if (is_win || is_mac) {
       ":base",
       ":version_header",
       "//base",
+      "//base:i18n",
       "//chrome/updater/device_management",
       "//components/crash/core/common:crash_key",
       "//components/crx_file:crx_file",
@@ -227,46 +231,6 @@ if (is_win || is_mac) {
     output = "$target_gen_dir/updater_version.h"
   }
 
-  # This build target is defined to minimize the impact of -Wno-missing-braces
-  # compiler switch. In the future it might be possible to isolate the
-  # dependency of ATL in the UI so ATL headers are not visible in the
-  # compilation units outside the UI itself.
-  # TODO(sorin): https://crbug.com/1014311
-  source_set("app_install") {
-    sources = [ "app/app_install.h" ]
-
-    if (is_win) {
-      sources += [ "win/app_install.cc" ]
-    }
-
-    deps = [
-      ":lib",
-      "//base",
-      "//chrome/updater:base",
-      "//chrome/updater:version_header",
-    ]
-    if (is_mac) {
-      deps += [ "//chrome/updater/mac:updater_setup_sources" ]
-    }
-
-    if (is_win) {
-      deps += [
-        "//base:i18n",
-        "//chrome/updater/win:install_progress_observer",
-        "//chrome/updater/win:lib",
-        "//chrome/updater/win/ui",
-        "//components/prefs",
-        "//components/update_client",
-      ]
-    }
-
-    allow_circular_includes_from = [ ":lib" ]
-
-    if (is_win) {
-      cflags_cc = [ "-Wno-missing-braces" ]
-    }
-  }
-
   source_set("updater_tests_support") {
     testonly = true
 

chrome/updater/app/app_install.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/updater/app/app_install.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/check.h"
+#include "base/command_line.h"
+#include "base/i18n/icu_util.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "base/version.h"
+#include "build/build_config.h"
+#include "chrome/updater/constants.h"
+#include "chrome/updater/control_service.h"
+#include "chrome/updater/persisted_data.h"
+#include "chrome/updater/prefs.h"
+#include "chrome/updater/setup.h"
+#include "chrome/updater/updater_version.h"
+#include "components/prefs/pref_service.h"
+
+namespace updater {
+
+#if !defined(OS_WIN)
+namespace {
+
+class SplashScreenImpl : public SplashScreen {
+ public:
+  // Overrides for SplashScreen.
+  void Show() override {}
+  void Dismiss(base::OnceClosure callback) override {
+    base::SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                     std::move(callback));
+  }
+};
+
+class AppInstallControllerImpl : public AppInstallController {
+ public:
+  // Override for AppInstallController.
+  void InstallApp(const std::string& app_id,
+                  base::OnceCallback<void(int)> callback) override {
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback), 0));
+  }
+
+ protected:
+  ~AppInstallControllerImpl() override = default;
+};
+
+}  // namespace
+
+scoped_refptr<App> MakeAppInstall() {
+  return base::MakeRefCounted<AppInstall>(
+      base::BindRepeating([]() -> std::unique_ptr<SplashScreen> {
+        return std::make_unique<SplashScreenImpl>();
+      }),
+      base::BindRepeating([]() -> scoped_refptr<AppInstallController> {
+        return base::MakeRefCounted<AppInstallControllerImpl>();
+      }));
+}
+#endif  // !defined(OS_WIN)
+
+AppInstall::AppInstall(SplashScreen::Maker splash_screen_maker,
+                       AppInstallController::Maker app_install_controller_maker)
+    : splash_screen_maker_(std::move(splash_screen_maker)),
+      app_install_controller_maker_(app_install_controller_maker) {
+  DCHECK(splash_screen_maker_);
+  DCHECK(app_install_controller_maker_);
+}
+
+AppInstall::~AppInstall() = default;
+
+void AppInstall::Initialize() {
+  base::i18n::InitializeICU();
+
+  // Creating |global_prefs_| requires acquiring a global lock, and this lock is
+  // typically owned by the RPC server. That means that if the server is
+  // running, the following code will block, and the install will not proceed
+  // until the server releases the lock.
+  global_prefs_ = CreateGlobalPrefs();
+  local_prefs_ = CreateLocalPrefs();
+}
+
+void AppInstall::FirstTaskRun() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+
+  splash_screen_ = splash_screen_maker_.Run();
+  splash_screen_->Show();
+
+  base::ThreadPool::PostTaskAndReplyWithResult(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskPriority::USER_BLOCKING,
+       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
+      base::BindOnce([]() { return InstallCandidate(false); }),
+      base::BindOnce(
+          [](SplashScreen* splash_screen, base::OnceCallback<void(int)> done,
+             int result) {
+            splash_screen->Dismiss(base::BindOnce(std::move(done), result));
+          },
+          splash_screen_.get(),
+          base::BindOnce(&AppInstall::InstallCandidateDone, this)));
+}
+
+// Updates the prefs if installing the updater is successful, then continue
+// installing the application if --app-id is specified on the command line.
+void AppInstall::InstallCandidateDone(int result) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (result != 0) {
+    Shutdown(result);
+    return;
+  }
+
+  // Invoke |HandleAppId| to continue the execution flow.
+  MakeActive(base::BindOnce(&AppInstall::HandleAppId, this));
+}
+
+void AppInstall::HandleAppId() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // This releases the prefs lock, and the RPC server can be started.
+  global_prefs_ = nullptr;
+  local_prefs_ = nullptr;
+
+  // If no app id is provided, then invoke ControlService::Run to wake
+  // this version of the updater, do an update check, and possibly promote
+  // this version as a result.
+  const std::string app_id =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(kAppIdSwitch);
+  if (app_id.empty()) {
+    // The instance of |CreateControlService| has sequence affinity. Bind it
+    // in the closure to ensure it is released in this sequence.
+    scoped_refptr<ControlService> control_service = CreateControlService();
+    control_service->Run(base::BindOnce(
+        [](scoped_refptr<ControlService> /*control_service*/,
+           scoped_refptr<AppInstall> app_install) { app_install->Shutdown(0); },
+        control_service, base::WrapRefCounted(this)));
+    return;
+  }
+
+  app_install_controller_ = app_install_controller_maker_.Run();
+  app_install_controller_->InstallApp(
+      app_id, base::BindOnce(&AppInstall::Shutdown, this));
+}
+
+// TODO(crbug.com/1109231) - this is a temporary workaround.
+void AppInstall::MakeActive(base::OnceClosure done) {
+  local_prefs_->SetQualified(true);
+  local_prefs_->GetPrefService()->CommitPendingWrite(base::BindOnce(
+      [](base::OnceClosure done, PrefService* pref_service) {
+        DCHECK(pref_service);
+        auto persisted_data = base::MakeRefCounted<PersistedData>(pref_service);
+        persisted_data->SetProductVersion(
+            kUpdaterAppId, base::Version(UPDATER_VERSION_STRING));
+        pref_service->CommitPendingWrite(std::move(done));
+      },
+      std::move(done), global_prefs_->GetPrefService()));
+}
+
+}  // namespace updater

chrome/updater/app/app_install.h

@@ -5,11 +5,91 @@
 #ifndef CHROME_UPDATER_APP_APP_INSTALL_H_
 #define CHROME_UPDATER_APP_APP_INSTALL_H_
 
-#include "base/memory/scoped_refptr.h"
+#include <memory>
+#include <string>
+
+#include "base/callback_forward.h"
+#include "base/memory/ref_counted.h"
+#include "base/sequence_checker.h"
+#include "chrome/updater/app/app.h"
+#include "chrome/updater/splash_screen.h"
+
+namespace base {
+class TaskRunner;
+}
 
 namespace updater {
 
-class App;
+class LocalPrefs;
+class GlobalPrefs;
+
+// This class defines an interface for installing an application. The interface
+// is intended to be implemented for scenerios where UI and RPC calls to
+// |UpdateService| are involved, hence the word `controller` in the name of
+// the ]interface.
+class AppInstallController
+    : public base::RefCountedThreadSafe<AppInstallController> {
+ public:
+  using Maker = base::RepeatingCallback<scoped_refptr<AppInstallController>()>;
+  virtual void InstallApp(const std::string& app_id,
+                          base::OnceCallback<void(int)> callback) = 0;
+
+ protected:
+  virtual ~AppInstallController() = default;
+
+ private:
+  friend class base::RefCountedThreadSafe<AppInstallController>;
+};
+
+// Sets the updater up, shows up a splash screen, then installs an application
+// while displaying the UI progress window.
+class AppInstall : public App {
+ public:
+  AppInstall(SplashScreen::Maker splash_screen_maker,
+             AppInstallController::Maker app_install_controller_maker);
+
+ private:
+  ~AppInstall() override;
+
+  // Overrides for App.
+  void Initialize() override;
+  void FirstTaskRun() override;
+
+  void InstallCandidateDone(int result);
+
+  // Handles the --app-id command line argument, and triggers installing of the
+  // corresponding app-id if the argument is present.
+  void HandleAppId();
+
+  // Makes this version of the updater active, self-registers for updates, then
+  // runs the |done| closure.
+  void MakeActive(base::OnceClosure done);
+
+  // Bound to the main sequence.
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  // Creates instances of |SplashScreen|.
+  SplashScreen::Maker splash_screen_maker_;
+
+  // Creates instances of |AppInstallController|.
+  AppInstallController::Maker app_install_controller_maker_;
+
+  // The splash screen has a fading effect. That means that the splash screen
+  // needs to be alive for a while, until the fading effect is over.
+  std::unique_ptr<SplashScreen> splash_screen_;
+
+  scoped_refptr<AppInstallController> app_install_controller_;
+
+  // These prefs objects are used to make the updater active and register this
+  // version of the updater for self-updates.
+  //
+  // TODO(crbug.com/1109231) - this is a temporary workaround until a better
+  // fix is found.
+  std::unique_ptr<LocalPrefs> local_prefs_;
+  std::unique_ptr<GlobalPrefs> global_prefs_;
+
+  scoped_refptr<base::TaskRunner> make_active_task_runner_;
+};
 
 scoped_refptr<App> MakeAppInstall();
 

chrome/updater/mac/BUILD.gn

@@ -12,7 +12,6 @@ group("mac") {
     ":browser_install_script",
     ":updater_bundle",
     ":updater_install_script",
-    "//chrome/updater:app_install",
     "//chrome/updater/mac/signing",
   ]
 }
@@ -61,7 +60,6 @@ mac_app_bundle("updater_bundle") {
   deps = [
     ":network_fetcher_sources",
     ":updater_setup_sources",
-    "//chrome/updater:app_install",
     "//chrome/updater:lib",
   ]
 }
@@ -169,7 +167,6 @@ source_set("updater_setup_tests") {
     "//base",
     "//base/test:test_support",
     "//chrome/common:constants",
-    "//chrome/updater:app_install",
     "//testing/gtest",
   ]
 }

chrome/updater/splash_screen.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_UPDATER_SPLASH_SCREEN_H_
+#define CHROME_UPDATER_SPLASH_SCREEN_H_
+
+#include <memory>
+
+#include "base/callback_forward.h"
+
+namespace updater {
+
+// Displays a splash screen during install.
+class SplashScreen {
+ public:
+  using Maker = base::RepeatingCallback<std::unique_ptr<SplashScreen>()>;
+  virtual ~SplashScreen() = default;
+
+  virtual void Show() = 0;
+  virtual void Dismiss(base::OnceClosure callback) = 0;
+};
+
+}  // namespace updater
+
+#endif  // CHROME_UPDATER_SPLASH_SCREEN_H_

chrome/updater/test/integration_tests.cc

@@ -69,8 +69,8 @@ TEST_F(IntegrationTest, InstallAndPromote) {
   ExpectActiveVersion(UPDATER_VERSION_STRING);
 #else
   ExpectActiveVersion("0");
-  RunWake(0);  // Candidate qualifies and promotes to active.
 #endif
+  RunWake(0);  // Candidate qualifies and promotes to active.
   ExpectActiveVersion(UPDATER_VERSION_STRING);
   ExpectActive();
   Uninstall();

chrome/updater/updater.cc

@@ -36,8 +36,8 @@
 
 // Instructions For Windows.
 // - To install only the updater, run "updatersetup.exe" from the build out dir.
-// - To install Chrome and the updater, do the same but use the --appid:
-//    updatersetup.exe --appid={8A69D345-D564-463c-AFF1-A69D9E530F96}
+// - To install Chrome and the updater, do the same but use the --app-id:
+//    updatersetup.exe --app-id={8A69D345-D564-463c-AFF1-A69D9E530F96}
 // - To uninstall, run "updater.exe --uninstall" from its install directory,
 // which is under %LOCALAPPDATA%\Google\GoogleUpdater, or from the |out|
 // directory of the build.

chrome/updater/win/BUILD.gn

@@ -25,10 +25,10 @@ executable("updater") {
   configs += [ "//build/config/win:windowed" ]
 
   deps = [
+    ":app_install_controller",
     ":lib",
     ":version_resources",
     "//build/win:default_exe_manifest",
-    "//chrome/updater:app_install",
     "//chrome/updater:lib",
     "//chrome/updater/win/ui:ui_resources",
   ]
@@ -136,6 +136,33 @@ source_set("install_progress_observer") {
   deps = [ "//base" ]
 }
 
+# This build target is defined to minimize the impact of -Wno-missing-braces
+# compiler switch. In the future it might be possible to isolate the
+# dependency of ATL in the UI so ATL headers are not visible in the
+# compilation units outside the UI itself.
+# TODO(sorin): https://crbug.com/1014311
+source_set("app_install_controller") {
+  if (is_win) {
+    visibility = [ "//chrome/updater/win/*" ]
+    allow_circular_includes_from = [ "//chrome/updater:lib" ]
+
+    cflags_cc = [ "-Wno-missing-braces" ]
+
+    sources = [ "app_install_controller.cc" ]
+
+    deps = [
+      ":install_progress_observer",
+      ":lib",
+      "//base",
+      "//base:i18n",
+      "//chrome/updater:base",
+      "//chrome/updater:lib",
+      "//chrome/updater:version_header",
+      "//chrome/updater/win/ui",
+    ]
+  }
+}
+
 source_set("updater_tests") {
   testonly = true
 
@@ -149,11 +176,11 @@ source_set("updater_tests") {
   ]
 
   deps = [
+    ":app_install_controller",
     ":constants",
     ":lib",
     ":tag_extractor",
     "//base/test:test_support",
-    "//chrome/updater:app_install",
     "//testing/gtest",
     "//url:url",
   ]
@@ -183,10 +210,10 @@ test("updater_unittests") {
   ]
 
   deps = [
+    ":app_install_controller",
     ":lib",
     "//base",
     "//base/test:test_support",
-    "//chrome/updater:app_install",
     "//chrome/updater:version_header",
     "//chrome/updater/win/test:test_executables",
     "//chrome/updater/win/test:test_strings",

chrome/updater/win/test/BUILD.gn

@@ -25,8 +25,8 @@ source_set("test_common") {
 
   deps = [
     "//base",
-    "//chrome/updater:app_install",
     "//chrome/updater:base",
+    "//chrome/updater/win:app_install_controller",
     "//chrome/updater/win:lib",
   ]
 }

chrome/updater/win/ui/splash_screen.h

@@ -12,6 +12,7 @@
 #include "base/threading/thread_checker.h"
 #include "base/win/atl.h"
 #include "base/win/scoped_gdi_object.h"
+#include "chrome/updater/splash_screen.h"
 #include "chrome/updater/win/ui/owner_draw_controls.h"
 #include "chrome/updater/win/ui/resources/resources.grh"
 
@@ -19,8 +20,9 @@ namespace updater {
 namespace ui {
 
 class SplashScreen : public CAxDialogImpl<SplashScreen>,
+                     public CustomDlgColors,
                      public OwnerDrawTitleBar,
-                     public CustomDlgColors {
+                     public updater::SplashScreen {
  public:
   static constexpr int IDD = IDD_PROGRESS;
 
@@ -29,10 +31,9 @@ class SplashScreen : public CAxDialogImpl<SplashScreen>,
   SplashScreen& operator=(const SplashScreen&) = delete;
   ~SplashScreen() override;
 
-  void Show();
-
-  // Does alpha blending and closese the window.
-  void Dismiss(base::OnceClosure on_close_closure);
+  // Overrides for SplashScreen.
+  void Show() override;
+  void Dismiss(base::OnceClosure on_close_closure) override;
 
   BEGIN_MSG_MAP(SplashScreen)
     MESSAGE_HANDLER(WM_TIMER, OnTimer)