Updater: Update test app to follow proposed Chromium registration.

Specifically, the test app should install and register itself by:
 1. Call --install on its bundled updater.
 2. Use IPC registration to register the test app's presence.

Also, this CL unifies the test app appid on Win and Mac, and fixes
ownership / initialization issues associated with the test app
initializing services prior to updater installation.

There are also some minor code cleanups around repeated vs once
callbacks.

Change-Id: Ib033be4479ded6b83637f24b133eea4f38004fac
Bug: 1122773
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2422138
Commit-Queue: Joshua Pawlicki <waffles@chromium.org>
Auto-Submit: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809794}

chrome/updater/BUILD.gn

@@ -279,7 +279,7 @@ if (is_win || is_mac) {
       "//base/test:test_support",
       "//chrome/common:constants",
       "//chrome/updater/device_management:unittest",
-      "//chrome/updater/test/test_app",
+      "//chrome/updater/test/test_app:constants",
       "//chrome/updater/test/test_app:version_header",
       "//chrome/updater/tools:unittest",
       "//components/prefs:test_support",
@@ -313,8 +313,6 @@ if (is_win || is_mac) {
         "mac/scoped_xpc_service_mock.mm",
         "mac/update_service_out_of_process_test.mm",
         "test/integration_tests_mac.mm",
-        "test/test_app/constants.cc",
-        "test/test_app/constants.h",
       ]
 
       deps += [
@@ -331,7 +329,7 @@ if (is_win || is_mac) {
 
       data_deps = [
         "//chrome/updater/mac:updater_bundle",
-        "//chrome/updater/test/test_app:test_app",
+        "//chrome/updater/test/test_app",
       ]
     }
   }

chrome/updater/test/integration_tests.cc

@@ -5,8 +5,12 @@
 #include "chrome/updater/test/integration_tests.h"
 
 #include "base/test/task_environment.h"
+#include "base/version.h"
 #include "build/build_config.h"
+#include "chrome/updater/persisted_data.h"
 #include "chrome/updater/prefs.h"
+#include "chrome/updater/test/test_app/constants.h"
+#include "chrome/updater/test/test_app/test_app_version.h"
 #include "chrome/updater/updater_version.h"
 #include "chrome/updater/util.h"
 #include "testing/gtest/include/gtest/gtest.h"

chrome/updater/test/test_app/BUILD.gn

@@ -45,12 +45,17 @@ process_version("version_header") {
   output = "$target_gen_dir/test_app_version.h"
 }
 
+source_set("constants") {
+  sources = [
+    "constants.cc",
+    "constants.h",
+  ]
+}
+
 source_set("app_sources") {
   sources = [
     "//chrome/updater/app/app.cc",
     "//chrome/updater/app/app.h",
-    "constants.cc",
-    "constants.h",
     "test_app.cc",
     "test_app.h",
     "test_app_mac.mm",
@@ -64,6 +69,7 @@ source_set("app_sources") {
   ]
 
   deps = [
+    ":constants",
     ":version_header",
     "//base",
     "//chrome/updater:base",

chrome/updater/test/test_app/constants.cc

@@ -10,9 +10,6 @@ const char kInstallUpdaterSwitch[] = "install-updater";
 const char kRegisterUpdaterSwitch[] = "register-updater";
 const char kRegisterToUpdaterSwitch[] = "ipc-register";
 const char kForegroundUpdateSwitch[] = "ipc-update";
-
-#if defined(OS_WIN)
-const base::char16 kAppId[] = L"6f0f9a34-a0ab-4a75-a0eb-6eab78d0dc4b";
-#endif  // defined(OS_WIN)
+const char kTestAppId[] = "6f0f9a34-a0ab-4a75-a0eb-6eab78d0dc4b";
 
 }  // namespace updater

chrome/updater/test/test_app/constants.h

@@ -5,7 +5,6 @@
 #ifndef CHROME_UPDATER_TEST_TEST_APP_CONSTANTS_H_
 #define CHROME_UPDATER_TEST_TEST_APP_CONSTANTS_H_
 
-#include "base/strings/string16.h"
 #include "build/build_config.h"
 
 namespace updater {
@@ -22,10 +21,8 @@ extern const char kRegisterToUpdaterSwitch[];
 // Initiates a foreground update through IPC.
 extern const char kForegroundUpdateSwitch[];
 
-#if defined(OS_WIN)
-// Test app GUID.
-extern const base::char16 kAppId[];
-#endif  // defined(OS_WIN)
+// The application ID of the test app.
+extern const char kTestAppId[];
 
 // Update process state machine.
 enum class UpdateStatus {

chrome/updater/test/test_app/test_app.cc

@@ -43,8 +43,6 @@ class TestApp : public App {
                        const std::string& version,
                        int64_t size,
                        const base::string16& message);
-
-  scoped_refptr<UpdateClient> update_client;
 };
 
 void TestApp::SetUpdateStatus(UpdateStatus status,
@@ -83,19 +81,17 @@ void TestApp::SetUpdateStatus(UpdateStatus status,
 }
 
 void TestApp::Register() {
-  update_client->Register(base::BindRepeating(&TestApp::Shutdown, this));
+  UpdateClient::Create()->Register(base::BindOnce(&TestApp::Shutdown, this));
 }
 
 void TestApp::DoForegroundUpdate() {
-  update_client->CheckForUpdate(
+  UpdateClient::Create()->CheckForUpdate(
       base::BindRepeating(&TestApp::SetUpdateStatus, this));
 }
 
 void TestApp::ParseCommandLine() {
   const base::CommandLine* command_line =
       base::CommandLine::ForCurrentProcess();
-  update_client = UpdateClient::Create();
-
   if (command_line->HasSwitch(kInstallUpdaterSwitch)) {
     base::ThreadPool::PostTaskAndReplyWithResult(
         FROM_HERE, {base::MayBlock()}, base::BindOnce(&InstallUpdater),
@@ -107,7 +103,17 @@ void TestApp::ParseCommandLine() {
   } else if (command_line->HasSwitch(kRegisterUpdaterSwitch)) {
     base::ThreadPool::PostTaskAndReplyWithResult(
         FROM_HERE, {base::MayBlock()}, base::BindOnce(&InstallUpdater),
-        base::BindOnce(&TestApp::Shutdown, this));
+        base::BindOnce(
+            [](base::OnceClosure register_func,
+               base::OnceCallback<void(int)> shutdown_func, int error) {
+              if (error) {
+                std::move(shutdown_func).Run(error);
+                return;
+              }
+              std::move(register_func).Run();
+            },
+            base::BindOnce(&TestApp::Register, this),
+            base::BindOnce(&TestApp::Shutdown, this)));
   } else {
     Shutdown(0);
   }

chrome/updater/test/test_app/test_app_mac.mm

@@ -63,9 +63,7 @@ int InstallUpdater() {
   }
 
   base::CommandLine command(updater_executable_path);
-  command.AppendSwitch(kRegisterSwitch);
-  command.AppendSwitchASCII(kAppIdSwitch, TEST_APP_FULLNAME_STRING);
-  command.AppendSwitchASCII(kAppVersionSwitch, TEST_APP_VERSION_STRING);
+  command.AppendSwitch(kInstallSwitch);
   command.AppendSwitchASCII("--vmodule", "*/updater/*=2");
 
   std::string output;

chrome/updater/test/test_app/update_client.cc

@@ -29,11 +29,10 @@ bool UpdateClient::CanCheckForUpdate() {
   return CanDialIPC();
 }
 
-void UpdateClient::Register(base::RepeatingCallback<void(int)> callback) {
-  registration_callback_ = std::move(callback);
-
+void UpdateClient::Register(base::OnceCallback<void(int)> callback) {
   BeginRegister({}, {}, TEST_APP_VERSION_STRING,
-                base::BindOnce(&UpdateClient::RegistrationCompleted, this));
+                base::BindOnce(&UpdateClient::RegistrationCompleted, this,
+                               std::move(callback)));
 }
 
 void UpdateClient::CheckForUpdate(StatusCallback callback) {
@@ -84,15 +83,15 @@ void UpdateClient::HandleStatusUpdate(UpdateService::UpdateState update_state) {
                                 base::string16()));
 }
 
-void UpdateClient::RegistrationCompleted(UpdateService::Result result) {
+void UpdateClient::RegistrationCompleted(base::OnceCallback<void(int)> callback,
+                                         UpdateService::Result result) {
   if (result != UpdateService::Result::kSuccess) {
     LOG(ERROR) << "Updater registration error: "
                << base::NumberToString(static_cast<int>(result));
   }
 
   callback_task_runner_->PostTask(
-      FROM_HERE,
-      base::BindOnce(registration_callback_, static_cast<int>(result)));
+      FROM_HERE, base::BindOnce(std::move(callback), static_cast<int>(result)));
 }
 
 void UpdateClient::UpdateCompleted(UpdateService::Result result) {

chrome/updater/test/test_app/update_client.h

@@ -35,10 +35,11 @@ class UpdateClient : public base::RefCountedThreadSafe<UpdateClient> {
 
   UpdateClient();
 
-  void Register(base::RepeatingCallback<void(int)> callback);
+  void Register(base::OnceCallback<void(int)> callback);
   void CheckForUpdate(StatusCallback callback);
   void HandleStatusUpdate(UpdateService::UpdateState update_state);
-  void RegistrationCompleted(UpdateService::Result result);
+  void RegistrationCompleted(base::OnceCallback<void(int)> callback,
+                             UpdateService::Result result);
   void UpdateCompleted(UpdateService::Result result);
 
  protected:
@@ -60,7 +61,6 @@ class UpdateClient : public base::RefCountedThreadSafe<UpdateClient> {
   virtual bool CanDialIPC() = 0;
 
   StatusCallback callback_;
-  base::RepeatingCallback<void(int)> registration_callback_;
 
   scoped_refptr<base::SequencedTaskRunner> callback_task_runner_;
 };

chrome/updater/test/test_app/update_client_mac.mm

@@ -20,6 +20,7 @@
 #import "chrome/updater/app/server/mac/service_protocol.h"
 #import "chrome/updater/app/server/mac/update_service_wrappers.h"
 #import "chrome/updater/mac/xpc_service_names.h"
+#include "chrome/updater/test/test_app/constants.h"
 #include "chrome/updater/test/test_app/test_app_version.h"
 
 @interface CRUUpdateClientOnDemandImpl : NSObject <CRUUpdateChecking> {
@@ -132,8 +133,7 @@ void UpdateClientMac::BeginRegister(const std::string& brand_code,
                                   static_cast<UpdateService::Result>(error)));
   };
 
-  [client_.get() registerForUpdatesWithAppId:base::SysUTF8ToNSString(
-                                                 base::mac::BaseBundleID())
+  [client_.get() registerForUpdatesWithAppId:base::SysUTF8ToNSString(kTestAppId)
                                    brandCode:base::SysUTF8ToNSString(brand_code)
                                          tag:base::SysUTF8ToNSString(tag)
                                      version:base::SysUTF8ToNSString(version)

chrome/updater/test/test_app/update_client_win.cc

@@ -184,7 +184,8 @@ void UpdateClientWin::UpdateCheckInternal(
 
   auto observer =
       Microsoft::WRL::Make<UpdaterObserver>(updater_, std::move(callback));
-  HRESULT hr = updater_->Update(kAppId, observer.Get());
+  HRESULT hr =
+      updater_->Update(base::ASCIIToUTF16(kTestAppId).c_str(), observer.Get());
   if (FAILED(hr)) {
     LOG(ERROR) << "Failed to call IUpdater::UpdateAll " << std::hex << hr;
     UpdateService::UpdateState state;