Mojofy unittests: ServiceWorkerJobTest

Mojofy unittests: ServiceWorkerJobTest

This patch mojofies ServiceWorkerJobTest by using MojoServiceWorkerTestP, and
also this patch fixes error handling in EmbeddedWorkerInstance.

BUG=629701
TEST=./out/Debug/content_unittests -f --gtest_filter="ServiceWorkerJobTest/ServiceWorkerJobTestP*"

Committed: https://crrev.com/5578ab3ea785cb01e7b5f52b89eabea442af2ddd
Cr-Commit-Position: refs/heads/master@{#426172}

[shimazu, 2016-10-18 09:46:34]

shimazu@chromium.org changed reviewers:
+ horo@chromium.org

[shimazu, 2016-10-18 09:46:35]

ptal:)

[horo, 2016-10-18 11:03:53]

lgtm with nits

https://codereview.chromium.org/2429753003/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2429753003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_job_unittest.cc:11: #include
"base/run_loop.h"
#include "base/optional.h"

https://codereview.chromium.org/2429753003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_job_unittest.cc:1619: int
num_of_startworker() { return num_of_startworker_; }
nit: const

https://codereview.chromium.org/2429753003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_job_unittest.cc:1636: };
nit:
DISALLOW_COPY_AND_ASSIGN(CheckPauseAfterDownloadEmbeddedWorkerInstanceClient);

[shimazu, 2016-10-18 11:58:07]

Thanks, updated.

https://codereview.chromium.org/2429753003/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2429753003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_job_unittest.cc:11: #include
"base/run_loop.h"
On 2016/10/18 11:03:53, horo wrote:
> #include "base/optional.h"

Done.

https://codereview.chromium.org/2429753003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_job_unittest.cc:1619: int
num_of_startworker() { return num_of_startworker_; }
On 2016/10/18 11:03:53, horo wrote:
> nit: const

Done.

https://codereview.chromium.org/2429753003/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_job_unittest.cc:1636: };
On 2016/10/18 11:03:53, horo wrote:
> nit:
> DISALLOW_COPY_AND_ASSIGN(CheckPauseAfterDownloadEmbeddedWorkerInstanceClient);

Done.

[shimazu, 2016-10-18 11:58:14]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2016-10-18 11:58:15]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2429753003/#ps20001
(title: "Updated after review comments")

[commit-bot: I haz the power, 2016-10-18 11:58:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 13:05:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 13:05:57]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shimazu, 2016-10-18 15:56:33]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2016-10-18 15:56:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 16:58:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 16:58:11]

Failed to apply patch for
content/browser/service_worker/embedded_worker_instance.cc:
While running git apply --index -3 -p1;
  error: patch failed:
content/browser/service_worker/embedded_worker_instance.cc:601
  Falling back to three-way merge...
  Applied patch to 'content/browser/service_worker/embedded_worker_instance.cc'
with conflicts.
  U content/browser/service_worker/embedded_worker_instance.cc

Patch:       content/browser/service_worker/embedded_worker_instance.cc
Index: content/browser/service_worker/embedded_worker_instance.cc
diff --git a/content/browser/service_worker/embedded_worker_instance.cc
b/content/browser/service_worker/embedded_worker_instance.cc
index
338d49063a61a55fc58f070dc6752c8658b6b77d..8f6593fa1b13fcaf1802a2f9757663d2fe3b1d97
100644
--- a/content/browser/service_worker/embedded_worker_instance.cc
+++ b/content/browser/service_worker/embedded_worker_instance.cc
@@ -375,7 +375,7 @@ class EmbeddedWorkerInstance::StartTask {
     params->wait_for_debugger = wait_for_debugger;
 
     if (ServiceWorkerUtils::IsMojoForServiceWorkerEnabled())
-      instance_->SendMojoStartWorker(std::move(params));
+      SendMojoStartWorker(std::move(params));
     else
       SendStartWorker(std::move(params));
   }
@@ -400,6 +400,18 @@ class EmbeddedWorkerInstance::StartTask {
     // is evaluated.
   }
 
