Add developer tools message forwarding to browser

Developer tools messages are forwarded as wrapped IPC::Messages(previously they were represented as int type+string body).

I stole IPC::Message serialization code from jam's CL(http://codereview.chromium.org/20413).

jam: please look at message forwarding code. When tools messages are send from browser to renderer they are also wrapped(unlike worker messages which are unwrapped and sent as is). It allows to describe developer tools messages in its own file instead of putting all of them into render_messages_internal.h

brettw: please check if it's OK to have ForwardMessageToToolsClient in WebContentsView


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

[Søren Thygesen Gjesse, 2009-02-20 16:14:36]

LGTM

[jam, 2009-02-20 21:04:57]

http://codereview.chromium.org/21540/diff/39/1029
File chrome/browser/debugger/tools_view.cc (right):

http://codereview.chromium.org/21540/diff/39/1029#newcode33
Line 33: new ViewMsg_ForwardToToolsClient(routing_id, message));
per my earlier comment, this isn't needed.  You can just do Send(new
IPC::Message(message)).  (You need to make a copy since Send owns the message
that you give it.

http://codereview.chromium.org/21540/diff/39/1038
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/21540/diff/39/1038#newcode1193
Line 1193: host->Send(new ViewMsg_ForwardToToolsAgent(inspected_view_id_,
message));
this isn't needed, you can just do Send(new IPC::Message(message))

[yurys, 2009-02-24 18:03:23]

- Removed ToolsContent from the change: ViewMsg_SetupToolsClient is now sent
from RenderViewHost when renderer created if RenderViewHost::SetInspectedView
has been called.

- Messages are wrapped only on renderer->browser hop.

http://codereview.chromium.org/21540/diff/39/1029
File chrome/browser/debugger/tools_view.cc (right):

http://codereview.chromium.org/21540/diff/39/1029#newcode33
Line 33: new ViewMsg_ForwardToToolsClient(routing_id, message));
On 2009/02/20 21:04:57, John Abd-El-Malek wrote:
> per my earlier comment, this isn't needed.  You can just do Send(new
> IPC::Message(message)).  (You need to make a copy since Send owns the message
> that you give it.

Done.

http://codereview.chromium.org/21540/diff/39/1038
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/21540/diff/39/1038#newcode1193
Line 1193: host->Send(new ViewMsg_ForwardToToolsAgent(inspected_view_id_,
message));
On 2009/02/20 21:04:57, John Abd-El-Malek wrote:
> this isn't needed, you can just do Send(new IPC::Message(message))

Done.

[brettw, 2009-02-24 18:34:16]

This is a lot better, thanks.

One think that crossed my mind that you should test. We have the capability to
switch RenderViews on a WebContents. This happens most commonly when you are on
the new tab page and go somewhere, although depending on the process model it
can happen going between sites.

Be sure to test that your tools window doesn't get too confused when the
RenderView it's inspecting goes away.

http://codereview.chromium.org/21540/diff/5001/5015
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/21540/diff/5001/5015#newcode620
Line 620: int inspected_process_id_;
Can you provide comments for these two variables? Be sure to document what -1
means.

http://codereview.chromium.org/21540/diff/5001/5009
File chrome/browser/tab_contents/web_contents_view_mac.mm (right):

http://codereview.chromium.org/21540/diff/5001/5009#newcode106
Line 106: void WebContentsViewMac::ForwardMessageToToolsClient(const
IPC::Message& message) {
This line is > 80 cols.

http://codereview.chromium.org/21540/diff/5001/5016
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/21540/diff/5001/5016#newcode897
Line 897: struct ParamTraits<Message> {
Do you still need this with the new routing?

[yurys, 2009-02-24 18:52:19]

On 2009/02/24 18:34:16, brettw wrote:
> This is a lot better, thanks.
> 
> One think that crossed my mind that you should test. We have the capability to
> switch RenderViews on a WebContents. This happens most commonly when you are
on
> the new tab page and go somewhere, although depending on the process model it
> can happen going between sites.
> 
> Be sure to test that your tools window doesn't get too confused when the
> RenderView it's inspecting goes away.
> 
We'are planning to support page inspection in case of cross domain navigation
when new renderer process is created but not in this CL. To get started we first
of all need communication channel between developer tools UI and inspected page.
This CL is focused on browser side of this channel and it's not final
implementation.

[yurys, 2009-02-24 18:52:27]

http://codereview.chromium.org/21540/diff/5001/5015
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/21540/diff/5001/5015#newcode620
Line 620: int inspected_process_id_;
On 2009/02/24 18:34:16, brettw wrote:
> Can you provide comments for these two variables? Be sure to document what -1
> means.

Done.

http://codereview.chromium.org/21540/diff/5001/5009
File chrome/browser/tab_contents/web_contents_view_mac.mm (right):

http://codereview.chromium.org/21540/diff/5001/5009#newcode106
Line 106: void WebContentsViewMac::ForwardMessageToToolsClient(const
IPC::Message& message) {
On 2009/02/24 18:34:16, brettw wrote:
> This line is > 80 cols.

Done.

http://codereview.chromium.org/21540/diff/5001/5016
File chrome/common/ipc_message_utils.h (right):

http://codereview.chromium.org/21540/diff/5001/5016#newcode897
Line 897: struct ParamTraits<Message> {
On 2009/02/24 18:34:16, brettw wrote:
> Do you still need this with the new routing?

Yes, I need this to wrap tools messages on renderer->browser hop(see
ViewHostMsg_ForwardToToolsClient, ViewHostMsg_ForwardToToolsAgent).

[brettw, 2009-02-24 18:55:34]

This patch looks fine to me from the RenderViewHost level. I didn't check the
views code really. I've added Ben to this review since he's the expert in this
area (he also suggested a naming change and you and he should agree on what this
will be).

Sorry for all the reviews, when you're adding very central code like this, it
touches a lot of areas and we want to be extra sure it's done properly.

[jam, 2009-02-24 22:08:08]

ipc stuff looks good to me (please update the description)

[yurys, 2009-02-25 15:43:56]

Brett, I discovered that to support loading dev tools from a URL with
chrome-ui:// scheme  we need to add a new DOMUI implementation that would
support dev tools URL. I've added necessary changes to this CL but I'm a bit
confused about this design. It looks to me that adding a new DOMUI
implementation is much like adding new WebContents descendant that we wanted to
avoid. Can you please clarify this? Is it OK to add new DOMUIs or this stuff is
also going to be refactored soon?

Loading dev tools from a URL with a different scheme is not an option as it
would require adding new TabContents type and deriving from WebContents.


On 2009/02/24 18:55:34, brettw wrote:
> This patch looks fine to me from the RenderViewHost level. I didn't check the
> views code really. I've added Ben to this review since he's the expert in this
> area (he also suggested a naming change and you and he should agree on what
this
> will be).
> 
> Sorry for all the reviews, when you're adding very central code like this, it
> touches a lot of areas and we want to be extra sure it's done properly.

[brettw, 2009-02-25 17:34:24]

LGTM. I have a few style suggestions before you check in (you don't need to wait
for another review).

http://codereview.chromium.org/21540/diff/4060/5123
File chrome/browser/dom_ui/dev_tools_ui.cc (right):

http://codereview.chromium.org/21540/diff/4060/5123#newcode12
Line 12: std::string url = DOMUIContents::GetScheme();
Maybe this would be simpler as:
return GURL(DOMUIContents::GetScheme() + "://" + kDevToolsHost);

http://codereview.chromium.org/21540/diff/4060/5124
File chrome/browser/dom_ui/dev_tools_ui.h (right):

http://codereview.chromium.org/21540/diff/4060/5124#newcode5
Line 5: #ifndef CHROME_BROWSER_DOM_UI_DEV_TOOLS_UI_H__
Only use one underscore at the end.

http://codereview.chromium.org/21540/diff/4060/5124#newcode12
Line 12: explicit DevToolsUI(DOMUIContents* contents) : DOMUI(contents) {} ;
You don't need the ; at the end here.

http://codereview.chromium.org/21540/diff/4060/5124#newcode20
Line 20: private:
No blank line after public/private.

[yurys, 2009-02-25 18:01:17]

http://codereview.chromium.org/21540/diff/4060/5123
File chrome/browser/dom_ui/dev_tools_ui.cc (right):

http://codereview.chromium.org/21540/diff/4060/5123#newcode12
Line 12: std::string url = DOMUIContents::GetScheme();
On 2009/02/25 17:34:24, brettw wrote:
> Maybe this would be simpler as:
> return GURL(DOMUIContents::GetScheme() + "://" + kDevToolsHost); 

Done.

http://codereview.chromium.org/21540/diff/4060/5124
File chrome/browser/dom_ui/dev_tools_ui.h (right):

http://codereview.chromium.org/21540/diff/4060/5124#newcode5
Line 5: #ifndef CHROME_BROWSER_DOM_UI_DEV_TOOLS_UI_H__
On 2009/02/25 17:34:24, brettw wrote:
> Only use one underscore at the end.

Done.

http://codereview.chromium.org/21540/diff/4060/5124#newcode12
Line 12: explicit DevToolsUI(DOMUIContents* contents) : DOMUI(contents) {} ;
On 2009/02/25 17:34:24, brettw wrote:
> You don't need the ; at the end here.

Done.

http://codereview.chromium.org/21540/diff/4060/5124#newcode20
Line 20: private:
On 2009/02/25 17:34:24, brettw wrote:
> No blank line after public/private.

Done.

[yurys, 2009-02-25 18:34:33]

brettw: I added DOMUI::RendererCreated method and moved dev tools specific
messag passinf from RenderViewHost::CreateRenderView there. Please review this.

[brettw, 2009-02-25 18:40:09]

LGTM

http://codereview.chromium.org/21540/diff/5215/5236
File chrome/browser/dom_ui/dev_tools_ui.h (right):

http://codereview.chromium.org/21540/diff/5215/5236#newcode20
Line 20: virtual void RendererCreated(RenderViewHost* render_view_host);
Since this is part of the DOMUI implementation, I wouldn't put a blank line
above this. We usually group overridden functions from a single source together
like this.

http://codereview.chromium.org/21540/diff/5215/5241
File chrome/browser/dom_ui/dom_ui.h (right):

http://codereview.chromium.org/21540/diff/5215/5241#newcode25
Line 25: virtual void RendererCreated(RenderViewHost* render_view_host) {}
When you sync, you'll notice I renamed RendererCreated to RenderViewCreated to
be more accurate. You should change all of your names as well, including this
new one.

[yurys, 2009-02-26 08:11:04]

http://codereview.chromium.org/21540/diff/5215/5236
File chrome/browser/dom_ui/dev_tools_ui.h (right):

http://codereview.chromium.org/21540/diff/5215/5236#newcode20
Line 20: virtual void RendererCreated(RenderViewHost* render_view_host);
On 2009/02/25 18:40:09, brettw wrote:
> Since this is part of the DOMUI implementation, I wouldn't put a blank line
> above this. We usually group overridden functions from a single source
together
> like this.

Done.

http://codereview.chromium.org/21540/diff/5215/5241
File chrome/browser/dom_ui/dom_ui.h (right):

http://codereview.chromium.org/21540/diff/5215/5241#newcode25
Line 25: virtual void RendererCreated(RenderViewHost* render_view_host) {}
On 2009/02/25 18:40:09, brettw wrote:
> When you sync, you'll notice I renamed RendererCreated to RenderViewCreated to
> be more accurate. You should change all of your names as well, including this
> new one.

Done.