ServiceWorker: remove EmbeddedWorkerMsg_StartWorker

ServiceWorker: remove EmbeddedWorkerMsg_StartWorker

A clean up patch after --disable-mojo-service-worker flag has been removed.
This patch removes the StartWorker message and also EmbeddedWorkerSetup
interface which is used only for setting up the mojo interfaces when StartWorker
message has been sent via the legacy IPC.

BUG=629701
Review-Url: https://codereview.chromium.org/2637743002
Cr-Commit-Position: refs/heads/master@{#444253}
Committed: https://chromium.googlesource.com/chromium/src/+/271c0525253b60dba0d09d8142cf3706217bea6f

[shimazu, 2017-01-16 02:07:22]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 02:07:35]

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

[shimazu, 2017-01-16 02:10:11]

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

[shimazu, 2017-01-16 02:10:12]

ptal

[commit-bot: I haz the power, 2017-01-16 02:20:00]

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

[commit-bot: I haz the power, 2017-01-16 02:20:01]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[shimazu, 2017-01-16 02:30:24]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 02:30:37]

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

[shimazu, 2017-01-16 03:08:57]

Description was changed from

==========
ServiceWorker: remove EmbeddedWorkerMsg_StartWorker

A clean up patch after --disable-mojo-service-worker flag is removed.
This patch also removes EmbeddedWorkerSetup which is an interface used only for
setting up the mojo interfaces when StartWorker message has been sent via the
legacy IPC.

BUG=629701
==========

to

==========
ServiceWorker: remove EmbeddedWorkerMsg_StartWorker

A clean up patch after --disable-mojo-service-worker flag has been removed.
This patch also removes EmbeddedWorkerSetup which is an interface used only for
setting up the mojo interfaces when StartWorker message has been sent via the
legacy IPC.

BUG=629701
==========

[shimazu, 2017-01-16 03:10:00]

Description was changed from

==========
ServiceWorker: remove EmbeddedWorkerMsg_StartWorker

A clean up patch after --disable-mojo-service-worker flag has been removed.
This patch also removes EmbeddedWorkerSetup which is an interface used only for
setting up the mojo interfaces when StartWorker message has been sent via the
legacy IPC.

BUG=629701
==========

to

==========
ServiceWorker: remove EmbeddedWorkerMsg_StartWorker

A clean up patch after --disable-mojo-service-worker flag has been removed.
This patch removes the StartWorker message and also EmbeddedWorkerSetupalso
which is used only for
setting up the mojo interfaces when StartWorker message has been sent via the
legacy IPC.

BUG=629701
==========

[shimazu, 2017-01-16 03:10:24]

Description was changed from

==========
ServiceWorker: remove EmbeddedWorkerMsg_StartWorker

A clean up patch after --disable-mojo-service-worker flag has been removed.
This patch removes the StartWorker message and also EmbeddedWorkerSetupalso
which is used only for
setting up the mojo interfaces when StartWorker message has been sent via the
legacy IPC.

BUG=629701
==========

to

==========
ServiceWorker: remove EmbeddedWorkerMsg_StartWorker

A clean up patch after --disable-mojo-service-worker flag has been removed.
This patch removes the StartWorker message and also EmbeddedWorkerSetup
interface which is used only for
setting up the mojo interfaces when StartWorker message has been sent via the
legacy IPC.

BUG=629701
==========

[shimazu, 2017-01-16 03:12:29]

Description was changed from

==========
ServiceWorker: remove EmbeddedWorkerMsg_StartWorker

A clean up patch after --disable-mojo-service-worker flag has been removed.
This patch removes the StartWorker message and also EmbeddedWorkerSetup
interface which is used only for
setting up the mojo interfaces when StartWorker message has been sent via the
legacy IPC.

BUG=629701
==========

to

==========
ServiceWorker: remove EmbeddedWorkerMsg_StartWorker

A clean up patch after --disable-mojo-service-worker flag has been removed.
This patch removes the StartWorker message and also EmbeddedWorkerSetup
interface which is used only for setting up the mojo interfaces when StartWorker
message has been sent via the legacy IPC.

