ServiceWorker: Implement StartWorker by using mojo

ServiceWorker: Implement StartWorker by using mojo

This patch is the first step of mojofication on the service worker.
In this patch, EWInstance will send StartWorker IPC using mojo. The endpoint on
the renderer process is EWInstanceClient which is corresponding to one
EWInstance. In future, this enables us to remove EWDispatcher, but
EWInstanceClient has reference to EWDispatcher for now.

BUG=629701
Committed: https://crrev.com/5db8eb672a52385059d4d188e27d7a7f0a5dd29d
Cr-Commit-Position: refs/heads/master@{#421427}

[shimazu, 2016-08-23 08:36:59]

Description was changed from

==========
[WIP] ServiceWorker: Start to implement StartWorker by using mojo

This patch is the first step of mojofication on the service worker.
In this patch, EmbeddedWorkerRegistry will send StartWorker using mojo
(currently it's under construction)

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

to

==========
ServiceWorker: Implement StartWorker by using mojo

This patch is the first step of mojofication on the service worker.
In this patch, EWInstance will send StartWorker IPC using mojo. The endpoint on
the renderer process is EWInstanceClient which is corresponding to one
EWInstance. In future, this enables us to remove EWDispatcher, but
EWInstanceClient has reference to EWDispatcher for now.

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

[shimazu, 2016-08-23 08:40:12]

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

[shimazu, 2016-08-23 08:40:13]

Could you take a look at this CL as is?

I'm going to add unitttests for mojo later, but I'd like to hear your comments
of this structure.
You can try this by adding '--mojo-service-worker' flag.

[horo, 2016-08-23 10:25:06]

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

[commit-bot: I haz the power, 2016-08-23 10:25:24]

Dry run: 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-08-23 11:27:01]

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

[commit-bot: I haz the power, 2016-08-23 11:27:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Marijn Kruisselbrink, 2016-08-23 18:56:08]

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

[Marijn Kruisselbrink, 2016-08-23 18:56:09]

Awesome, I once tried to do something similar myself
(https://codereview.chromium.org/1223193009), just to see how complicated it
would be, but eventually gave up. Not because using mojo for Start/StopWorker
(in my patch I just used the mojo connection disconnecting as a replacement for
the StopWorker IPC) was hard, but because updating all the unit tests was
terrible. Maybe things are a bit better these days, but good luck with all that
anyway (at least with the command line flag approach you should be able to do it
in chunks rather than all at once).

https://codereview.chromium.org/2227593002/diff/140001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2227593002/diff/140001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:130:
rph->GetRemoteInterfaces()->GetInterface(&instance_client);
Rather than creating and binding the InterfacePtr here, and then rebinding it on
the IO thread (and having extra thread hops to get back on the IO thread), you
could just have StartTask create the InterfaceRequest for the InterfacePtr, pass
the InterfaceRequest to this method, and then immediately synchronously do what
OnGotRemoteEmbeddedWorkerInstanceClient does in StartTask (as as soon as you
created the InterfaceRequest you can start calling methods on the InterfacePtr).

[horo, 2016-08-24 04:59:16]

https://codereview.chromium.org/2227593002/diff/140001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2227593002/diff/140001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:623:
static_cast<mojom::V8CacheOptions>(params->settings.v8_cache_options);
We should have static_assert() for mojo::V8CacheOptions and
content::V8CacheOptions.

https://codereview.chromium.org/2227593002/diff/140001/content/public/common/...
File content/public/common/BUILD.gn (right):

https://codereview.chromium.org/2227593002/diff/140001/content/public/common/...
content/public/common/BUILD.gn:175: # This interface is internal to content.
However, this is not exported from
Do we need this comment after "However, ..."?

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.cc:856:
base::Unretained(embedded_worker_dispatcher_.get())));
I don't know well about the life cycle of InterfaceRegistry, but is it safe to
pass the unretained embedded_worker_dispatcher_?
If it is safe, please write comments about it.

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_dispatcher.h (right):

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_dispatcher.h:17: #include
"content/renderer/service_worker/embedded_worker_devtools_agent.h"
You don't need to include this header file.

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_dispatcher.h:30: 
class EmbeddedWorkerDevToolsAgent;

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_instance_client_impl.cc
(right):

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:49:
start_data.v8CacheOptions = static_cast<blink::WebSettings::V8CacheOptions>(
We should have static_assert() for mojo::V8CacheOptions and
blink::WebSettings::V8CacheOptions.

[shimazu, 2016-08-24 08:44:59]

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

[commit-bot: I haz the power, 2016-08-24 08:45:16]

Dry run: 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-08-24 09:45:58]

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

