Introduce worker_devtools_agent_route_id for EmbeddedWorker.

Introduce worker_devtools_agent_route_id for EmbeddedWorker.

worker_devtools_agent_route_id is created with RenderProcessHostImpl::GetNextRoutingIDForProcess().
The worker_devtools_agent_route_id is sent to the renderer wrapped in EmbeddedWorkerMsg_StartWorker message and used to call DevToolsClientMsg_DispatchOnInspectorFrontend and DevToolsHostMsg_SaveAgentRuntimeState in EmbeddedWorkerContextClient.

These messages will be handled by SharedWorkerDevToolsManager::WorkerDevToolsAgentHost in the browser process.

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

[horo, 2014-04-25 02:25:26]

kinuko@
Could you please review?

[michaeln, 2014-04-25 02:53:14]

https://codereview.chromium.org/252633003/diff/1/content/browser/service_work...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/252633003/diff/1/content/browser/service_work...
content/browser/service_worker/embedded_worker_registry.cc:45: new
EmbeddedWorkerMsg_StartWorker(*worker_route_id,
Can we not route/identify based on the existing embedded_worker_id_? I'm curious
about how this new id is used and why it has to be in the same process specific
id-space as that defined by MessagePortMessageFilter?

[horo, 2014-04-25 04:34:43]

https://codereview.chromium.org/252633003/diff/1/content/browser/service_work...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/252633003/diff/1/content/browser/service_work...
content/browser/service_worker/embedded_worker_registry.cc:45: new
EmbeddedWorkerMsg_StartWorker(*worker_route_id,
On 2014/04/25 02:53:15, michaeln wrote:
> Can we not route/identify based on the existing embedded_worker_id_? I'm
curious
> about how this new id is used and why it has to be in the same process
specific
> id-space as that defined by MessagePortMessageFilter?

I want to reuse the architecture of DevTools for SharedWorker.
And I want to use this worker_route_id to handle DevToolsClient messsages in
SharedWorkerDevToolsManager::SharedWorkerDevToolsAgentHost.
(I will rename these classes later.)
SharedWorkerDevToolsAgentHost is registered to RenderProcessHostImpl's
listeners_ with the route_id.
But if we can't use route_id for EmbeddedWorker, we have to route the message in
another way.

[kinuko, 2014-04-25 10:37:06]

On 2014/04/25 04:34:43, horo wrote:
>
https://codereview.chromium.org/252633003/diff/1/content/browser/service_work...
> File content/browser/service_worker/embedded_worker_registry.cc (right):
> 
>
https://codereview.chromium.org/252633003/diff/1/content/browser/service_work...
> content/browser/service_worker/embedded_worker_registry.cc:45: new
> EmbeddedWorkerMsg_StartWorker(*worker_route_id,
> On 2014/04/25 02:53:15, michaeln wrote:
> > Can we not route/identify based on the existing embedded_worker_id_? I'm
> curious
> > about how this new id is used and why it has to be in the same process
> specific
> > id-space as that defined by MessagePortMessageFilter?
> 
> I want to reuse the architecture of DevTools for SharedWorker.
> And I want to use this worker_route_id to handle DevToolsClient messsages in
> SharedWorkerDevToolsManager::SharedWorkerDevToolsAgentHost.
> (I will rename these classes later.)
> SharedWorkerDevToolsAgentHost is registered to RenderProcessHostImpl's
> listeners_ with the route_id.
> But if we can't use route_id for EmbeddedWorker, we have to route the message
in
> another way.

I chatted w/ horo, and I'm basically convinced to have a different ID in this
message. (Sounds like we can also work around it but it'd need a lot more
additional code)

A few comments:
- Let's use a better, more descriptive name rather than worker_id.
- I'd like to keep embedded_worker_id the first field of the message.
- Get-next-routing-ID callback stuff should be cleaned up, the current code
looks cryptic enough. It could be also given from RenderProcessHostImpl when
ServiceWorkerDispatcherHost's created like MessagePortMessageFilter does.

[horo, 2014-04-25 10:39:31]

On 2014/04/25 04:34:43, horo wrote:
>
https://codereview.chromium.org/252633003/diff/1/content/browser/service_work...
> File content/browser/service_worker/embedded_worker_registry.cc (right):
> 
>
https://codereview.chromium.org/252633003/diff/1/content/browser/service_work...
> content/browser/service_worker/embedded_worker_registry.cc:45: new
> EmbeddedWorkerMsg_StartWorker(*worker_route_id,
> On 2014/04/25 02:53:15, michaeln wrote:
> > Can we not route/identify based on the existing embedded_worker_id_? I'm
> curious
> > about how this new id is used and why it has to be in the same process
> specific
> > id-space as that defined by MessagePortMessageFilter?
> 
> I want to reuse the architecture of DevTools for SharedWorker.
> And I want to use this worker_route_id to handle DevToolsClient messsages in
> SharedWorkerDevToolsManager::SharedWorkerDevToolsAgentHost.
> (I will rename these classes later.)
> SharedWorkerDevToolsAgentHost is registered to RenderProcessHostImpl's
> listeners_ with the route_id.
> But if we can't use route_id for EmbeddedWorker, we have to route the message
in
> another way.

kinuko@
As we talked offline. I made the following changes and uploaded "patch set 2"
- Create NextRoutingIDCallback from widget_helper_ in RenderProcessHostImpl.
- Rename worker_route_id to worker_devtools_agent_route_id and change the order
in EmbeddedWorkerMsg_StartWorker and methods.

[kinuko, 2014-04-25 13:56:57]

https://codereview.chromium.org/252633003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/252633003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.h:34: const
base::Callback<int(void)>& next_routing_id_callback);
The new parameter needs a descriptive comment.

https://codereview.chromium.org/252633003/diff/60001/content/browser/service_...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/252633003/diff/60001/content/browser/service_...
content/browser/service_worker/embedded_worker_registry.cc:154:
process_next_id_callback_map_.insert(std::make_pair(process_id, callback));
It might be nicer if AddChildProcessSender() can just take an interface (which
could be implemented by DispatcherHost) like:

public ProcessHost : public IPC::Sender {
  // Returns a unique routing ID for the (renderer) process.
  virtual int NextRoutingID() const = 0;
};

and these {Add,Remove}ChildProcessNextRoutingIDCallback can be subsumed into it?
 (These method names and what they do look a bit cryptic without description)

[michaeln, 2014-04-25 19:23:46]

Thnx, now i see why it must be in an id-space that shared with others.

I have a gripe with the generic use of the term "route_id" in the project. It
means too many different things in too many different contexts and its very
difficult to understand any given usage of that term. In this case, it's being
used as a "RenderProcessHostIPCListnerRouteID", but a reader of the code would
not easily figure that out (its well beyond being just a little obscure). The
callback plumbing is whacky looking inscrutableness, all just to get to the same
AtomicSequenceNumber instance buried in RenderWidgetHelper. Something like this
is a whole lot more grokable...

// The SharedWorkerDevToolsAgentHost is listener of RenderProcessHost ipcs,
// it needs a route id to eaves drop on this embedded worker.
int magic_route_id =
RenderWidgetHelper::FromProcessHostID(process_id)->GetNextRoutingID();

That works provided RenderProcessHost::GetNextRoutingID() is guaranteed to be
implemented in terms of RenderWidgetHelper.

[michaeln, 2014-04-25 19:29:49]

Ah... how it's arranged in snapshot 3 is much nicer then 1!

https://codereview.chromium.org/252633003/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.h (right):

https://codereview.chromium.org/252633003/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.h:34: const
base::Callback<int(void)>& next_routing_id_callback);
On 2014/04/25 13:56:57, kinuko wrote:
> The new parameter needs a descriptive comment.

descriptive name would be even better

https://codereview.chromium.org/252633003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/252633003/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:766:
base::Bind(&RenderWidgetHelper::GetNextRoutingID, widget_helper_));
ah... this is nice

[michaeln, 2014-04-25 20:10:33]

https://codereview.chromium.org/252633003/diff/60001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/252633003/diff/60001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:766:
base::Bind(&RenderWidgetHelper::GetNextRoutingID, widget_helper_));
Here's a thought? Define a new static method and nix the abstraction layer by
way of bind closure and map book keeping. Less and more understandable code.
Wdyt?

