DevTools: devtools message plumbing between worker and page processes

This change provides initial support for sending DevTools messages between Worker and Page processes.

On the Page side devtools messages are handled by WorkerDevtoolsAgentProxy. It implements WebWorkerBase::DevToolsDelegate interface which isolates worker stuff under content/ from DevTools specifics.

In the worker process it's WorkerDevtoolsAgent that is responsible for sending/receiving devtools messages. It implements WebWorkerStub::DevToolsDelegate and WebWorkerClientProxy::DevToolsDelegate which insulate worker stuff under content/ from the devtools implementation details.

WorkerDevtoolsAgentProxy and WorkerDevtoolsAgent are counterparts of WebWorkerProxy and WebWorkerStub. Since devtools is not a part of HTML5 the new classes live under chrome/

ContentWorkerClient is introduced for notifying embedder about certain events when worker clients can be installed.

BUG=13684
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86991

[jam, 2011-05-24 20:21:31]

Hi, I started reviewing this but then I hit the fact that even though the actual
implementation is in chrome, content still has the devtools interface
(WebWorkerBase::DevToolsDelegate).  If a feature is in chrome, then it shouldn't
have parts specific to it in content.  i.e. there could be a generic interface
for observing certain events, and the embedder (i.e. chrome) can choose to do
devtools specific actions based on them.  but there's no point having the dev
tools code in chrome and also interfaces for it in content.

Darin has convinced me that the devtools code should be in the content layer,
not chrome.  The reasons are that if we expect webkit developers to use the
multi-process test shell as a debugging and testing platform (instead of
chrome), then they will want to use the debugger.

I realize this creates some more work for you, but I think this is probably the
better approach.  Choosing the initial set of features to leave in chrome wasn't
an exact science, so we're moving some stuff back and forth as we finalize the
split.  So for you, if you can leave the code entirely in content that'd be
good.  For the larger dev tools team, we'll need people to move
chrome\browser\devtools to content\browser\devtools and figure out how to tease
out the extensions dependency etc (extensions are a chrome feature).

http://codereview.chromium.org/6990059/diff/1/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/6990059/diff/1/chrome/app/chrome_main.cc#newco...
chrome/app/chrome_main.cc:232: InitializeChromeContentWorkerClient();
so far, by design we only have one process getter in the ContentClient interface
that's non NULL at a time (other than single process mode of course).

We supported (in theory only) creating WebWorkerProxy in the worker in case of
workers creating other workers.  But we never actually implemented it.  Once
dedicated workers run in the renderer, this case won't even matter.  So
basically, we should only have ContentWorkerClient in the worker process.

http://codereview.chromium.org/6990059/diff/1/chrome/chrome_worker.gypi
File chrome/chrome_worker.gypi (right):

http://codereview.chromium.org/6990059/diff/1/chrome/chrome_worker.gypi#newco...
chrome/chrome_worker.gypi:10: 'msvs_guid':
'6024324B-4874-45ac-9447-B964DEA05BFC',
nit: guids aren't needed anymore

http://codereview.chromium.org/6990059/diff/1/chrome/renderer/worker_devtools...
File chrome/renderer/worker_devtools_agent_proxy.h (right):

http://codereview.chromium.org/6990059/diff/1/chrome/renderer/worker_devtools...
chrome/renderer/worker_devtools_agent_proxy.h:9: #include <string>
nit: probably not needed

http://codereview.chromium.org/6990059/diff/1/chrome/renderer/worker_devtools...
chrome/renderer/worker_devtools_agent_proxy.h:15: class Message;
nit: not needed

http://codereview.chromium.org/6990059/diff/1/chrome/renderer/worker_devtools...
chrome/renderer/worker_devtools_agent_proxy.h:22: class WebWorkerBase;
niht: not needed

http://codereview.chromium.org/6990059/diff/1/chrome/worker/chrome_content_wo...
File chrome/worker/chrome_content_worker_client.h (right):

http://codereview.chromium.org/6990059/diff/1/chrome/worker/chrome_content_wo...
chrome/worker/chrome_content_worker_client.h:12: class WebWorkerClient;
nit: not needed

http://codereview.chromium.org/6990059/diff/1/content/worker/content_worker_c...
File content/worker/content_worker_client.h (right):