[commit-bot: I haz the power, 2016-08-24 09:45:59]

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

[shimazu, 2016-08-25 05:10:42]

Patchset #10 (id:180001) has been deleted

[shimazu, 2016-08-25 05:13:19]

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

[commit-bot: I haz the power, 2016-08-25 05:13:47]

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

[shimazu, 2016-08-25 05:14:53]

Thanks for your comments! :)
Added a unit test and revised this CL. ptal again.

https://codereview.chromium.org/2227593002/diff/140001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2227593002/diff/140001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:130:
rph->GetRemoteInterfaces()->GetInterface(&instance_client);
On 2016/08/23 18:56:08, Marijn Kruisselbrink wrote:
> Rather than creating and binding the InterfacePtr here, and then rebinding it
on
> the IO thread (and having extra thread hops to get back on the IO thread), you
> could just have StartTask create the InterfaceRequest for the InterfacePtr,
pass
> the InterfaceRequest to this method, and then immediately synchronously do
what
> OnGotRemoteEmbeddedWorkerInstanceClient does in StartTask (as as soon as you
> created the InterfaceRequest you can start calling methods on the
InterfacePtr).

Thanks, it seems better. Updated.

https://codereview.chromium.org/2227593002/diff/140001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:623:
static_cast<mojom::V8CacheOptions>(params->settings.v8_cache_options);
On 2016/08/24 04:59:16, horo wrote:
> We should have static_assert() for mojo::V8CacheOptions and
> content::V8CacheOptions.

Done.

https://codereview.chromium.org/2227593002/diff/140001/content/public/common/...
File content/public/common/BUILD.gn (right):

https://codereview.chromium.org/2227593002/diff/140001/content/public/common/...
content/public/common/BUILD.gn:175: # This interface is internal to content.
However, this is not exported from
On 2016/08/24 04:59:16, horo wrote:
> Do we need this comment after "However, ..."?

Oops, this comment is probably copied from somewhere.
Actually this interface is not internal (because this is in 'public'), so
removed this comment and visibility.

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_dispatcher.h (right):

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_dispatcher.h:17: #include
"content/renderer/service_worker/embedded_worker_devtools_agent.h"
On 2016/08/24 04:59:16, horo wrote:
> You don't need to include this header file.

Done.

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_dispatcher.h:30: 
On 2016/08/24 04:59:16, horo wrote:
> class EmbeddedWorkerDevToolsAgent;

Done.

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_instance_client_impl.cc
(right):

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:49:
start_data.v8CacheOptions = static_cast<blink::WebSettings::V8CacheOptions>(
On 2016/08/24 04:59:16, horo wrote:
> We should have static_assert() for mojo::V8CacheOptions and
> blink::WebSettings::V8CacheOptions.

Done.

[shimazu, 2016-08-25 05:26:15]

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

[shimazu, 2016-08-25 05:26:16]

dcheng@, could you take a look around mojo?

Especially I'd like to know base::Unretained doesn't cause a problem in this
case. 
|embedded_worker_dispatcher_| will be destroyed at RenderThreadImpl::Shutdown(),
but InterfaceProvider seems to be destroyed when RenderThreadImpl gets
destructed. As I wrote at an in-line comment, I'm not sure what will happen if
IPC request comes when Shutdown() is executing/has executed and RenderThreadImpl
(InterfaceProvider) has not been destructed.

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2227593002/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.cc:856:
base::Unretained(embedded_worker_dispatcher_.get())));
On 2016/08/24 04:59:16, horo wrote:
> I don't know well about the life cycle of InterfaceRegistry, but is it safe to
> pass the unretained embedded_worker_dispatcher_?
> If it is safe, please write comments about it.

I thought this was no problem, but now I'm suspecting something wrong might
happen if an IPC request comes when executing RenderThreadImpl::Shutdown.

[commit-bot: I haz the power, 2016-08-25 05:32:35]

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

[commit-bot: I haz the power, 2016-08-25 05:32:36]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[horo, 2016-08-30 03:18:05]

https://codereview.chromium.org/2227593002/diff/140001/content/public/common/...
File content/public/common/BUILD.gn (right):

https://codereview.chromium.org/2227593002/diff/140001/content/public/common/...
content/public/common/BUILD.gn:175: # This interface is internal to content.
However, this is not exported from
On 2016/08/25 05:14:52, shimazu wrote:
> On 2016/08/24 04:59:16, horo wrote:
> > Do we need this comment after "However, ..."?
> 
> Oops, this comment is probably copied from somewhere.
> Actually this interface is not internal (because this is in 'public'), so
> removed this comment and visibility.