// static
int RenderProcessHostImpl::GetNextRouteIDForProcess(process_id)  {
  RenderWidgetHelper* helper =
RenderWidgetHelper::FromProcessHostID(process_id);
  if (!helper)
    return -1;  // Maybe null in unittest.
  return helper->GetNextRoutingID();
}

[michaeln, 2014-04-25 20:26:50]

Also hust noticing there's also a static RenderProcessHost* FromID(int
render_process_id) method. Anyway, it looks like it could be straightforward to
get these IDs generated w/o all the closure plumbing.

[horo, 2014-04-26 06:33:21]

GetNextRouteIDForProcess is a good idea!

RenderProcessHost::FromID must be called on UI thread.
So I added RenderProcessHostImpl::GetNextRoutingIDForProcess.

[kinuko, 2014-04-28 04:31:29]

Didn't know we have a convenient static method to get RenderWidgetHelper that
can be called on IO thread, that's nice. Some more nits:

https://codereview.chromium.org/252633003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/252633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:472:
RenderWidgetHelper* helper =
Would be safer to receive this as scoped_refptr

https://codereview.chromium.org/252633003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/252633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:225: static int
GetNextRoutingIDForProcess(int render_process_id);
Per comment in RenderWidgetHelper::FromProcessHostID this needs to be called on
IO thread, while most (if not all) of public RenderProcessHostImpl methods are
supposed to be called on UI thread. Would be definitely good to comment about
that.. and probably it could also note that if the caller is on UI thread it can
directly call GetNextRoutingID().

