Define DevTools content API

Define DevTools content API

The API consists of the following parts:
* DevToolsManager routes messages between devtools agents and clients
* DevToolsAgentHost provides an abstract interface to the debuggee, currently it is either RenderViewHost or Shared Worker. Client can obtain DevToolsAgentHost from DevToolsAgentHostRegistry.
* DevToolsClientHost is an API that should be implemented by DevTools front-end. There is a default Chromium implementation living in chrome/ and a remote debugging server which also implements this interface. Clients can extend it in order to provide custom front-end. There is a default DevTools front-end implementation and content/ provides a way for creating corresponding DevToolsClientHost by means of DevToolsClientHost::CreateDevToolsFrontendHost. The embedder just needs to provide a concrete delegate.
* This patch also removes DevToolsHost_ForwardToAgent and DevToolsHost_ForwardToClient IPC messages which were used to forward only one message. DevTools IPC messages are now hidden behind the devtools content API.


BUG=104625
TEST=Existing tests

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

[yurys, 2011-11-21 14:13:26]

We still need to figure out what to do with content/common/devtools_messages.h
If we don't want to expose DevTools IPC specifics through the API, we can add a
couple of methods on DevToolsClientHost to hide that details.

content/browser/debugger/devtools_http_protocol_handler.h should also has a
public API which I'm going to introduce in another change.

[yurys, 2011-11-21 14:15:46]

Note, that this patch includes changes from
http://codereview.chromium.org/8609010/ and once the latter is landed this patch
should shrink a bit.

[pfeldman, 2011-11-21 14:52:20]

I am not a fan of the GetInstance implementation on the devtools manager.
Otherwise, devtools lgtm.

http://codereview.chromium.org/8549022/diff/1/content/public/browser/devtools...
File content/public/browser/devtools/devtools_agent_host_registry.h (right):

http://codereview.chromium.org/8549022/diff/1/content/public/browser/devtools...
content/public/browser/devtools/devtools_agent_host_registry.h:19: static
DevToolsAgentHost* FindDevToolsAgentHost(RenderViewHost* rvh);
nit: Difference between Get and Find is not always clear. Could we add
HasDevToolsAgentHost instead of Find? Or add a boolean parameter
create_if_missing in GetDevToolsAgentHost.