Not changed??

https://codereview.chromium.org/2227593002/diff/220001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2227593002/diff/220001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:79: // A pointer is
passed as a parameter because mock functions cannot take
Do you want to check the params in tests?

https://codereview.chromium.org/2227593002/diff/220001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:134: // Only used
for tests that force creating a new render process. There is no
please update this comment.

[shimazu, 2016-09-02 03:01:25]

Patchset #14 (id:280001) has been deleted

[shimazu, 2016-09-02 03:05:05]

Updated. ptal:)

dcheng@: ping, kindly?

https://codereview.chromium.org/2227593002/diff/140001/content/public/common/...
File content/public/common/BUILD.gn (right):

https://codereview.chromium.org/2227593002/diff/140001/content/public/common/...
content/public/common/BUILD.gn:175: # This interface is internal to content.
However, this is not exported from
On 2016/08/30 03:18:05, horo wrote:
> On 2016/08/25 05:14:52, shimazu wrote:
> > On 2016/08/24 04:59:16, horo wrote:
> > > Do we need this comment after "However, ..."?
> > 
> > Oops, this comment is probably copied from somewhere.
> > Actually this interface is not internal (because this is in 'public'), so
> > removed this comment and visibility.
> 
> Not changed??

My mistake, sorry.. Done.

https://codereview.chromium.org/2227593002/diff/220001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2227593002/diff/220001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:79: // A pointer is
passed as a parameter because mock functions cannot take
On 2016/08/30 03:18:05, horo wrote:
> Do you want to check the params in tests?

Yes actually. Updated the test using ON_MOCK and checking the argument.

https://codereview.chromium.org/2227593002/diff/220001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:134: // Only used
for tests that force creating a new render process. There is no
On 2016/08/30 03:18:05, horo wrote:
> please update this comment.

Done.

[horo, 2016-09-05 03:40:38]

lgtm

But please ensure that base::Unretained(embedded_worker_dispatcher_.get()) in
RenderThreadImpl::Init() is safe.

[falken, 2016-09-07 05:01:40]

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

[falken, 2016-09-07 05:01:42]