[horo, 2014-04-28 04:51:22]

https://codereview.chromium.org/252633003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/252633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:472:
RenderWidgetHelper* helper =
On 2014/04/28 04:31:29, kinuko wrote:
> Would be safer to receive this as scoped_refptr

Done.

https://codereview.chromium.org/252633003/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/252633003/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:225: static int
GetNextRoutingIDForProcess(int render_process_id);
On 2014/04/28 04:31:29, kinuko wrote:
> Per comment in RenderWidgetHelper::FromProcessHostID this needs to be called
on
> IO thread, while most (if not all) of public RenderProcessHostImpl methods are
> supposed to be called on UI thread. Would be definitely good to comment about
> that.. and probably it could also note that if the caller is on UI thread it
can
> directly call GetNextRoutingID().

Done.

[kinuko, 2014-04-28 07:09:52]

lgtm

https://codereview.chromium.org/252633003/diff/90016/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/252633003/diff/90016/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:223: // Gets the next
available routing id of the proccess on IO thread.
Please explicitly note that this method needs to be called on IO thread, this
comment sounds still unclear about what 'on IO thread' qualifies.

[horo, 2014-04-28 07:29:41]

https://codereview.chromium.org/252633003/diff/90016/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/252633003/diff/90016/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.h:223: // Gets the next
available routing id of the proccess on IO thread.
On 2014/04/28 07:09:52, kinuko wrote:
> Please explicitly note that this method needs to be called on IO thread, this
> comment sounds still unclear about what 'on IO thread' qualifies.

Done.

[michaeln, 2014-04-29 00:24:13]

thnx for nix'ing the routeidcloser plumbing
before nix'age: (+138 lines, -35 lines)
after nix'age: (+82 lines, -27 lines)
nice!

[horo, 2014-04-29 00:47:43]

sky@
Could you please review
content/browser/renderer_host/render_process_host_impl.*?

[horo, 2014-04-29 00:48:24]

nasko@
Could you please review embedded_worker_messages.h?

[sky, 2014-04-29 04:17:38]

Sorry, I'm not familiar with RenderWidgetHelper. Maybe John?
sky->jam

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

lgtm with comment: inline the RPH method into the calling site

https://codereview.chromium.org/252633003/diff/190001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/252633003/diff/190001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:226: static int
GetNextRoutingIDForProcess(int render_process_id);
i don't see the point of adding this method on RenderProcessHostImpl for two
reasons:
-all the methods are called on the UI threads, so it's better to keep the class
used on one thread only to avoid confusion
-this method doesn't use any internal RPH state, and it's called in one place,
so just move that implementation there

[horo, 2014-04-29 06:54:38]

