Refactoring: EmbeddedWorkerInstance::Start takes a struct of params

I want to add |is_installed| for UMA purposes, but the function is already
taking too many params. Making it a struct also cleans up
the pause_on_download default param.

Review URL: https://codereview.chromium.org/1831463003

Cr-Commit-Position: refs/heads/master@{#383234}

4 files changed, 92 insertions, 69 deletions

diff --git a/content/browser/service_worker/embedded_worker_instance.cc b/content/browser/service_worker/embedded_worker_instance.cc
index d9aeb0f..f52ae2b 100644
--- a/content/browser/service_worker/embedded_worker_instance.cc
+++ b/content/browser/service_worker/embedded_worker_instance.cc
@@ -346,11 +346,9 @@ EmbeddedWorkerInstance::~EmbeddedWorkerInstance() {
   process_handle_.reset();
 }
 
-void EmbeddedWorkerInstance::Start(int64_t service_worker_version_id,
-                                   const GURL& scope,
-                                   const GURL& script_url,
-                                   const StatusCallback& callback,
-                                   bool pause_after_download) {
+void EmbeddedWorkerInstance::Start(
+    scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params,
+    const StatusCallback& callback) {
   if (!context_) {
     callback.Run(SERVICE_WORKER_ERROR_ABORT);
     // |this| may be destroyed by the callback.
@@ -358,9 +356,7 @@ void EmbeddedWorkerInstance::Start(int64_t service_worker_version_id,
   }
   DCHECK(status_ == STOPPED);
 
-  // TODO(horo): If we will see crashes here, we have to find the root cause of
-  // the invalid version ID. Otherwise change CHECK to DCHECK.
-  CHECK_NE(service_worker_version_id, kInvalidServiceWorkerVersionId);
+  DCHECK_NE(kInvalidServiceWorkerVersionId, params->service_worker_version_id);
   start_timing_ = base::TimeTicks::Now();
   status_ = STARTING;
   starting_phase_ = ALLOCATING_PROCESS;
@@ -368,15 +364,9 @@ void EmbeddedWorkerInstance::Start(int64_t service_worker_version_id,
   service_registry_.reset(new ServiceRegistryImpl());
   FOR_EACH_OBSERVER(Listener, listener_list_, OnStarting());
 
-  scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
-      new EmbeddedWorkerMsg_StartWorker_Params());
   params->embedded_worker_id = embedded_worker_id_;
-  params->service_worker_version_id = service_worker_version_id;
-  params->scope = scope;
-  params->script_url = script_url;
   params->worker_devtools_agent_route_id = MSG_ROUTING_NONE;
   params->wait_for_debugger = false;
-  params->pause_after_download = pause_after_download;
   params->settings.v8_cache_options = GetV8CacheOptions();
 
   inflight_start_task_.reset(new StartTask(this));
diff --git a/content/browser/service_worker/embedded_worker_instance.h b/content/browser/service_worker/embedded_worker_instance.h
index 2bcbaab..87aac2b 100644
--- a/content/browser/service_worker/embedded_worker_instance.h
+++ b/content/browser/service_worker/embedded_worker_instance.h
@@ -107,14 +107,11 @@ class CONTENT_EXPORT EmbeddedWorkerInstance {
 
   // Starts the worker. It is invalid to call this when the worker is not in
   // STOPPED status. |callback| is invoked after the worker script has been
-  // started and evaluated, or when an error occurs. If |pause_after_download|
-  // is true, the worker pauses after loading until ResumeAfterDownload() is
-  // called.
-  void Start(int64_t service_worker_version_id,
-             const GURL& scope,
-             const GURL& script_url,
-             const StatusCallback& callback,
-             bool pause_after_download = false);
+  // started and evaluated, or when an error occurs.
+  // |params| should be populated with service worker version info needed
+  // to start the worker.
+  void Start(scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params,
+             const StatusCallback& callback);
 
   // Stops the worker. It is invalid to call this when the worker is
   // not in STARTING or RUNNING status.
diff --git a/content/browser/service_worker/embedded_worker_instance_unittest.cc b/content/browser/service_worker/embedded_worker_instance_unittest.cc
index 67e7f26..1db995a 100644
--- a/content/browser/service_worker/embedded_worker_instance_unittest.cc
+++ b/content/browser/service_worker/embedded_worker_instance_unittest.cc
@@ -32,6 +32,17 @@ void SaveStatusAndCall(ServiceWorkerStatusCode* out,
   callback.Run();
 }
 
+scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params>
+CreateStartParams(int version_id, const GURL& scope, const GURL& script_url) {
+  scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+      new EmbeddedWorkerMsg_StartWorker_Params);
+  params->service_worker_version_id = version_id;
+  params->scope = scope;
+  params->script_url = script_url;
+  params->pause_after_download = false;
+  return params;
+}
+
 }  // namespace
 
 class EmbeddedWorkerInstanceTest : public testing::Test,
@@ -85,8 +96,10 @@ class EmbeddedWorkerInstanceTest : public testing::Test,
                                       const GURL& url) {
     ServiceWorkerStatusCode status;
     base::RunLoop run_loop;
-    worker->Start(id, pattern, url, base::Bind(&SaveStatusAndCall, &status,
-                                               run_loop.QuitClosure()));
+    scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params =
+        CreateStartParams(id, pattern, url);
+    worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                                run_loop.QuitClosure()));
     run_loop.Run();
     return status;
   }
