Implementations of SharedWorker in the renderer process.

Implementations of SharedWorker in the renderer process.

In this CL I introduce 2 classes (EmbeddedSharedWorkerDevToolsAgent, EmbeddedSharedWorkerStub).

EmbeddedSharedWorkerStub and EmbeddedSharedWorkerDevToolsAgent are almost same as WebSharedWorkerStub and SharedWorkerDevToolsAgent which are used in the worker process now.
These classes are not used yet.
EmbeddedSharedWorkerStub will be created when CreateWorker message is received by RenderThreadImpl.

In this CL I move shared_worker_devtools_agent.* from content/worker/ to content/child/ because I want use it from content/renderer/shared_worker.

BUG=327256
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251335

[horo, 2014-02-12 08:06:54]

kinuko@
Could you please review this cl?
Thank you.

[kinuko, 2014-02-12 11:49:33]

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
File content/renderer/shared_worker/embedded_shared_worker_devtools_agent.cc
(right):

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_devtools_agent.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
Is it possible to adjust --similarity when you upload the patch (I assume you
use git cl upload) so that this can be showed in a diff from
shared_worker_devtools_agent.cc?

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
File content/renderer/shared_worker/embedded_shared_worker_message_filter.cc
(right):

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_message_filter.cc:36: if
(!WorkerProcessMsg_CreateWorker::Read(&msg, &param)) {
It feels we'll need new names for these messages?

Could we maybe rename all of these messages to something like
SharedWorkerMsg_Foo to start with?
(Are we going to use the same set of messages?)

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_message_filter.cc:44:
return true;
I feel using regular IPC_BEGIN_MESSAGE_MAP would be less error-prone here.

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
File content/renderer/shared_worker/embedded_shared_worker_message_filter.h
(right):

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_message_filter.h:23: const
IPC::Message& msg) OVERRIDE;
Are we going to need this thread override?  If all you want is to get messages
on the main thread you can just inherit from IPC::Listener and call
OnMessageReceived in the render_thread_impl.cc.  (Or it'd be probably ok to just
add IPC_MESSAGE_HANDLER() case for CreateWorker msg directly in the
render_thread_impl.cc-- if the reviewer agrees)

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right):

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:17: 
nit: extra empty line

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:24: }
Do we need this in .cc?

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:26: namespace
content {
nit: have empty line after namespace {

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:36: url_(url) {
Will we call RenderThreadImpl::current()->AddRoute() here (and RemoveRoute in
the dtor)?

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:107: // Coming
soon.
Is this TODO still applicable / do we need this?

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:163:
impl_->terminateWorkerContext();
Will we have 'ensure worker context terminates' part here?

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
File content/renderer/shared_worker/embedded_shared_worker_stub.h (right):

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.h:30: class
EmbeddedSharedWorkerStub : public IPC::Listener ,
nit: weird space before ','

[horo, 2014-02-13 05:30:00]

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
File content/renderer/shared_worker/embedded_shared_worker_devtools_agent.cc
(right):

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_devtools_agent.cc:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/02/12 11:49:33, kinuko wrote:
> Is it possible to adjust --similarity when you upload the patch (I assume you
> use git cl upload) so that this can be showed in a diff from
> shared_worker_devtools_agent.cc?

Done.

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
File content/renderer/shared_worker/embedded_shared_worker_message_filter.cc
(right):

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_message_filter.cc:36: if
(!WorkerProcessMsg_CreateWorker::Read(&msg, &param)) {
On 2014/02/12 11:49:33, kinuko wrote:
> It feels we'll need new names for these messages?
> 
> Could we maybe rename all of these messages to something like
> SharedWorkerMsg_Foo to start with?
> (Are we going to use the same set of messages?)

I will define SharedWorkerMsg_CreateWorker in another CL.

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
File content/renderer/shared_worker/embedded_shared_worker_message_filter.h
(right):

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_message_filter.h:23: const
IPC::Message& msg) OVERRIDE;
On 2014/02/12 11:49:33, kinuko wrote:
> Are we going to need this thread override?  If all you want is to get messages
> on the main thread you can just inherit from IPC::Listener and call
> OnMessageReceived in the render_thread_impl.cc.  (Or it'd be probably ok to
just
> add IPC_MESSAGE_HANDLER() case for CreateWorker msg directly in the
> render_thread_impl.cc-- if the reviewer agrees)

I deleted EmbeddedSharedWorkerMessageFilter from this CL.

EmbeddedSharedWorkerStub will be created when CreateWorker message is received
by RenderThreadImpl.

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right):

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:17: 
On 2014/02/12 11:49:33, kinuko wrote:
> nit: extra empty line

Done.

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:24: }
On 2014/02/12 11:49:33, kinuko wrote:
> Do we need this in .cc?

Done.

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:26: namespace
content {
On 2014/02/12 11:49:33, kinuko wrote:
> nit: have empty line after namespace {

Done.

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:36: url_(url) {
On 2014/02/12 11:49:33, kinuko wrote:
> Will we call RenderThreadImpl::current()->AddRoute() here (and RemoveRoute in
> the dtor)?

Done.

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:107: // Coming
soon.
On 2014/02/12 11:49:33, kinuko wrote:
> Is this TODO still applicable / do we need this?

This is copied from websharedworkerclient_proxy.cc.
I will ask johnnyg@ whether there is still a plan to implement
WebSharedWorkerClientProxy::notificationPresenter().

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:163:
impl_->terminateWorkerContext();
On 2014/02/12 11:49:33, kinuko wrote:
> Will we have 'ensure worker context terminates' part here?

I think we don't need to call EnsureWorkerContextTerminates like
WebSharedWorkerClientProxy::EnsureWorkerContextTerminates.

This code has been introduced in order to prevent a worker that loops forever
from preventing shutdown of the worker process in
https://codereview.chromium.org/112049.
But in current code, even if "while(1){}" is being evaluated by
script->evaluate() in WorkerThread::workerThread(),
WebSharedWorkerImpl::terminateWorkerContext() will call
v8::V8::TerminateExecution() and then script->evaluate() will stop evaluation.

Is my understanding incorrect?

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
File content/renderer/shared_worker/embedded_shared_worker_stub.h (right):

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.h:30: class
EmbeddedSharedWorkerStub : public IPC::Listener ,
On 2014/02/12 11:49:33, kinuko wrote:
> nit: weird space before ','

Done.

[kinuko, 2014-02-13 06:09:35]

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right):

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:107: // Coming
soon.
On 2014/02/13 05:30:01, horo wrote:
> On 2014/02/12 11:49:33, kinuko wrote:
> > Is this TODO still applicable / do we need this?
> 
> This is copied from websharedworkerclient_proxy.cc.
> I will ask johnnyg@ whether there is still a plan to implement
> WebSharedWorkerClientProxy::notificationPresenter().

Afaik johnny's transferred to another team (youtube), and this method doesn't
seem to be called by anyone.

We can probably have a better different TODO comment for this?  (E.g. delete
this method if we have no plan to implement this, etc)

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:163:
impl_->terminateWorkerContext();
On 2014/02/13 05:30:01, horo wrote:
> On 2014/02/12 11:49:33, kinuko wrote:
> > Will we have 'ensure worker context terminates' part here?
> 
> I think we don't need to call EnsureWorkerContextTerminates like
> WebSharedWorkerClientProxy::EnsureWorkerContextTerminates.
> 
> This code has been introduced in order to prevent a worker that loops forever
> from preventing shutdown of the worker process in
> https://codereview.chromium.org/112049.
> But in current code, even if "while(1){}" is being evaluated by
> script->evaluate() in WorkerThread::workerThread(),
> WebSharedWorkerImpl::terminateWorkerContext() will call
> v8::V8::TerminateExecution() and then script->evaluate() will stop evaluation.
> 
> Is my understanding incorrect?

I don't know, just wondered why you didn't copy this.

v8::V8::TerminateExecution change seems to have made after the addition of
EnsureWorkerContextTerminates, so probably you're right. (Does that mean we
don't need it in WebSharedWorkerClientProxy either, right?)
https://bugs.webkit.org/show_bug.cgi?id=35137

https://codereview.chromium.org/158953008/diff/100001/content/renderer/shared...
File content/renderer/shared_worker/embedded_shared_worker_devtools_agent.h
(right):

https://codereview.chromium.org/158953008/diff/100001/content/renderer/shared...
content/renderer/shared_worker/embedded_shared_worker_devtools_agent.h:23: class
EmbeddedSharedWorkerDevToolsAgent {
Will we need a separate class from SharedWorkerDevToolsAgent for this? (I.e.
will it diverge?)

[horo, 2014-02-13 08:54:20]

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right):

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:107: // Coming
soon.
On 2014/02/13 06:09:36, kinuko wrote:
> On 2014/02/13 05:30:01, horo wrote:
> > On 2014/02/12 11:49:33, kinuko wrote:
> > > Is this TODO still applicable / do we need this?
> > 
> > This is copied from websharedworkerclient_proxy.cc.
> > I will ask johnnyg@ whether there is still a plan to implement
> > WebSharedWorkerClientProxy::notificationPresenter().
> 
> Afaik johnny's transferred to another team (youtube), and this method doesn't
> seem to be called by anyone.
> 
> We can probably have a better different TODO comment for this?  (E.g. delete
> this method if we have no plan to implement this, etc)

Done.

https://codereview.chromium.org/158953008/diff/1/content/renderer/shared_work...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:163:
impl_->terminateWorkerContext();
On 2014/02/13 06:09:36, kinuko wrote:
> On 2014/02/13 05:30:01, horo wrote:
> > On 2014/02/12 11:49:33, kinuko wrote:
> > > Will we have 'ensure worker context terminates' part here?
> > 
> > I think we don't need to call EnsureWorkerContextTerminates like
> > WebSharedWorkerClientProxy::EnsureWorkerContextTerminates.
> > 
> > This code has been introduced in order to prevent a worker that loops
forever
> > from preventing shutdown of the worker process in
> > https://codereview.chromium.org/112049.
> > But in current code, even if "while(1){}" is being evaluated by
> > script->evaluate() in WorkerThread::workerThread(),
> > WebSharedWorkerImpl::terminateWorkerContext() will call
> > v8::V8::TerminateExecution() and then script->evaluate() will stop
evaluation.
> > 
> > Is my understanding incorrect?
> 
> I don't know, just wondered why you didn't copy this.
> 
> v8::V8::TerminateExecution change seems to have made after the addition of
> EnsureWorkerContextTerminates, so probably you're right. (Does that mean we
> don't need it in WebSharedWorkerClientProxy either, right?)
> https://bugs.webkit.org/show_bug.cgi?id=35137

I think we don't need WebSharedWorkerClientProxy::EnsureWorkerContextTerminates
either.

https://codereview.chromium.org/158953008/diff/100001/content/renderer/shared...
File content/renderer/shared_worker/embedded_shared_worker_devtools_agent.h
(right):

https://codereview.chromium.org/158953008/diff/100001/content/renderer/shared...
content/renderer/shared_worker/embedded_shared_worker_devtools_agent.h:23: class
EmbeddedSharedWorkerDevToolsAgent {
On 2014/02/13 06:09:36, kinuko wrote:
> Will we need a separate class from SharedWorkerDevToolsAgent for this? (I.e.
> will it diverge?)

I moved shared_worker_devtools_agent.* to content/child.

[kinuko, 2014-02-13 09:06:48]

lgtm

https://codereview.chromium.org/158953008/diff/190001/content/renderer/shared...
File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right):

https://codereview.chromium.org/158953008/diff/190001/content/renderer/shared...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:13: #include
"content/child/worker_thread_task_runner.h"
Not necessary?

[horo, 2014-02-13 09:38:27]

https://codereview.chromium.org/158953008/diff/190001/content/renderer/shared...
File content/renderer/shared_worker/embedded_shared_worker_stub.cc (right):

https://codereview.chromium.org/158953008/diff/190001/content/renderer/shared...
content/renderer/shared_worker/embedded_shared_worker_stub.cc:13: #include
"content/child/worker_thread_task_runner.h"
On 2014/02/13 09:06:48, kinuko wrote:
> Not necessary?

Done.

[horo, 2014-02-13 09:39:03]

jochen@
Could you please review?
Thank you.

[horo, 2014-02-14 08:49:37]

jochen@
ping :)

[jochen (gone - plz use gerrit), 2014-02-14 09:36:42]

lgtm

[horo, 2014-02-14 10:49:22]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-02-14 10:49:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/horo@chromium.org/158953008/320001

[commit-bot: I haz the power, 2014-02-14 15:30:26]

Change committed as 251335

[sof, 2014-06-23 08:17:22]

https://codereview.chromium.org/158953008/diff/320001/content/renderer/shared...
File content/renderer/shared_worker/embedded_shared_worker_stub.h (right):

https://codereview.chromium.org/158953008/diff/320001/content/renderer/shared...
content/renderer/shared_worker/embedded_shared_worker_stub.h:73:
blink::WebSharedWorker* impl_;
Who gets to delete this object? Might it explain the
https://code.google.com/p/chromium/issues/detail?id=364390#c2 shared worker
leaks?

[horo, 2014-06-23 08:23:03]

https://codereview.chromium.org/158953008/diff/320001/content/renderer/shared...
File content/renderer/shared_worker/embedded_shared_worker_stub.h (right):

https://codereview.chromium.org/158953008/diff/320001/content/renderer/shared...
content/renderer/shared_worker/embedded_shared_worker_stub.h:73:
blink::WebSharedWorker* impl_;
On 2014/06/23 08:17:22, sof wrote:
> Who gets to delete this object? Might it explain the
> https://code.google.com/p/chromium/issues/detail?id=364390#c2 shared worker
> leaks?
WebSharedWorkerImpl deletes itself in
WebSharedWorkerImpl::workerGlobalScopeDestroyedOnMainThread().
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...