https://codereview.chromium.org/252633003/diff/190001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/252633003/diff/190001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.h:226: static int
GetNextRoutingIDForProcess(int render_process_id);
On 2014/04/29 06:28:22, jam wrote:
> i don't see the point of adding this method on RenderProcessHostImpl for two
> reasons:
> -all the methods are called on the UI threads, so it's better to keep the
class
> used on one thread only to avoid confusion
> -this method doesn't use any internal RPH state, and it's called in one place,
> so just move that implementation there

Done.

[nasko, 2014-04-29 14:33:02]

IPC LGTM

[horo, 2014-04-29 17:02:26]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-29 17:03:26]

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

[commit-bot: I haz the power, 2014-04-29 17:03:45]

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

[commit-bot: I haz the power, 2014-04-29 17:03:46]

Failed to apply patch for
content/browser/service_worker/embedded_worker_instance.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/service_worker/embedded_worker_instance.cc
  Hunk #1 FAILED at 28.
  Hunk #2 succeeded at 75 with fuzz 1 (offset 5 lines).
  Hunk #3 succeeded at 109 (offset 15 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
content/browser/service_worker/embedded_worker_instance.cc.rej

Patch:       content/browser/service_worker/embedded_worker_instance.cc
Index: content/browser/service_worker/embedded_worker_instance.cc
diff --git a/content/browser/service_worker/embedded_worker_instance.cc
b/content/browser/service_worker/embedded_worker_instance.cc
index
6ee4356e6797705b5d6cb933b6bcf95570ebec79..09bd0478aacbec1b89758d9349077a4cc3c0181c
100644
--- a/content/browser/service_worker/embedded_worker_instance.cc
+++ b/content/browser/service_worker/embedded_worker_instance.cc
@@ -28,10 +28,12 @@ ServiceWorkerStatusCode EmbeddedWorkerInstance::Start(
                              embedded_worker_id_,
                              service_worker_version_id,
                              scope,
-                             script_url);
+                             script_url,
+                             &worker_devtools_agent_route_id_);
   if (status != SERVICE_WORKER_OK) {
     status_ = STOPPED;
     process_id_ = -1;
+    worker_devtools_agent_route_id_ = MSG_ROUTING_NONE;
   }
   return status;
 }
@@ -70,14 +72,14 @@ void EmbeddedWorkerInstance::ReleaseProcessReference(int
process_id) {
     process_refs_.erase(found);
 }
 
-EmbeddedWorkerInstance::EmbeddedWorkerInstance(
-    EmbeddedWorkerRegistry* registry,
-    int embedded_worker_id)
+EmbeddedWorkerInstance::EmbeddedWorkerInstance(EmbeddedWorkerRegistry*
registry,
+                                               int embedded_worker_id)
     : registry_(registry),
       embedded_worker_id_(embedded_worker_id),
       status_(STOPPED),
       process_id_(-1),
-      thread_id_(-1) {
+      thread_id_(-1),
+      worker_devtools_agent_route_id_(MSG_ROUTING_NONE) {
 }
 
 void EmbeddedWorkerInstance::OnStarted(int thread_id) {
@@ -94,6 +96,7 @@ void EmbeddedWorkerInstance::OnStopped() {
   status_ = STOPPED;
   process_id_ = -1;
   thread_id_ = -1;
+  worker_devtools_agent_route_id_ = MSG_ROUTING_NONE;
   FOR_EACH_OBSERVER(Listener, listener_list_, OnStopped());
 }

[horo, 2014-04-29 23:04:54]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-29 23:07:51]

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

[horo, 2014-04-29 23:11:10]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-04-30 01:13:02]

The CQ bit was checked by horo@chromium.org

[horo, 2014-04-30 01:13:03]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-04-30 01:24:13]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-30 01:27:16]

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

[horo, 2014-04-30 01:39:43]

The CQ bit was unchecked by horo@chromium.org

[commit-bot: I haz the power, 2014-04-30 01:41:06]

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

[commit-bot: I haz the power, 2014-04-30 01:41:06]

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

[horo, 2014-04-30 02:39:23]

The CQ bit was checked by horo@chromium.org

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

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

[horo, 2014-04-30 02:41:09]

The CQ bit was unchecked by horo@chromium.org

[horo, 2014-04-30 03:21:25]

The CQ bit was checked by horo@chromium.org

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

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

[commit-bot: I haz the power, 2014-04-30 10:59:17]

Change committed as 267173