drive by comments

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:74: void
RegisterToWorkerDevToolsManagerAndBindInterfaceOnUI(
These are now two pretty unrelated tasks. Maybe just SetupOnUI or RegisterOnUI
naming is better?

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:464:
GetProxy(&instance_client_);
Where is GetProxy from? should this be mojo::GetProxy?

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:607:
registry_->AddWorker(process_id(), embedded_worker_id());
The registry already holds all workers, even ones that aren't started yet. So
AddWorker() seems like a misleading name. Are you trying to notify the registry
that this worker is starting up?

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:310: // Current
running information.
This comment doesn't seem to fit the members on line 313-315, which seem to have
longer lifetimes than between start and stop. Can you separate them from 311-312
with whitespace and even better add individual comments for them?

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:315:
mojom::EmbeddedWorkerInstanceClientPtr instance_client_;
nit: Since this is a member of EmbeddedWorkerInstance, it seems it could be more
concisely named "client_".

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:269:
.Times(1);
Sorry I got to this late. gmock is fairly discouraged in chromium:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/-KH_IP0rIWQ
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/gmock$...

Do we really need gmock here?

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.h (right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.h:132:
ServiceWorkerStatusCode AddWorker(int process_id, int embedded_worker_id);
I see it's just temporary but I'd prefer a more precise name and comment. All
embedded workers should already be in the registry.

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:78: // take
move-only objects directly.
I'd rather we avoid gmock if possible.

https://codereview.chromium.org/2227593002/diff/300001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_instance_client_impl.cc
(right):

https://codereview.chromium.org/2227593002/diff/300001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:74:
DVLOG(2) << __FUNCTION__;
NOTIMPLEMENTED()

[dcheng, 2016-09-07 07:51:10]

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:589: auto mojo_params
= mojom::EmbeddedWorkerStartWorkerParams::New();
It might be slightly simpler to just declare
EmbeddedWorkerMsg_StartWorker_Params as a native mojo struct for now, and then
just migrate over when we remove the non-mojo path.

Alternatively, we can typemap (go/chrome-mojo-docs/typemapping), which is
preferable to writing manual serialization code like this.

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.cc:80: Factories...
factories) {
I think it might be clearer to just write this out rather than use a variadic
template.

https://codereview.chromium.org/2227593002/diff/300001/content/public/common/...
File content/public/common/web_preferences.mojom (right):

https://codereview.chromium.org/2227593002/diff/300001/content/public/common/...
content/public/common/web_preferences.mojom:9: V8_CACHE_OPTIONS_DEFAULT,
mojo enums are enum classes by default, so you can omit the V8_CACHE_OPTIONS_
prefix.

https://codereview.chromium.org/2227593002/diff/300001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_instance_client_impl.cc
(right):

https://codereview.chromium.org/2227593002/diff/300001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:27: void
EmbeddedWorkerInstanceClientImpl::StartWorker(
I guess this is based on the existing SW code? Will it be a maintenance problem
during the transition period?

https://codereview.chromium.org/2227593002/diff/300001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:43:
start_data.scriptURL = params->script_url;
Let's add a typemap for this instead of using native mojo structs actually, that
will make this code a lot simpler.

[shimazu, 2016-09-12 04:35:57]

Patchset #16 (id:340001) has been deleted

[shimazu, 2016-09-12 05:14:42]

Patchset #17 (id:380001) has been deleted

[shimazu, 2016-09-12 06:28:20]

PTAL again :D

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:74: void
RegisterToWorkerDevToolsManagerAndBindInterfaceOnUI(
On 2016/09/07 05:01:41, falken wrote:
> These are now two pretty unrelated tasks. Maybe just SetupOnUI or RegisterOnUI
> naming is better?

Done.

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:464:
GetProxy(&instance_client_);
On 2016/09/07 05:01:41, falken wrote:
> Where is GetProxy from? should this be mojo::GetProxy?

Done.

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:589: auto mojo_params
= mojom::EmbeddedWorkerStartWorkerParams::New();
On 2016/09/07 07:51:09, dcheng wrote:
> It might be slightly simpler to just declare
> EmbeddedWorkerMsg_StartWorker_Params as a native mojo struct for now, and then
> just migrate over when we remove the non-mojo path.
> 
> Alternatively, we can typemap (go/chrome-mojo-docs/typemapping), which is
> preferable to writing manual serialization code like this.

Done.

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:607:
registry_->AddWorker(process_id(), embedded_worker_id());
On 2016/09/07 05:01:41, falken wrote:
> The registry already holds all workers, even ones that aren't started yet. So
> AddWorker() seems like a misleading name. Are you trying to notify the
registry
> that this worker is starting up?

Done.

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:310: // Current
running information.
On 2016/09/07 05:01:42, falken wrote:
> This comment doesn't seem to fit the members on line 313-315, which seem to
have
> longer lifetimes than between start and stop. Can you separate them from
311-312
> with whitespace and even better add individual comments for them?

Done.

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:315:
mojom::EmbeddedWorkerInstanceClientPtr instance_client_;
On 2016/09/07 05:01:41, falken wrote:
> nit: Since this is a member of EmbeddedWorkerInstance, it seems it could be
more
> concisely named "client_".

Done.

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:269:
.Times(1);
On 2016/09/07 05:01:42, falken wrote:
> Sorry I got to this late. gmock is fairly discouraged in chromium:
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/-KH_IP0rIWQ
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/gmock$...
> 
> Do we really need gmock here?

Oops, I didn't know that...
The only use case of gmock in series of the paches is here:
https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...

I'll rewrite this test.

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.h (right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.h:132:
ServiceWorkerStatusCode AddWorker(int process_id, int embedded_worker_id);
On 2016/09/07 05:01:42, falken wrote:
> I see it's just temporary but I'd prefer a more precise name and comment. All
> embedded workers should already be in the registry.

Done.

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.cc:80: Factories...
factories) {
On 2016/09/07 07:51:09, dcheng wrote:
> I think it might be clearer to just write this out rather than use a variadic
> template.

Done.

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2227593002/diff/300001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:78: // take
move-only objects directly.
On 2016/09/07 05:01:42, falken wrote:
> I'd rather we avoid gmock if possible.

Done.

https://codereview.chromium.org/2227593002/diff/300001/content/public/common/...
File content/public/common/web_preferences.mojom (right):

https://codereview.chromium.org/2227593002/diff/300001/content/public/common/...
content/public/common/web_preferences.mojom:9: V8_CACHE_OPTIONS_DEFAULT,
On 2016/09/07 07:51:09, dcheng wrote:
> mojo enums are enum classes by default, so you can omit the V8_CACHE_OPTIONS_
> prefix.

Acknowledged.

https://codereview.chromium.org/2227593002/diff/300001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_instance_client_impl.cc
(right):

https://codereview.chromium.org/2227593002/diff/300001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:27: void
EmbeddedWorkerInstanceClientImpl::StartWorker(
On 2016/09/07 07:51:09, dcheng wrote:
> I guess this is based on the existing SW code? Will it be a maintenance
problem
> during the transition period?

Yes, actually. Moved the shared codes into EWDispatcher.

https://codereview.chromium.org/2227593002/diff/300001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:43:
start_data.scriptURL = params->script_url;
On 2016/09/07 07:51:09, dcheng wrote:
> Let's add a typemap for this instead of using native mojo structs actually,
that
> will make this code a lot simpler.

I tried to use the typemap, but I'm not certain about how to include blink-side
object into the container. I guess we can add an additional typemap when we move
these objects to blink-side for content modularization.

https://codereview.chromium.org/2227593002/diff/300001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:74:
DVLOG(2) << __FUNCTION__;
On 2016/09/07 05:01:42, falken wrote:
> NOTIMPLEMENTED()

Done.

[falken, 2016-09-13 05:48:03]

Sorry for the delay, another round of comments.
(If possible, it's good to break big patches like this into smaller ones;
sometimes that's not possible though)

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:591: return;
no need for return here

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:313: // renderer
process. |interface_registry_| will be sent after a process has
Maybe just "|client_| is used to send messages to the renderer process"?

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:315: // when the
worker thread on the remote process gets ready for IPC.
Thank you writing for the comments.

I'm confused what it means to send the interface_registry_ and
remote_interfaces_.

|interface_registry_| is sent to the renderer process after the worker thread
starts (in OnThreadStarted)?

|remote_interfaces_| is sent back by the renderer process when the worker thread
is ready for IPC?

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.cc:276: return
SERVICE_WORKER_ERROR_ABORT;
nit: If I understand correctly this function isn't using context_ anywhere, so
we don't need a return value.

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.h (right):

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.h:129: // Called when
EmbeddedWorkerInstance gets ready for IPC. This function
english nit: do you mean "is ready for IPC"? "gets ready" may mean it's still
getting ready.

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.cc:129:
helper->mock_instance_clients_[helper->mock_instance_clients_next_++];
Is the |next_| thing really needed? It's possible for size() to not equal
|next_|? Looking at the code now it looks like mock_instance_clients_ is only
mutated by Bind(), in which case you shouldn't need this complexity.

If it is needed, I'd recommend some refactoring so
"helper->mock_instance_clients" isn't repeated so much. Something like

std::vector<std::unique_ptr<MockEmbeddedWorkerInstanceClient>>* clients =
helper->mock_instance_clients_;
size_t next_client_index = helper->next_mock_instance_client_index_;

ASSERT_GE(clients->size(), next_client_index);
if (clients->size() == next_client_index) {
 
clients->push_back(base::MakeUnique<MockEmbeddedWorkerInstanceClient>(helper));
}

std::unique_ptr<MockEmbeddedWorkerInstanceClient> client =
clients[next_client_index];
helper->next_mock_instance_client_index_ = next_client_index + 1;
if (client)
  client->binding_.Bind(std::move(request));

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:77: //
Implementation of mojo interfaces
nit: period at end

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:225: size_t
mock_instance_clients_next_;
nit: next_mock_instance_client_index_ or position_ is more conventional.

https://codereview.chromium.org/2227593002/diff/420001/content/common/service...
File content/common/service_worker/embedded_worker.mojom (right):

https://codereview.chromium.org/2227593002/diff/420001/content/common/service...
content/common/service_worker/embedded_worker.mojom:13: interface
EmbeddedWorkerInstanceClient {
Can you add some lightweight documentation? I'm looking at
chrome/src/ash/public/interfaces/wallpaper.mojom or shelf.mojom as an example
(when in doubt ash/ tends to have clean modern code)

https://codereview.chromium.org/2227593002/diff/420001/content/public/common/...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/2227593002/diff/420001/content/public/common/...
content/public/common/content_switches.cc:674: // Use a Mojo-based ServiceWorker
implimentation.
implementation

https://codereview.chromium.org/2227593002/diff/420001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_instance_client_impl.cc
(right):

https://codereview.chromium.org/2227593002/diff/420001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:55:
DVLOG(2) << __FUNCTION__;
dont think you need this DVLOG since you ahve the NOTIMPLEMENTED

[shimazu, 2016-09-15 03:24:42]

Thanks for your comments.
PTAL again:)

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:591: return;
On 2016/09/13 05:48:02, falken wrote:
> no need for return here

Done.

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:313: // renderer
process. |interface_registry_| will be sent after a process has
On 2016/09/13 05:48:02, falken wrote:
> Maybe just "|client_| is used to send messages to the renderer process"?

Done.

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:315: // when the
worker thread on the remote process gets ready for IPC.
On 2016/09/13 05:48:02, falken wrote:
> Thank you writing for the comments.
> 
> I'm confused what it means to send the interface_registry_ and
> remote_interfaces_.
> 
> |interface_registry_| is sent to the renderer process after the worker thread
> starts (in OnThreadStarted)?
> 
> |remote_interfaces_| is sent back by the renderer process when the worker
thread
> is ready for IPC?

Sorry for confusing.
It's connected on EmbeddedWorkerSetup->ExchangeInterfaces() in this patch, and
InterfaceRegistry/Provider in this object will be connected at
StartWorker/AttachClientInterfaceProvider at https://crrev.com/2313653002 .
I wrote what will happen after https://crrev.com/2313653002 is landed by mistake
;(
I'll move this message to that CL.

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.cc:276: return
SERVICE_WORKER_ERROR_ABORT;
On 2016/09/13 05:48:02, falken wrote:
> nit: If I understand correctly this function isn't using context_ anywhere, so
> we don't need a return value.

I think so. Removed.

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
File content/browser/service_worker/embedded_worker_registry.h (right):

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_registry.h:129: // Called when
EmbeddedWorkerInstance gets ready for IPC. This function
On 2016/09/13 05:48:02, falken wrote:
> english nit: do you mean "is ready for IPC"? "gets ready" may mean it's still
> getting ready.

Done.

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.cc:129:
helper->mock_instance_clients_[helper->mock_instance_clients_next_++];
On 2016/09/13 05:48:02, falken wrote:
> Is the |next_| thing really needed? It's possible for size() to not equal
> |next_|? Looking at the code now it looks like mock_instance_clients_ is only
> mutated by Bind(), in which case you shouldn't need this complexity.
> 
> If it is needed, I'd recommend some refactoring so
> "helper->mock_instance_clients" isn't repeated so much. Something like
> 
> std::vector<std::unique_ptr<MockEmbeddedWorkerInstanceClient>>* clients =
> helper->mock_instance_clients_;
> size_t next_client_index = helper->next_mock_instance_client_index_;
> 
> ASSERT_GE(clients->size(), next_client_index);
> if (clients->size() == next_client_index) {
>  
>
clients->push_back(base::MakeUnique<MockEmbeddedWorkerInstanceClient>(helper));
> }
> 
> std::unique_ptr<MockEmbeddedWorkerInstanceClient> client =
> clients[next_client_index];
> helper->next_mock_instance_client_index_ = next_client_index + 1;
> if (client)
>   client->binding_.Bind(std::move(request));

Sorry for confusing you... This will be used in later patch. [1] is where it's
used to simulate failure of the connection.
Updated.

[1]
https://codereview.chromium.org/2307543002/diff/60001/content/browser/service...

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:77: //
Implementation of mojo interfaces
On 2016/09/13 05:48:02, falken wrote:
> nit: period at end

Done.

https://codereview.chromium.org/2227593002/diff/420001/content/browser/servic...
content/browser/service_worker/embedded_worker_test_helper.h:225: size_t
mock_instance_clients_next_;
On 2016/09/13 05:48:02, falken wrote:
> nit: next_mock_instance_client_index_ or position_ is more conventional.

Done.

https://codereview.chromium.org/2227593002/diff/420001/content/common/service...
File content/common/service_worker/embedded_worker.mojom (right):

https://codereview.chromium.org/2227593002/diff/420001/content/common/service...
content/common/service_worker/embedded_worker.mojom:13: interface
EmbeddedWorkerInstanceClient {
On 2016/09/13 05:48:02, falken wrote:
> Can you add some lightweight documentation? I'm looking at
> chrome/src/ash/public/interfaces/wallpaper.mojom or shelf.mojom as an example
> (when in doubt ash/ tends to have clean modern code)

Done.

https://codereview.chromium.org/2227593002/diff/420001/content/public/common/...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/2227593002/diff/420001/content/public/common/...
content/public/common/content_switches.cc:674: // Use a Mojo-based ServiceWorker
implimentation.
On 2016/09/13 05:48:02, falken wrote:
> implementation

Done.

https://codereview.chromium.org/2227593002/diff/420001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_instance_client_impl.cc
(right):

https://codereview.chromium.org/2227593002/diff/420001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:55:
DVLOG(2) << __FUNCTION__;
On 2016/09/13 05:48:02, falken wrote:
> dont think you need this DVLOG since you ahve the NOTIMPLEMENTED

Done.

[falken, 2016-09-20 03:03:32]

Apologies again.

https://codereview.chromium.org/2227593002/diff/440001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2227593002/diff/440001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:355: scope,
is_installed_, base::Passed(&request_),
Is it ok to do base::Passed on a member variable that isn't a pointer? Should
|request_| be a unique_ptr instead?

https://codereview.chromium.org/2227593002/diff/440001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:405:
mojom::EmbeddedWorkerInstanceClientRequest request_;
It bugs me that |instance_| has a lifetime comment but |request_| doesn't.
"Ownership is transferred by base::Passed() to a task after process allocation"?

I also think it's better if |request_| is a unique_ptr to make clear ownership
is transferred as we get a clean crash if we try to use it after transfer. But
maybe I'm not so familiar with mojo conventions.

https://codereview.chromium.org/2227593002/diff/440001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2227593002/diff/440001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:312: // These are
connected to a renderer process after OnThreadStarted.
nit: "the renderer process" ("a" makes me wonder which renderer process? In this
case, it's obvious it's the one with the thread that started, so "the" reduces
confusion.)

[shimazu, 2016-09-20 05:37:30]

Thanks for your comments.
Updated.

https://codereview.chromium.org/2227593002/diff/440001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2227593002/diff/440001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:355: scope,
is_installed_, base::Passed(&request_),
On 2016/09/20 03:03:32, falken (behind on reviews) wrote:
> Is it ok to do base::Passed on a member variable that isn't a pointer? Should
> |request_| be a unique_ptr instead?

base::Passed internally uses std::move, so there seems to be no problem.

IIUC, InterfaceRequest<T> is a container class for a pipe instead of 
unique_ptr, so I think it's okay to have InterfaceRequest as an member.
WDYT?

https://codereview.chromium.org/2227593002/diff/440001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:405:
mojom::EmbeddedWorkerInstanceClientRequest request_;
On 2016/09/20 03:03:32, falken (behind on reviews) wrote:
> It bugs me that |instance_| has a lifetime comment but |request_| doesn't.
> "Ownership is transferred by base::Passed() to a task after process
allocation"?
> 
> I also think it's better if |request_| is a unique_ptr to make clear ownership
> is transferred as we get a clean crash if we try to use it after transfer. But
> maybe I'm not so familiar with mojo conventions.

Added the comment.

I think InterfaceRequest (mojom::EmbeddedWorkerInstanceClientRequest is an alias
of InterfaceRequest<mojom::EmbeddedWorkerInstanceClient>) is a container class
similar to unique_ptr<>. I think it's not necessary to wrap the request by
unique_ptr.

[shimazu, 2016-09-20 05:38:09]

https://codereview.chromium.org/2227593002/diff/440001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/2227593002/diff/440001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.h:312: // These are
connected to a renderer process after OnThreadStarted.
On 2016/09/20 03:03:32, falken wrote:
> nit: "the renderer process" ("a" makes me wonder which renderer process? In
this
> case, it's obvious it's the one with the thread that started, so "the" reduces
> confusion.)

Done.

[falken, 2016-09-20 05:40:35]

OK lgtm

[shimazu, 2016-09-20 05:49:52]

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

[shimazu, 2016-09-20 05:49:53]

avi@: Please review changes in the following:
- content/browser/renderer_host
- content/renderer/renderer_thread_impl.cc
- content/public
- BUILD.gn, typemaps.gni, content_common_mojo_bindings.gyp

[shimazu, 2016-09-20 05:49:54]

avi@: Please review changes in the following:
- content/browser/renderer_host
- content/renderer/renderer_thread_impl.cc
- content/public
- BUILD.gn, typemaps.gni, content_common_mojo_bindings.gyp

[Avi (use Gerrit), 2016-09-20 14:05:56]

lgtm

stampity stamp

[dcheng, 2016-09-21 08:21:17]

ipc + mojom lgtm

https://codereview.chromium.org/2227593002/diff/300001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_instance_client_impl.cc
(right):

https://codereview.chromium.org/2227593002/diff/300001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:43:
start_data.scriptURL = params->script_url;
On 2016/09/12 06:28:19, shimazu wrote:
> On 2016/09/07 07:51:09, dcheng wrote:
> > Let's add a typemap for this instead of using native mojo structs actually,
> that
> > will make this code a lot simpler.
> 
> I tried to use the typemap, but I'm not certain about how to include
blink-side
> object into the container. I guess we can add an additional typemap when we
move
> these objects to blink-side for content modularization.

Yes, you're right, that would be the right time to do that. Eventually =)

https://codereview.chromium.org/2227593002/diff/460001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_instance_client_impl.cc
(right):

https://codereview.chromium.org/2227593002/diff/460001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:24: new
EmbeddedWorkerInstanceClientImpl(dispatcher, std::move(request));
I think it's preferred to use mojo::MakeStrongBInding now to avoid bare new like
this.

[shimazu, 2016-09-23 03:04:14]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2016-09-23 03:04:15]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org,
falken@chromium.org, avi@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2227593002/#ps480001
(title: "Used MakeStrongBinding")

[shimazu, 2016-09-23 03:04:40]

https://codereview.chromium.org/2227593002/diff/460001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_instance_client_impl.cc
(right):

https://codereview.chromium.org/2227593002/diff/460001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:24: new
EmbeddedWorkerInstanceClientImpl(dispatcher, std::move(request));
On 2016/09/21 08:21:17, dcheng wrote:
> I think it's preferred to use mojo::MakeStrongBInding now to avoid bare new
like
> this.

Done.

[commit-bot: I haz the power, 2016-09-23 03:04:40]

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-09-23 04:03:06]

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

[commit-bot: I haz the power, 2016-09-23 04:03:09]

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-09-23 07:54:53]

On 2016/09/23 04:03:09, commit-bot: I haz the power wrote:
> 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_...)

It's caused by an update of IPC capability:
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-mojo/capab...

[shimazu, 2016-09-23 11:43:09]

dcheng@: could you check the json just in case?
Is it enough to add a name of the interface?

[dcheng, 2016-09-27 16:53:30]

json LGTM

[shimazu, 2016-09-28 00:14:15]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2016-09-28 00:14:15]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
avi@chromium.org, horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2227593002/#ps500001
(title: "Added EmbeddedWorkerInstanceClient as an interface")

[commit-bot: I haz the power, 2016-09-28 00:14: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, 2016-09-28 02:10:31]

Description was changed from

==========
ServiceWorker: Implement StartWorker by using mojo

This patch is the first step of mojofication on the service worker.
In this patch, EWInstance will send StartWorker IPC using mojo. The endpoint on
the renderer process is EWInstanceClient which is corresponding to one
EWInstance. In future, this enables us to remove EWDispatcher, but
EWInstanceClient has reference to EWDispatcher for now.

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

to

==========
ServiceWorker: Implement StartWorker by using mojo

This patch is the first step of mojofication on the service worker.
In this patch, EWInstance will send StartWorker IPC using mojo. The endpoint on
the renderer process is EWInstanceClient which is corresponding to one
EWInstance. In future, this enables us to remove EWDispatcher, but
EWInstanceClient has reference to EWDispatcher for now.

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

[commit-bot: I haz the power, 2016-09-28 02:10:34]

Committed patchset #22 (id:500001)

[commit-bot: I haz the power, 2016-09-28 02:12:48]

Description was changed from

==========
ServiceWorker: Implement StartWorker by using mojo

This patch is the first step of mojofication on the service worker.
In this patch, EWInstance will send StartWorker IPC using mojo. The endpoint on
the renderer process is EWInstanceClient which is corresponding to one
EWInstance. In future, this enables us to remove EWDispatcher, but
EWInstanceClient has reference to EWDispatcher for now.

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

to

==========
ServiceWorker: Implement StartWorker by using mojo

This patch is the first step of mojofication on the service worker.
In this patch, EWInstance will send StartWorker IPC using mojo. The endpoint on
the renderer process is EWInstanceClient which is corresponding to one
EWInstance. In future, this enables us to remove EWDispatcher, but
EWInstanceClient has reference to EWDispatcher for now.

BUG=629701

Committed: https://crrev.com/5db8eb672a52385059d4d188e27d7a7f0a5dd29d
Cr-Commit-Position: refs/heads/master@{#421427}
==========

[commit-bot: I haz the power, 2016-09-28 02:12:50]

Patchset 22 (id:??) landed as
https://crrev.com/5db8eb672a52385059d4d188e27d7a7f0a5dd29d
Cr-Commit-Position: refs/heads/master@{#421427}

[jwd, 2016-09-28 13:58:25]

A revert of this CL (patchset #22 id:500001) has been created in
https://codereview.chromium.org/2381513002/ by jwd@chromium.org.

The reason for reverting is: Looks like this is causing failures on webkit
builders
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu...
.