@@ -164,11 +177,10 @@ TEST_F(EmbeddedWorkerInstanceTest, StartAndStop) {
   // Start should succeed.
   ServiceWorkerStatusCode status;
   base::RunLoop run_loop;
-  worker->Start(
-      service_worker_version_id,
-      pattern,
-      url,
-      base::Bind(&SaveStatusAndCall, &status, run_loop.QuitClosure()));
+  scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params =
+      CreateStartParams(service_worker_version_id, pattern, url);
+  worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                              run_loop.QuitClosure()));
   EXPECT_EQ(EmbeddedWorkerInstance::STARTING, worker->status());
   run_loop.Run();
   EXPECT_EQ(SERVICE_WORKER_OK, status);
@@ -220,9 +232,10 @@ TEST_F(EmbeddedWorkerInstanceTest, ForceNewProcess) {
     // Start once normally.
     ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_MAX_VALUE;
     base::RunLoop run_loop;
-    worker->Start(
-        service_worker_version_id, pattern, url,
-        base::Bind(&SaveStatusAndCall, &status, run_loop.QuitClosure()));
+    scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+        CreateStartParams(service_worker_version_id, pattern, url));
+    worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                                run_loop.QuitClosure()));
     run_loop.Run();
     EXPECT_EQ(SERVICE_WORKER_OK, status);
     EXPECT_EQ(EmbeddedWorkerInstance::RUNNING, worker->status());
@@ -243,9 +256,10 @@ TEST_F(EmbeddedWorkerInstanceTest, ForceNewProcess) {
     // Start again.
     ServiceWorkerStatusCode status;
     base::RunLoop run_loop;
-    worker->Start(
-        service_worker_version_id, pattern, url,
-        base::Bind(&SaveStatusAndCall, &status, run_loop.QuitClosure()));
+    scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+        CreateStartParams(service_worker_version_id, pattern, url));
+    worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                                run_loop.QuitClosure()));
     EXPECT_EQ(EmbeddedWorkerInstance::STARTING, worker->status());
     run_loop.Run();
     EXPECT_EQ(SERVICE_WORKER_OK, status);
@@ -322,9 +336,10 @@ TEST_F(EmbeddedWorkerInstanceTest, RemoveWorkerInSharedProcess) {
     // Start worker1.
     ServiceWorkerStatusCode status;
     base::RunLoop run_loop;
-    worker1->Start(
-        version_id1, pattern, url,
-        base::Bind(&SaveStatusAndCall, &status, run_loop.QuitClosure()));
+    scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+        CreateStartParams(version_id1, pattern, url));
+    worker1->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                                 run_loop.QuitClosure()));
     run_loop.Run();
     EXPECT_EQ(SERVICE_WORKER_OK, status);
   }
