Introduce EmbeddedWorkerDevToolsAgent.

Introduce EmbeddedWorkerDevToolsAgent.

EmbeddedWorkerDevToolsAgent is registered to RenderThreadImpl with worker_devtools_agent_route_id which will be introduced by https://codereview.chromium.org/252633003.

This change depends on  https://codereview.chromium.org/252633003/.

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

[horo, 2014-04-25 02:35:27]

kinuko@
Could you please review?

[kinuko, 2014-04-25 04:38:01]

https://codereview.chromium.org/251723002/diff/1/content/common/service_worke...
File content/common/service_worker/embedded_worker_messages.h (right):

https://codereview.chromium.org/251723002/diff/1/content/common/service_worke...
content/common/service_worker/embedded_worker_messages.h:43: IPC::Message /*
message */)
I found that DevToolsAgentMsg_* are existing messages and we use similar nested
messaging to deliver them to SharedWorker, but these DevToolsAgentMsgs are
routed, why don't we use routing_id to send it to the target worker here? (We're
in general trying to remove nested messaging from EmbeddedWorker code)

[kinuko, 2014-04-25 10:29:00]

On 2014/04/25 04:38:01, kinuko wrote:
>
https://codereview.chromium.org/251723002/diff/1/content/common/service_worke...
> File content/common/service_worker/embedded_worker_messages.h (right):
> 
>
https://codereview.chromium.org/251723002/diff/1/content/common/service_worke...
> content/common/service_worker/embedded_worker_messages.h:43: IPC::Message /*
> message */)
> I found that DevToolsAgentMsg_* are existing messages and we use similar
nested
> messaging to deliver them to SharedWorker, but these DevToolsAgentMsgs are
> routed, why don't we use routing_id to send it to the target worker here?
(We're
> in general trying to remove nested messaging from EmbeddedWorker code)

Ok on the second thoughts (and after chatting w/ horo) if intent here is to use
the same message types for several targets (e.g. SharedWorker and
EmbeddedWorker) we'll need this wrapping or some mechanism to send the same type
of messages to different modules.

[kinuko, 2014-04-28 07:25:56]

https://codereview.chromium.org/251723002/diff/20001/content/common/service_w...
File content/common/service_worker/embedded_worker_messages.h (right):