http://codereview.chromium.org/8549022/diff/4001/content/browser/browser_main...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/8549022/diff/4001/content/browser/browser_main...
content/browser/browser_main_loop.cc:301:
NotificationService::current()->Notify(
Could we use a simple singleton instead?

http://codereview.chromium.org/8549022/diff/4001/content/browser/debugger/dev...
File content/browser/debugger/devtools_manager_impl.h (right):

http://codereview.chromium.org/8549022/diff/4001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_impl.h:111: int
DetachClientHost(RenderViewHost* from_rvh);
Could you leave comments to these methods?

http://codereview.chromium.org/8549022/diff/4001/content/public/browser/devto...
File content/public/browser/devtools/devtools_agent_host_registry.h (right):

http://codereview.chromium.org/8549022/diff/4001/content/public/browser/devto...
content/public/browser/devtools/devtools_agent_host_registry.h:23: // Returns
true iff an instance of DevToolsAgentHost for the |rvh|
Blank line before the comments.

[yurys, 2011-11-21 15:07:14]

http://codereview.chromium.org/8549022/diff/1/content/public/browser/devtools...
File content/public/browser/devtools/devtools_agent_host_registry.h (right):

http://codereview.chromium.org/8549022/diff/1/content/public/browser/devtools...
content/public/browser/devtools/devtools_agent_host_registry.h:19: static
DevToolsAgentHost* FindDevToolsAgentHost(RenderViewHost* rvh);
On 2011/11/21 14:52:21, pfeldman wrote:
> nit: Difference between Get and Find is not always clear. Could we add
> HasDevToolsAgentHost instead of Find? Or add a boolean parameter
> create_if_missing in GetDevToolsAgentHost.

It is already done(HasDevToolsAgentHost), this comment is for one of the early
versions of this patch.

http://codereview.chromium.org/8549022/diff/4001/content/browser/browser_main...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/8549022/diff/4001/content/browser/browser_main...
content/browser/browser_main_loop.cc:301:
NotificationService::current()->Notify(
On 2011/11/21 14:52:21, pfeldman wrote:
> Could we use a simple singleton instead?
As I wrote I'm going to resolve it in a separate change that should be landed
before this one.

http://codereview.chromium.org/8549022/diff/4001/content/browser/debugger/dev...
File content/browser/debugger/devtools_manager_impl.h (right):

http://codereview.chromium.org/8549022/diff/4001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_impl.h:111: int
DetachClientHost(RenderViewHost* from_rvh);
On 2011/11/21 14:52:21, pfeldman wrote:
> Could you leave comments to these methods?

Done.

http://codereview.chromium.org/8549022/diff/4001/content/public/browser/devto...
File content/public/browser/devtools/devtools_agent_host_registry.h (right):

http://codereview.chromium.org/8549022/diff/4001/content/public/browser/devto...
content/public/browser/devtools/devtools_agent_host_registry.h:23: // Returns
true iff an instance of DevToolsAgentHost for the |rvh|
On 2011/11/21 14:52:21, pfeldman wrote:
> Blank line before the comments.

Done.

[jam, 2011-11-21 21:16:04]

Thanks so much for working on this, it's really appreciated.

Please add rules in chrome/browser/DEPS to disallow devtools_messages.h (you can
uncomment the line I added) and also content/browser/debugger

http://codereview.chromium.org/8549022/diff/9001/content/browser/debugger/dev...
File content/browser/debugger/devtools_manager_impl.cc (right):

http://codereview.chromium.org/8549022/diff/9001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_impl.cc:26: using
content::DevToolsAgentHostRegistry;
nit: these using statements aren't needed anymore?

http://codereview.chromium.org/8549022/diff/9001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_impl.cc:124: delete this;
why do you care about deleting this object at shutdown? in general we're trying
to move to a world where we do as little as possible at shutdown, so that we can
quit as quickly as possible. it doesn't look like the destructor here does
anything important. NOTIFICATION_MAIN_MESSAGE_LOOP_EXITING wouldn't be needed
then.

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
File content/public/browser/devtools/devtools_agent_host_registry.h (right):

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_agent_host_registry.h:20: 
nit: unnecessary blank line

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_agent_host_registry.h:35: static bool
IsDebuggerAttached(TabContents*);
nit: chrome style is to name parameters in the headers

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
File content/public/browser/devtools/devtools_client_host.h (right):

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_client_host.h:63: virtual
RenderViewHost* GetClientRenderViewHost();
we've been trying to make all the interfaces in content/public to consist of
only pure virtual functions. can you make this one as well?

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_client_host.h:68: void
ForwardToDevToolsAgent(const IPC::Message& message);
is this actually used by anyone? i couldn't find it in searching (but i could be
mistaken).

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_client_host.h:72: void
NotifyCloseListener();
this and close_listener: can they be taken out of the interface (and change
set_close_listener to a pure virtual SetCloseListener)? we want to avoid member
variables on interfaces. this is really a one liner (call an interface), and you
can have the embedder implementations call this function in almost the same
lines of code that it takes them to call this function, so it doesn't seem worth
it to keep this functionality in the interface.

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_client_host.h:76:
DISALLOW_COPY_AND_ASSIGN(DevToolsClientHost);
nit: not needed

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
File content/public/browser/devtools/devtools_manager.h (right):

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_manager.h:22: class CONTENT_EXPORT
DevToolsManager {
nit: comment about what this class is used for

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_manager.h:26: virtual bool
ForwardToDevToolsAgent(DevToolsClientHost* from,
nit: comment (also 36)

[yurys, 2011-11-30 15:49:43]

http://codereview.chromium.org/8549022/diff/9001/content/browser/debugger/dev...
File content/browser/debugger/devtools_manager_impl.cc (right):

http://codereview.chromium.org/8549022/diff/9001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_impl.cc:26: using
content::DevToolsAgentHostRegistry;
On 2011/11/21 21:16:04, John Abd-El-Malek wrote:
> nit: these using statements aren't needed anymore?

Done.

http://codereview.chromium.org/8549022/diff/9001/content/browser/debugger/dev...
content/browser/debugger/devtools_manager_impl.cc:124: delete this;
On 2011/11/21 21:16:04, John Abd-El-Malek wrote:
> why do you care about deleting this object at shutdown? in general we're
trying
> to move to a world where we do as little as possible at shutdown, so that we
can
> quit as quickly as possible. it doesn't look like the destructor here does
> anything important. NOTIFICATION_MAIN_MESSAGE_LOOP_EXITING wouldn't be needed
> then.

Removed this method and all the stuff related to the notification service here.
It was actually a part of another change and I shouldn't have included it here,
sorry for the confusion.

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
File content/public/browser/devtools/devtools_agent_host_registry.h (right):

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_agent_host_registry.h:20: 
On 2011/11/21 21:16:04, John Abd-El-Malek wrote:
> nit: unnecessary blank line

Done.

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_agent_host_registry.h:35: static bool
IsDebuggerAttached(TabContents*);
On 2011/11/21 21:16:04, John Abd-El-Malek wrote:
> nit: chrome style is to name parameters in the headers

Done.

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
File content/public/browser/devtools/devtools_client_host.h (right):

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_client_host.h:63: virtual
RenderViewHost* GetClientRenderViewHost();
On 2011/11/21 21:16:04, John Abd-El-Malek wrote:
> we've been trying to make all the interfaces in content/public to consist of
> only pure virtual functions. can you make this one as well?

Done.

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_client_host.h:68: void
ForwardToDevToolsAgent(const IPC::Message& message);
On 2011/11/21 21:16:04, John Abd-El-Malek wrote:
> is this actually used by anyone? i couldn't find it in searching (but i could
be
> mistaken).

It is used only in DevToolsWindow::OnMessageReceived, inlined it there.

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_client_host.h:72: void
NotifyCloseListener();
On 2011/11/21 21:16:04, John Abd-El-Malek wrote:
> this and close_listener: can they be taken out of the interface (and change
> set_close_listener to a pure virtual SetCloseListener)? we want to avoid
member
> variables on interfaces. this is really a one liner (call an interface), and
you
> can have the embedder implementations call this function in almost the same
> lines of code that it takes them to call this function, so it doesn't seem
worth
> it to keep this functionality in the interface.

Done. Removed the CloseListener, there is no much sense in having it as we
expose abstract devtools client host interface. Clients can notify
DevToolsManager directly when they are closing.

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_client_host.h:76:
DISALLOW_COPY_AND_ASSIGN(DevToolsClientHost);
On 2011/11/21 21:16:04, John Abd-El-Malek wrote:
> nit: not needed

Done.

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
File content/public/browser/devtools/devtools_manager.h (right):

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_manager.h:22: class CONTENT_EXPORT
DevToolsManager {
On 2011/11/21 21:16:04, John Abd-El-Malek wrote:
> nit: comment about what this class is used for

Done.

http://codereview.chromium.org/8549022/diff/9001/content/public/browser/devto...
content/public/browser/devtools/devtools_manager.h:26: virtual bool
ForwardToDevToolsAgent(DevToolsClientHost* from,
On 2011/11/21 21:16:04, John Abd-El-Malek wrote:
> nit: comment (also 36)

Done.

[pfeldman, 2011-11-30 16:43:26]

Although the change looks good to me, it is getting hard to review it. Could we
please land it soon so that I don't have to go through it every time.

John, what are the requirements for getting lgtm from you on the changes to
content/ and chrome/browser/extensions? It is painful that the change is growing
so fast along the review process.

http://codereview.chromium.org/8549022/diff/19001/chrome/browser/debugger/dev...
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/8549022/diff/19001/chrome/browser/debugger/dev...
chrome/browser/debugger/devtools_window.cc:183:
ALLOW_THIS_IN_INITIALIZER_LIST(frontend_host_(
You could initialize it in the constructor instead.

http://codereview.chromium.org/8549022/diff/19001/chrome/browser/debugger/dev...
chrome/browser/debugger/devtools_window.cc:194: // 
content::SetupDevToolsFrontendDelegate(
remove these?

http://codereview.chromium.org/8549022/diff/19001/content/browser/debugger/de...
File content/browser/debugger/devtools_agent_host.h (right):

http://codereview.chromium.org/8549022/diff/19001/content/browser/debugger/de...
content/browser/debugger/devtools_agent_host.h:30: virtual void
SendMessageToAgent(IPC::Message* msg) = 0;
This method should be protected.

[yurys, 2011-11-30 17:08:28]

http://codereview.chromium.org/8549022/diff/19001/chrome/browser/debugger/dev...
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/8549022/diff/19001/chrome/browser/debugger/dev...
chrome/browser/debugger/devtools_window.cc:183:
ALLOW_THIS_IN_INITIALIZER_LIST(frontend_host_(
On 2011/11/30 16:43:26, pfeldman wrote:
> You could initialize it in the constructor instead.

Done.

http://codereview.chromium.org/8549022/diff/19001/chrome/browser/debugger/dev...
chrome/browser/debugger/devtools_window.cc:194: // 
content::SetupDevToolsFrontendDelegate(
On 2011/11/30 16:43:26, pfeldman wrote:
> remove these?

Done.

http://codereview.chromium.org/8549022/diff/19001/content/browser/debugger/de...
File content/browser/debugger/devtools_agent_host.h (right):

http://codereview.chromium.org/8549022/diff/19001/content/browser/debugger/de...
content/browser/debugger/devtools_agent_host.h:30: virtual void
SendMessageToAgent(IPC::Message* msg) = 0;
On 2011/11/30 16:43:26, pfeldman wrote:
> This method should be protected.

Done.

[jam, 2011-12-01 03:51:47]

lgtm with removing the cc file

I was just waiting for the review comments to be addressed. sorry this has grown
so big and taken a while. for the future, in general this could be addressed by
breaking it up, which would also make reviewing much easier :)

http://codereview.chromium.org/8549022/diff/24001/content/public/browser/devt...
File content/public/browser/devtools_client_host.cc (right):

http://codereview.chromium.org/8549022/diff/24001/content/public/browser/devt...
content/public/browser/devtools_client_host.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
nit: remove this file

http://codereview.chromium.org/8549022/diff/24001/content/public/browser/devt...
File content/public/browser/devtools_client_host.h (right):

http://codereview.chromium.org/8549022/diff/24001/content/public/browser/devt...
content/public/browser/devtools_client_host.h:30: DevToolsClientHost();
nit: we avoid constructors on interfaces

http://codereview.chromium.org/8549022/diff/24001/content/public/browser/devt...
content/public/browser/devtools_client_host.h:31: virtual ~DevToolsClientHost();
nit: convention for the content API is to inline the virtual destructor, that
way you don't need a .cc file for the interface

[yurys, 2011-12-01 15:20:06]

http://codereview.chromium.org/8549022/diff/24001/content/public/browser/devt...
File content/public/browser/devtools_client_host.cc (right):

http://codereview.chromium.org/8549022/diff/24001/content/public/browser/devt...
content/public/browser/devtools_client_host.cc:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/12/01 03:51:47, John Abd-El-Malek wrote:
> nit: remove this file

Done.

http://codereview.chromium.org/8549022/diff/24001/content/public/browser/devt...
File content/public/browser/devtools_client_host.h (right):

http://codereview.chromium.org/8549022/diff/24001/content/public/browser/devt...
content/public/browser/devtools_client_host.h:30: DevToolsClientHost();
On 2011/12/01 03:51:47, John Abd-El-Malek wrote:
> nit: we avoid constructors on interfaces

Done.

http://codereview.chromium.org/8549022/diff/24001/content/public/browser/devt...
content/public/browser/devtools_client_host.h:31: virtual ~DevToolsClientHost();
On 2011/12/01 03:51:47, John Abd-El-Malek wrote:
> nit: convention for the content API is to inline the virtual destructor, that
> way you don't need a .cc file for the interface

Done.