@@ -333,9 +348,10 @@ TEST_F(EmbeddedWorkerInstanceTest, RemoveWorkerInSharedProcess) {
     // Start worker2.
     ServiceWorkerStatusCode status;
     base::RunLoop run_loop;
-    worker2->Start(
-        version_id2, pattern, url,
-        base::Bind(&SaveStatusAndCall, &status, run_loop.QuitClosure()));
+    scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+        CreateStartParams(version_id2, pattern, url));
+    worker2->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                                 run_loop.QuitClosure()));
     run_loop.Run();
     EXPECT_EQ(SERVICE_WORKER_OK, status);
   }
@@ -369,9 +385,10 @@ TEST_F(EmbeddedWorkerInstanceTest, DetachDuringProcessAllocation) {
 
   // Run the start worker sequence and detach during process allocation.
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_MAX_VALUE;
-  worker->Start(
-      version_id, scope, url,
-      base::Bind(&SaveStatusAndCall, &status, base::Bind(&base::DoNothing)));
+  scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+      CreateStartParams(version_id, scope, url));
+  worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                              base::Bind(&base::DoNothing)));
   worker->Detach();
   base::RunLoop().RunUntilIdle();
 
@@ -400,9 +417,10 @@ TEST_F(EmbeddedWorkerInstanceTest, DetachAfterSendingStartWorkerMessage) {
 
   // Run the start worker sequence until a start worker message is sent.
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_MAX_VALUE;
-  worker->Start(
-      version_id, scope, url,
-      base::Bind(&SaveStatusAndCall, &status, base::Bind(base::DoNothing)));
+  scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+      CreateStartParams(version_id, scope, url));
+  worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                              base::Bind(&base::DoNothing)));
   base::RunLoop().RunUntilIdle();
 
   ASSERT_EQ(2u, events_.size());
@@ -437,9 +455,11 @@ TEST_F(EmbeddedWorkerInstanceTest, StopDuringProcessAllocation) {
 
   // Stop the start worker sequence before a process is allocated.
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_MAX_VALUE;
-  worker->Start(
-      version_id, scope, url,
-      base::Bind(&SaveStatusAndCall, &status, base::Bind(base::DoNothing)));
+
+  scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+      CreateStartParams(version_id, scope, url));
+  worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                              base::Bind(&base::DoNothing)));
   worker->Stop();
   base::RunLoop().RunUntilIdle();
 
@@ -459,8 +479,9 @@ TEST_F(EmbeddedWorkerInstanceTest, StopDuringProcessAllocation) {
   // Restart the worker.
   status = SERVICE_WORKER_ERROR_MAX_VALUE;
   scoped_ptr<base::RunLoop> run_loop(new base::RunLoop);
-  worker->Start(version_id, scope, url, base::Bind(&SaveStatusAndCall, &status,
-                                                   run_loop->QuitClosure()));
+  params = CreateStartParams(version_id, scope, url);
+  worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                              run_loop->QuitClosure()));
   run_loop->Run();
 
   EXPECT_EQ(SERVICE_WORKER_OK, status);
@@ -484,9 +505,12 @@ TEST_F(EmbeddedWorkerInstanceTest, StopDuringPausedAfterDownload) {
 
   // Run the start worker sequence until pause after download.
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_MAX_VALUE;
-  worker->Start(version_id, scope, url, base::Bind(&SaveStatusAndCall, &status,
-                                                   base::Bind(base::DoNothing)),
-                true /* pause_after_download */);
+
+  scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+      CreateStartParams(version_id, scope, url));
+  params->pause_after_download = true;
+  worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                              base::Bind(&base::DoNothing)));
   base::RunLoop().RunUntilIdle();
 
   // Make the worker stopping and attempt to send a resume after download