BUG=629701
==========

[commit-bot: I haz the power, 2017-01-16 03:51:35]

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

[commit-bot: I haz the power, 2017-01-16 03:51:36]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[horo, 2017-01-16 04:56:31]

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:22: #include
"content/common/service_worker/embedded_worker_setup.mojom.h"
remove

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:23: #include
"content/common/service_worker/embedded_worker_setup.mojom.h"
remove

https://codereview.chromium.org/2637743002/diff/20001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2637743002/diff/20001/content/renderer/render...
content/renderer/render_thread_impl.cc:77: #include
"content/common/service_worker/embedded_worker_setup.mojom.h"
remove

[falken, 2017-01-16 05:07:44]

falken@chromium.org changed reviewers:
+ falken@chromium.org

[falken, 2017-01-16 05:07:45]

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:390:
SendStartWorker(std::move(params));
Can we collapse SendStartWorker into this function now?

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:401: return;
This return seems unneeded.

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:466: request =
mojo::MakeRequest(&client_);
Can we initialize request in line 465?

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_registry.cc (left):

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_registry.cc:270: }
nice

[shimazu, 2017-01-16 05:49:11]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 05:49:27]

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

[shimazu, 2017-01-16 05:49:54]

Updated the path. PTAL again :)

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:22: #include
"content/common/service_worker/embedded_worker_setup.mojom.h"
On 2017/01/16 04:56:31, horo wrote:
> remove

Done.

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:390:
SendStartWorker(std::move(params));
On 2017/01/16 05:07:45, falken wrote:
> Can we collapse SendStartWorker into this function now?

Done.

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:401: return;
On 2017/01/16 05:07:45, falken wrote:
> This return seems unneeded.

Done.

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:466: request =
mojo::MakeRequest(&client_);
On 2017/01/16 05:07:45, falken wrote:
> Can we initialize request in line 465?

Yes, that's right. Done.

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_registry.cc (left):

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_registry.cc:270: }
On 2017/01/16 05:07:45, falken wrote:
> nice

yey!:)

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2637743002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:23: #include
"content/common/service_worker/embedded_worker_setup.mojom.h"
On 2017/01/16 04:56:31, horo wrote:
> remove

Done.

https://codereview.chromium.org/2637743002/diff/20001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2637743002/diff/20001/content/renderer/render...
content/renderer/render_thread_impl.cc:77: #include
"content/common/service_worker/embedded_worker_setup.mojom.h"
On 2017/01/16 04:56:31, horo wrote:
> remove

Done.

[falken, 2017-01-16 06:02:51]

lg to me after some more nits but let's wait for horo@ too.

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