https://codereview.chromium.org/251723002/diff/20001/content/common/service_w...
content/common/service_worker/embedded_worker_messages.h:41:
IPC_MESSAGE_CONTROL2(EmbeddedWorkerMsg_MessageToWorker,
If this message type's only used to dispatch devtools messages can we reflect
that to its name (and comment)?  We also have similar message
EmbeddedWorkerContextMsg_MessageToWorker and it's probably too confusing.

https://codereview.chromium.org/251723002/diff/20001/content/renderer/service...
File content/renderer/service_worker/embedded_worker_dispatcher.cc (right):

https://codereview.chromium.org/251723002/diff/20001/content/renderer/service...
content/renderer/service_worker/embedded_worker_dispatcher.cc:28: class
EmbeddedWorkerDispatcher::WorkerWrapper {
Now that it also handles messages the comment and class name is probably a bit
stale/misnomer.  It could be renamed to, say, EmbeddedWorkerStub, or we could
have a separate class for DevTools related messages, like,
EmbeddedWorkerDevToolsDispatcher, and this class can just have a pointer
(scoped_ptr) to it.  (In either case I think it's nicer to have it in a separate
file)

[horo, 2014-04-28 08:33:09]

https://codereview.chromium.org/251723002/diff/20001/content/common/service_w...
File content/common/service_worker/embedded_worker_messages.h (right):

https://codereview.chromium.org/251723002/diff/20001/content/common/service_w...
content/common/service_worker/embedded_worker_messages.h:41:
IPC_MESSAGE_CONTROL2(EmbeddedWorkerMsg_MessageToWorker,
On 2014/04/28 07:25:56, kinuko wrote:
> If this message type's only used to dispatch devtools messages can we reflect
> that to its name (and comment)?  We also have similar message
> EmbeddedWorkerContextMsg_MessageToWorker and it's probably too confusing.

Done.

https://codereview.chromium.org/251723002/diff/20001/content/renderer/service...
File content/renderer/service_worker/embedded_worker_dispatcher.cc (right):

https://codereview.chromium.org/251723002/diff/20001/content/renderer/service...
content/renderer/service_worker/embedded_worker_dispatcher.cc:28: class
EmbeddedWorkerDispatcher::WorkerWrapper {
On 2014/04/28 07:25:56, kinuko wrote:
> Now that it also handles messages the comment and class name is probably a bit
> stale/misnomer.  It could be renamed to, say, EmbeddedWorkerStub, or we could
> have a separate class for DevTools related messages, like,
> EmbeddedWorkerDevToolsDispatcher, and this class can just have a pointer
> (scoped_ptr) to it.  (In either case I think it's nicer to have it in a
separate
> file)

Done.
Added EmbeddedWorkerDevToolsAgent.

[kinuko, 2014-04-29 01:16:08]

lgtm

https://codereview.chromium.org/251723002/diff/60001/content/renderer/service...
File content/renderer/service_worker/embedded_worker_dispatcher.cc (right):

https://codereview.chromium.org/251723002/diff/60001/content/renderer/service...
content/renderer/service_worker/embedded_worker_dispatcher.cc:37: void
OnMessageToDevToolsAgent(const IPC::Message& message);
nit: this can just expose dev_tools_agent() field and we can call its
OnMessageReceived directly in
EmbeddedWorkerDispatcher::OnMessageToDevToolsAgent.

https://codereview.chromium.org/251723002/diff/60001/content/renderer/service...
content/renderer/service_worker/embedded_worker_dispatcher.cc:47:
dev_tools_agent_->OnMessageReceived(message);
maybe DCHECK the return value (to see if it returns true)

[horo, 2014-04-29 06:13:45]

https://codereview.chromium.org/251723002/diff/60001/content/renderer/service...
File content/renderer/service_worker/embedded_worker_dispatcher.cc (right):

https://codereview.chromium.org/251723002/diff/60001/content/renderer/service...
content/renderer/service_worker/embedded_worker_dispatcher.cc:37: void
OnMessageToDevToolsAgent(const IPC::Message& message);
On 2014/04/29 01:16:09, kinuko wrote:
> nit: this can just expose dev_tools_agent() field and we can call its
> OnMessageReceived directly in
> EmbeddedWorkerDispatcher::OnMessageToDevToolsAgent.

Done.

https://codereview.chromium.org/251723002/diff/60001/content/renderer/service...
content/renderer/service_worker/embedded_worker_dispatcher.cc:47:
dev_tools_agent_->OnMessageReceived(message);
On 2014/04/29 01:16:09, kinuko wrote:
> maybe DCHECK the return value (to see if it returns true)

Done in EmbeddedWorkerDispatcher::OnMessageToDevToolsAgent.

[horo, 2014-04-29 06:14:51]

nasko@
Could you please review embedded_worker_messages.h?

[jam, 2014-04-29 06:21:22]

https://codereview.chromium.org/251723002/diff/80001/content/common/service_w...
File content/common/service_worker/embedded_worker_messages.h (right):

https://codereview.chromium.org/251723002/diff/80001/content/common/service_w...
content/common/service_worker/embedded_worker_messages.h:44: IPC::Message /*
message */)
drive by: related to my previous comments about SW IPCs, there's no need to send
a nested IPC in a control IPC. just send the IPC that is nested directly, and
make it routed. the route id could be the embedded_worker_id

[horo, 2014-04-29 08:01:52]

On 2014/04/29 06:21:22, jam wrote:
>
https://codereview.chromium.org/251723002/diff/80001/content/common/service_w...
> File content/common/service_worker/embedded_worker_messages.h (right):
> 
>
https://codereview.chromium.org/251723002/diff/80001/content/common/service_w...
> content/common/service_worker/embedded_worker_messages.h:44: IPC::Message /*
> message */)
> drive by: related to my previous comments about SW IPCs, there's no need to
send
> a nested IPC in a control IPC. just send the IPC that is nested directly, and
> make it routed. the route id could be the embedded_worker_id

embedded_worker_id is not generated by RenderWidgetHelper::GetNextRoutingID.
So we should use worker_devtools_agent_route_id which will be introduces by
https://codereview.chromium.org/252633003/.

I rebased this cl with https://codereview.chromium.org/252633003/ and changed to
use this id for routing the messages.

[horo, 2014-04-29 08:03:09]

nasko@
Sorry I will remove you from the reviewers because embedded_worker_messages.h
was removed from this change.

[jam, 2014-04-29 16:04:32]

On 2014/04/29 08:01:52, horo wrote:
> On 2014/04/29 06:21:22, jam wrote:
> >
>
https://codereview.chromium.org/251723002/diff/80001/content/common/service_w...
> > File content/common/service_worker/embedded_worker_messages.h (right):
> > 
> >
>
https://codereview.chromium.org/251723002/diff/80001/content/common/service_w...
> > content/common/service_worker/embedded_worker_messages.h:44: IPC::Message /*
> > message */)
> > drive by: related to my previous comments about SW IPCs, there's no need to
> send
> > a nested IPC in a control IPC. just send the IPC that is nested directly,
and
> > make it routed. the route id could be the embedded_worker_id
> 
> embedded_worker_id is not generated by RenderWidgetHelper::GetNextRoutingID.
> So we should use worker_devtools_agent_route_id which will be introduces by
> https://codereview.chromium.org/252633003/.
> 
> I rebased this cl with https://codereview.chromium.org/252633003/ and changed
to
> use this id for routing the messages.

Do you need the functionality provided by devtools_agent_filter (i.e.
dispatching IPCs while the render thread is blocked?). I ask because before I
commented, you weren't using it. if you don't need it, then
EmbeddedWorkerDevToolsAgent can just register with RenderThread::AddRoute
instead of RenderThread::AddEmbeddedWorkerRoute. otherwise if you do, this
patchset is good.

to avoid delaying you anymore, lgtm with the above comment

Also, I agree that having EmbeddedWorkerDevToolsAgent register its route with
RenderThread is cleaner (with either method above). for future reference if you
need to, had you sent a routed IPC and dispatched it where you were before, that
would work as well since RenderThreadImpl passes all IPCs to it.

[horo, 2014-04-30 05:32:16]

On 2014/04/29 16:04:32, jam wrote:
> On 2014/04/29 08:01:52, horo wrote:
> > On 2014/04/29 06:21:22, jam wrote:
> > >
> >
>
https://codereview.chromium.org/251723002/diff/80001/content/common/service_w...
> > > File content/common/service_worker/embedded_worker_messages.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/251723002/diff/80001/content/common/service_w...
> > > content/common/service_worker/embedded_worker_messages.h:44: IPC::Message
/*
> > > message */)
> > > drive by: related to my previous comments about SW IPCs, there's no need
to
> > send
> > > a nested IPC in a control IPC. just send the IPC that is nested directly,
> and
> > > make it routed. the route id could be the embedded_worker_id
> > 
> > embedded_worker_id is not generated by RenderWidgetHelper::GetNextRoutingID.
> > So we should use worker_devtools_agent_route_id which will be introduces by
> > https://codereview.chromium.org/252633003/.
> > 
> > I rebased this cl with https://codereview.chromium.org/252633003/ and
changed
> to
> > use this id for routing the messages.
> 
> Do you need the functionality provided by devtools_agent_filter (i.e.
> dispatching IPCs while the render thread is blocked?). I ask because before I
> commented, you weren't using it. if you don't need it, then
> EmbeddedWorkerDevToolsAgent can just register with RenderThread::AddRoute
> instead of RenderThread::AddEmbeddedWorkerRoute. otherwise if you do, this
> patchset is good.
> 
> to avoid delaying you anymore, lgtm with the above comment
> 
> Also, I agree that having EmbeddedWorkerDevToolsAgent register its route with
> RenderThread is cleaner (with either method above). for future reference if
you
> need to, had you sent a routed IPC and dispatched it where you were before,
that
> would work as well since RenderThreadImpl passes all IPCs to it.