@@ -513,9 +537,10 @@ TEST_F(EmbeddedWorkerInstanceTest, StopAfterSendingStartWorkerMessage) {
 
   // Run the start worker sequence until a start worker message is sent.
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_MAX_VALUE;
-  worker->Start(
-      version_id, scope, url,
-      base::Bind(&SaveStatusAndCall, &status, base::Bind(base::DoNothing)));
+  scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+      CreateStartParams(version_id, scope, url));
+  worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                              base::Bind(&base::DoNothing)));
   base::RunLoop().RunUntilIdle();
 
   ASSERT_EQ(2u, events_.size());
@@ -544,8 +569,10 @@ TEST_F(EmbeddedWorkerInstanceTest, StopAfterSendingStartWorkerMessage) {
       ->set_force_stall_in_start(false);
   status = SERVICE_WORKER_ERROR_MAX_VALUE;
   scoped_ptr<base::RunLoop> run_loop(new base::RunLoop);
-  worker->Start(version_id, scope, url, base::Bind(&SaveStatusAndCall, &status,
-                                                   run_loop->QuitClosure()));
+
+  params = CreateStartParams(version_id, scope, url);
+  worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                              run_loop->QuitClosure()));
   run_loop->Run();
 
   // The worker should be started.
@@ -572,9 +599,10 @@ TEST_F(EmbeddedWorkerInstanceTest, Detach) {
 
   // Start the worker.
   base::RunLoop run_loop;
-  worker->Start(
-      version_id, pattern, url,
-      base::Bind(&SaveStatusAndCall, &status, run_loop.QuitClosure()));
+  scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+      CreateStartParams(version_id, pattern, url));
+  worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                              run_loop.QuitClosure()));
   run_loop.Run();
 
   // Detach.
@@ -606,9 +634,10 @@ TEST_F(EmbeddedWorkerInstanceTest, FailToSendStartIPC) {
 
   // Attempt to start the worker.
   base::RunLoop run_loop;
-  worker->Start(
-      version_id, pattern, url,
-      base::Bind(&SaveStatusAndCall, &status, run_loop.QuitClosure()));
+  scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+      CreateStartParams(version_id, pattern, url));
+  worker->Start(std::move(params), base::Bind(&SaveStatusAndCall, &status,
+                                              run_loop.QuitClosure()));
   run_loop.Run();
 
   // The callback should have run, and we should have got an OnStopped message.
diff --git a/content/browser/service_worker/service_worker_version.cc b/content/browser/service_worker/service_worker_version.cc
index 8302403..41987d0 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -33,6 +33,7 @@
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_metrics.h"
 #include "content/browser/service_worker/service_worker_registration.h"
+#include "content/common/service_worker/embedded_worker_messages.h"
 #include "content/common/service_worker/service_worker_messages.h"
 #include "content/common/service_worker/service_worker_type_converters.h"
 #include "content/common/service_worker/service_worker_utils.h"
@@ -1376,12 +1377,18 @@ void ServiceWorkerVersion::StartWorkerInternal() {
 
   StartTimeoutTimer();
 
+  scoped_ptr<EmbeddedWorkerMsg_StartWorker_Params> params(
+      new EmbeddedWorkerMsg_StartWorker_Params());
+  params->service_worker_version_id = version_id_;
+  params->scope = scope_;
+  params->script_url = script_url_;
   DCHECK(!pause_after_download_ || !IsInstalled(status()));
+  params->pause_after_download = pause_after_download_;
+
   embedded_worker_->Start(
-      version_id_, scope_, script_url_,
+      std::move(params),
       base::Bind(&ServiceWorkerVersion::OnStartSentAndScriptEvaluated,
-                 weak_factory_.GetWeakPtr()),
-      pause_after_download_);
+                 weak_factory_.GetWeakPtr()));
 }
 
 void ServiceWorkerVersion::StartTimeoutTimer() {