+  void SendMojoStartWorker(std::unique_ptr<EmbeddedWorkerStartParams> params) {
+    ServiceWorkerStatusCode status =
+        instance_->SendMojoStartWorker(std::move(params));
+    if (status != SERVICE_WORKER_OK) {
+      StatusCallback callback = start_callback_;
+      start_callback_.Reset();
+      instance_->OnStartFailed(callback, status);
+      // |this| may be destroyed.
+      return;
+    }
+  }
+
   // |instance_| must outlive |this|.
   EmbeddedWorkerInstance* instance_;
 
@@ -601,13 +613,16 @@ void
EmbeddedWorkerInstance::OnRegisteredToDevToolsManager(
     observer.OnRegisteredToDevToolsManager();
 }
 
-void EmbeddedWorkerInstance::SendMojoStartWorker(
+ServiceWorkerStatusCode EmbeddedWorkerInstance::SendMojoStartWorker(
     std::unique_ptr<EmbeddedWorkerStartParams> params) {
+  if (!context_)
+    return SERVICE_WORKER_ERROR_ABORT;
   client_->StartWorker(*params);
   registry_->BindWorkerToProcess(process_id(), embedded_worker_id());
   TRACE_EVENT_ASYNC_STEP_PAST1("ServiceWorker",
"EmbeddedWorkerInstance::Start",
                                this, "SendStartWorker", "Status", "mojo");
   OnStartWorkerMessageSent();
+  return SERVICE_WORKER_OK;
 }
 
 void EmbeddedWorkerInstance::OnStartWorkerMessageSent() {

[shimazu, 2016-10-19 01:52:03]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2016-10-19 01:52:04]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2429753003/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-19 01:52:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2016-10-19 04:37:20]

The CQ bit was unchecked by horo@chromium.org

[horo, 2016-10-19 04:39:01]

https://codereview.chromium.org/2429753003/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2429753003/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:1648:
std::vector<CheckPauseAfterDownloadEmbeddedWorkerInstanceClient*> clients = {
Sorry for the late comment.
You don't need to create the mock instance client when is_mojo_enabled() is
false.

[shimazu, 2016-10-19 05:32:54]

Thanks, updated.
ptal again:)

https://codereview.chromium.org/2429753003/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_job_unittest.cc (right):

https://codereview.chromium.org/2429753003/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_job_unittest.cc:1648:
std::vector<CheckPauseAfterDownloadEmbeddedWorkerInstanceClient*> clients = {
On 2016/10/19 04:39:01, horo wrote:
> Sorry for the late comment.
> You don't need to create the mock instance client when is_mojo_enabled() is
> false.

Done.

[horo, 2016-10-19 06:19:30]

lgtm

Thank you.

[shimazu, 2016-10-19 07:46:11]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2016-10-19 09:31:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 11:43:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 11:44:00]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[shimazu, 2016-10-19 12:47:56]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2016-10-19 12:48:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 13:35:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-21 13:07:45]

Description was changed from

==========
Mojofy unittests: ServiceWorkerJobTest

This patch mojofies ServiceWorkerJobTest by using MojoServiceWorkerTestP, and
also this patch fixes error handling in EmbeddedWorkerInstance.

BUG=629701
TEST=./out/Debug/content_unittests -f
--gtest_filter="ServiceWorkerJobTest/ServiceWorkerJobTestP*"
==========

to

==========
Mojofy unittests: ServiceWorkerJobTest

This patch mojofies ServiceWorkerJobTest by using MojoServiceWorkerTestP, and
also this patch fixes error handling in EmbeddedWorkerInstance.

BUG=629701
TEST=./out/Debug/content_unittests -f
--gtest_filter="ServiceWorkerJobTest/ServiceWorkerJobTestP*"

Committed: https://crrev.com/5578ab3ea785cb01e7b5f52b89eabea442af2ddd
Cr-Commit-Position: refs/heads/master@{#426172}
==========

[commit-bot: I haz the power, 2016-10-21 13:07:46]

Patchset 4 (id:??) landed as
https://crrev.com/5578ab3ea785cb01e7b5f52b89eabea442af2ddd
Cr-Commit-Position: refs/heads/master@{#426172}