I call AddEmbeddedWorkerRoute not to handle
DevToolsAgentMsg_DispatchOnInspectorBackend messages for Service Workers in
DevToolsAgentFilter.

If AddEmbeddedWorkerRoute is not called, these messages is not handled correctly
by EmbeddedWorkerDevToolsAgent.

[horo, 2014-04-30 14:28:03]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-30 14:28:30]

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

[commit-bot: I haz the power, 2014-04-30 14:33:48]

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

[commit-bot: I haz the power, 2014-04-30 14:33:49]

Try jobs failed on following builders:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium

[horo, 2014-04-30 14:59:57]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-30 15:00:13]

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

[commit-bot: I haz the power, 2014-04-30 15:04:21]

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

[commit-bot: I haz the power, 2014-04-30 15:04:22]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[horo, 2014-04-30 15:18:09]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-30 15:18:55]

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

[commit-bot: I haz the power, 2014-04-30 15:21:40]

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

[commit-bot: I haz the power, 2014-04-30 15:21:41]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[horo, 2014-04-30 15:36:44]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-30 15:37:28]

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

[commit-bot: I haz the power, 2014-04-30 15:41:08]

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

[commit-bot: I haz the power, 2014-04-30 15:41:09]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[horo, 2014-04-30 22:24:34]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:26:40]

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

[commit-bot: I haz the power, 2014-04-30 22:39:23]

Change committed as 267363