http://codereview.chromium.org/6990059/diff/1/content/worker/content_worker_c...
content/worker/content_worker_client.h:25: virtual void
WebWorkerProxyCreated(WebWorkerBase*, int route_id,
nit comment

[Dmitry Titov, 2011-05-25 01:38:43]

Note dependency check failing on the try bot, and also couple tiny nits:

http://codereview.chromium.org/6990059/diff/1/content/renderer/webworker_base.cc
File content/renderer/webworker_base.cc (right):

http://codereview.chromium.org/6990059/diff/1/content/renderer/webworker_base...
content/renderer/webworker_base.cc:58:
devtools_delegate_->SetRouteId(route_id_);
If MSG_ROUTING_NONE was used directly here, it'd be more readable.

http://codereview.chromium.org/6990059/diff/1/content/worker/content_worker_c...
File content/worker/content_worker_client.h (right):

http://codereview.chromium.org/6990059/diff/1/content/worker/content_worker_c...
content/worker/content_worker_client.h:25: virtual void
WebWorkerProxyCreated(WebWorkerBase*, int route_id,
On 2011/05/24 20:21:31, John Abd-El-Malek wrote:
> nit comment

I'd remove the comment on the previous method - it doesn't really say more then
the name of the method...

http://codereview.chromium.org/6990059/diff/1/content/worker/webworker_stub.h
File content/worker/webworker_stub.h (right):

http://codereview.chromium.org/6990059/diff/1/content/worker/webworker_stub.h...
content/worker/webworker_stub.h:9: #include "base/scoped_ptr.h"
not used.

[yurys, 2011-05-25 17:28:29]

I moved devtools_messages.h from chrome/common/ to content/common/ and put all
new worker devtools classes under content/ as John suggested. Please take
another look at the change. In particular, I have some doubts about using
WebWorkerBase::DevToolsDelegate interface vs.  WorkerDevToolsAgentProxy
directly.

http://codereview.chromium.org/6990059/diff/1/chrome/app/chrome_main.cc
File chrome/app/chrome_main.cc (right):

http://codereview.chromium.org/6990059/diff/1/chrome/app/chrome_main.cc#newco...
chrome/app/chrome_main.cc:232: InitializeChromeContentWorkerClient();
On 2011/05/24 20:21:31, John Abd-El-Malek wrote:
> so far, by design we only have one process getter in the ContentClient
interface
> that's non NULL at a time (other than single process mode of course).
> 
> We supported (in theory only) creating WebWorkerProxy in the worker in case of
> workers creating other workers.  But we never actually implemented it.  Once
> dedicated workers run in the renderer, this case won't even matter.  So
> basically, we should only have ContentWorkerClient in the worker process.

I deleted ContentWorkerClient completely since it's not needed if devtools code
moves to content/.

http://codereview.chromium.org/6990059/diff/1/chrome/chrome_worker.gypi
File chrome/chrome_worker.gypi (right):

http://codereview.chromium.org/6990059/diff/1/chrome/chrome_worker.gypi#newco...
chrome/chrome_worker.gypi:10: 'msvs_guid':
'6024324B-4874-45ac-9447-B964DEA05BFC',
On 2011/05/24 20:21:31, John Abd-El-Malek wrote:
> nit: guids aren't needed anymore
Deleted this file along with chrome/worker/ directory.

http://codereview.chromium.org/6990059/diff/1/chrome/renderer/worker_devtools...
File chrome/renderer/worker_devtools_agent_proxy.h (right):

http://codereview.chromium.org/6990059/diff/1/chrome/renderer/worker_devtools...
chrome/renderer/worker_devtools_agent_proxy.h:9: #include <string>
On 2011/05/24 20:21:31, John Abd-El-Malek wrote:
> nit: probably not needed
This file has changed, please look at it again.

http://codereview.chromium.org/6990059/diff/1/content/worker/content_worker_c...
File content/worker/content_worker_client.h (right):

http://codereview.chromium.org/6990059/diff/1/content/worker/content_worker_c...
content/worker/content_worker_client.h:25: virtual void
WebWorkerProxyCreated(WebWorkerBase*, int route_id,
On 2011/05/25 01:38:44, Dmitry Titov wrote:
> On 2011/05/24 20:21:31, John Abd-El-Malek wrote:
> > nit comment
> 
> I'd remove the comment on the previous method - it doesn't really say more
then
> the name of the method... 

This file has been removed.

http://codereview.chromium.org/6990059/diff/1/content/worker/webworker_stub.h
File content/worker/webworker_stub.h (right):

http://codereview.chromium.org/6990059/diff/1/content/worker/webworker_stub.h...
content/worker/webworker_stub.h:9: #include "base/scoped_ptr.h"
On 2011/05/25 01:38:44, Dmitry Titov wrote:
> not used.

In the new version it is used.

[jam, 2011-05-25 17:44:09]

On 2011/05/25 17:28:29, Yury Semikhatsky wrote:
> I moved devtools_messages.h from chrome/common/ to content/common/ and put all
> new worker devtools classes under content/ as John suggested. Please take
> another look at the change. In particular, I have some doubts about using
> WebWorkerBase::DevToolsDelegate interface vs.  WorkerDevToolsAgentProxy
> directly.


Agreed, no need for WebWorkerBase::DevToolsDelegate.

[jam, 2011-05-25 17:50:11]

http://codereview.chromium.org/6990059/diff/7001/content/renderer/webworker_p...
File content/renderer/webworker_proxy.cc (right):

http://codereview.chromium.org/6990059/diff/7001/content/renderer/webworker_p...
content/renderer/webworker_proxy.cc:95: {
nit: brace brackets on previous line

http://codereview.chromium.org/6990059/diff/7001/content/worker/webworkerclie...
File content/worker/webworkerclient_proxy.cc (right):

http://codereview.chromium.org/6990059/diff/7001/content/worker/webworkerclie...
content/worker/webworkerclient_proxy.cc:47: devtools_delegate_(0) {
nit: chrome style is to use NULL for pointers

http://codereview.chromium.org/6990059/diff/7001/content/worker/webworkerclie...
content/worker/webworkerclient_proxy.cc:182: void
WebWorkerClientProxy::set_devtools_delegate(DevToolsDelegate* delegate) {
note: chrome style is to put simple getters/setters in the header

http://codereview.chromium.org/6990059/diff/7001/content/worker/worker_devtoo...
File content/worker/worker_devtools_agent.cc (right):

http://codereview.chromium.org/6990059/diff/7001/content/worker/worker_devtoo...
content/worker/worker_devtools_agent.cc:10: #include "ipc/ipc_message_macros.h"
no need to include these two ipc headers.  they're always included when
including a message file

http://codereview.chromium.org/6990059/diff/7001/content/worker/worker_devtoo...
content/worker/worker_devtools_agent.cc:12: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebString.h"
no need?

http://codereview.chromium.org/6990059/diff/7001/content/worker/worker_devtoo...
content/worker/worker_devtools_agent.cc:21: , webworker_(webworker) {
comma on previous line

please check the chrome style guide, it's different from the webkit one :)

http://codereview.chromium.org/6990059/diff/7001/content/worker/worker_devtoo...
content/worker/worker_devtools_agent.cc:58: {
nit: brace bracket on previous line

[yurys, 2011-05-26 09:11:49]

All comments addressed. WebWorkerBase::DevToolsDelegate and
WebWorkerClientProxy::DevToolsDelegate were removed in favor of using
WorkerDevToolsAgent directly.

http://codereview.chromium.org/6990059/diff/7001/content/renderer/webworker_p...
File content/renderer/webworker_proxy.cc (right):

http://codereview.chromium.org/6990059/diff/7001/content/renderer/webworker_p...
content/renderer/webworker_proxy.cc:95: {
On 2011/05/25 17:50:12, John Abd-El-Malek wrote:
> nit: brace brackets on previous line

Done.

http://codereview.chromium.org/6990059/diff/7001/content/worker/webworkerclie...
File content/worker/webworkerclient_proxy.cc (right):

http://codereview.chromium.org/6990059/diff/7001/content/worker/webworkerclie...
content/worker/webworkerclient_proxy.cc:47: devtools_delegate_(0) {
On 2011/05/25 17:50:12, John Abd-El-Malek wrote:
> nit: chrome style is to use NULL for pointers

Done.

http://codereview.chromium.org/6990059/diff/7001/content/worker/webworkerclie...
content/worker/webworkerclient_proxy.cc:182: void
WebWorkerClientProxy::set_devtools_delegate(DevToolsDelegate* delegate) {
On 2011/05/25 17:50:12, John Abd-El-Malek wrote:
> note: chrome style is to put simple getters/setters in the header

Done.

http://codereview.chromium.org/6990059/diff/7001/content/worker/worker_devtoo...
File content/worker/worker_devtools_agent.cc (right):

http://codereview.chromium.org/6990059/diff/7001/content/worker/worker_devtoo...
content/worker/worker_devtools_agent.cc:10: #include "ipc/ipc_message_macros.h"
On 2011/05/25 17:50:12, John Abd-El-Malek wrote:
> no need to include these two ipc headers.  they're always included when
> including a message file

Done.

http://codereview.chromium.org/6990059/diff/7001/content/worker/worker_devtoo...
content/worker/worker_devtools_agent.cc:12: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebString.h"
On 2011/05/25 17:50:12, John Abd-El-Malek wrote:
> no need?

It's used in WorkerDevToolsAgent::SendDevToolsMessage.

http://codereview.chromium.org/6990059/diff/7001/content/worker/worker_devtoo...
content/worker/worker_devtools_agent.cc:21: , webworker_(webworker) {
On 2011/05/25 17:50:12, John Abd-El-Malek wrote:
> comma on previous line
> 
> please check the chrome style guide, it's different from the webkit one :)

Yeah, sorry about all these style errors, I definitely mixed Chromium and WebKit
styles.

http://codereview.chromium.org/6990059/diff/7001/content/worker/worker_devtoo...
content/worker/worker_devtools_agent.cc:58: {
On 2011/05/25 17:50:12, John Abd-El-Malek wrote:
> nit: brace bracket on previous line

Done.

[jam, 2011-05-26 18:28:53]

lgtm

http://codereview.chromium.org/6990059/diff/11001/content/renderer/worker_dev...
File content/renderer/worker_devtools_agent_proxy.h (right):

http://codereview.chromium.org/6990059/diff/11001/content/renderer/worker_dev...
content/renderer/worker_devtools_agent_proxy.h:39: 
nit: extra line

[yurys, 2011-05-27 07:45:43]

http://codereview.chromium.org/6990059/diff/11001/content/renderer/worker_dev...
File content/renderer/worker_devtools_agent_proxy.h (right):

http://codereview.chromium.org/6990059/diff/11001/content/renderer/worker_dev...
content/renderer/worker_devtools_agent_proxy.h:39: 
On 2011/05/26 18:28:53, John Abd-El-Malek wrote:
> nit: extra line

Done.