https://codereview.chromium.org/2637743002/diff/40001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:601: this,
"SendStartWorker", "Status", "mojo");
Status "mojo" seems superfluous now. Should we log something more interesting
like the script URL? (See
https://bugs.chromium.org/p/chromium/issues/detail?id=669772)

https://codereview.chromium.org/2637743002/diff/40001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2637743002/diff/40001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.h:310:
mojom::ServiceWorkerEventDispatcherRequest pending_dispatcher_request_;
Can this be removed now? If not, the TODO needs updating.

https://codereview.chromium.org/2637743002/diff/40001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_dispatcher.h (left):

https://codereview.chromium.org/2637743002/diff/40001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_dispatcher.h:72:
std::unique_ptr<WorkerWrapper> StartWorkerContext(
Does comment 68-71 still apply to this method?

[horo, 2017-01-16 06:05:06]

lgtm

[shimazu, 2017-01-16 07:04:48]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 07:05:02]

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

[shimazu, 2017-01-16 07:06:37]

shimazu@chromium.org changed reviewers:
+ avi@chromium.org, dcheng@chromium.org

[shimazu, 2017-01-16 07:06:38]

avi@: Could you review changes in render_thread_impl.cc and BUILD.gn?
dcheng@: Could you review changes in embedded_worker_setup.mojom and
embedded_worker_messages.h?

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

https://codereview.chromium.org/2637743002/diff/40001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:601: this,
"SendStartWorker", "Status", "mojo");
On 2017/01/16 06:02:51, falken wrote:
> Status "mojo" seems superfluous now. Should we log something more interesting
> like the script URL? (See
> https://bugs.chromium.org/p/chromium/issues/detail?id=669772)

The script URL is already recorded at the ctor of StartTask, so let's remove the
parameter.

https://codereview.chromium.org/2637743002/diff/40001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2637743002/diff/40001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.h:310:
mojom::ServiceWorkerEventDispatcherRequest pending_dispatcher_request_;
On 2017/01/16 06:02:51, falken wrote:
> Can this be removed now? If not, the TODO needs updating.

It will be a bit big patch because currently EmbeddedWorkerStartParams is still
on embedded_worker_messages.h instead of typemap for mojo, and the request
cannot be included in the EWStartParams. 
Updated the comment.

https://codereview.chromium.org/2637743002/diff/40001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_dispatcher.h (left):

https://codereview.chromium.org/2637743002/diff/40001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_dispatcher.h:72:
std::unique_ptr<WorkerWrapper> StartWorkerContext(
On 2017/01/16 06:02:51, falken wrote:
> Does comment 68-71 still apply to this method?

No, it doesn't. 
StartWorkerContext can be alread moved to EWInstanceClient, but I'll do it when
removing EWDispatcher after mojofying two remaining messages.

For now let's move the comment after this method.

[commit-bot: I haz the power, 2017-01-16 08:39:24]

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

[commit-bot: I haz the power, 2017-01-16 08:39:25]

Dry run: This issue passed the CQ dry run.

[shimazu, 2017-01-16 08:46:07]

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

[shimazu, 2017-01-16 08:46:08]

oops, avi seems ooo for a while.

clamy@: could you review changes in BUILD.gn and render_thread_impl.cc?

[clamy, 2017-01-16 12:38:32]

Thanks! Lgtm.

[dcheng, 2017-01-17 22:07:42]

ipc and mojo changes lgtm

[shimazu, 2017-01-18 00:09:57]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-01-18 00:09:57]

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/2637743002/#ps60001
(title: "Addressed falken's comment")

[commit-bot: I haz the power, 2017-01-18 00:10:37]

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, 2017-01-18 00:13:22]

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

[commit-bot: I haz the power, 2017-01-18 00:13:23]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[shimazu, 2017-01-18 01:18:06]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-01-18 01:18:06]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
dcheng@chromium.org, horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2637743002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-18 01:18:35]

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, 2017-01-18 02:57:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484702286109960,
"parent_rev": "441df0543f6f4029f73068dec974d4dac47f6a1f", "commit_rev":
"271c0525253b60dba0d09d8142cf3706217bea6f"}

[commit-bot: I haz the power, 2017-01-18 02:58:13]

Description was changed from

==========
ServiceWorker: remove EmbeddedWorkerMsg_StartWorker

A clean up patch after --disable-mojo-service-worker flag has been removed.
This patch removes the StartWorker message and also EmbeddedWorkerSetup
interface which is used only for setting up the mojo interfaces when StartWorker
message has been sent via the legacy IPC.

BUG=629701
==========

to

==========
ServiceWorker: remove EmbeddedWorkerMsg_StartWorker

A clean up patch after --disable-mojo-service-worker flag has been removed.
This patch removes the StartWorker message and also EmbeddedWorkerSetup
interface which is used only for setting up the mojo interfaces when StartWorker
message has been sent via the legacy IPC.

BUG=629701

Review-Url: https://codereview.chromium.org/2637743002
Cr-Commit-Position: refs/heads/master@{#444253}
Committed:
https://chromium.googlesource.com/chromium/src/+/271c0525253b60dba0d09d8142cf...
==========

[commit-bot: I haz the power, 2017-01-18 02:58:17]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/271c0525253b60dba0d09d8142cf...