DevTools: split DevToolsHandler into client and agent handlers

DevTools handler now handles messages from both client and agent and is created for all new render_view_hosts. This patch moves handling of IPC messages from agent to RenderViewDevToolsAgentHost and handling of client IPC messages to DevToolsWindow.

BUG=None
TEST=Existing DevTools tests

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

[pfeldman, 2011-08-29 10:46:27]

On Aug 29, 2011 1:37 PM, <yurys@chromium.org> wrote:
>
> Reviewers: loislo, pfeldman,
>
> Description:
> DevTools handler now handles messages from both client and agent and is
created
> for all new render_view_hosts. This patch breaks DevToolsHandler down into
> DevToolsClientHandler and DevToolsAgentHandler. DevToolsClientHandler is
owned
> by corresponding DevToolsClientHost. DevToolsAgentHost owns
> RenderViewDevToolsAgentHost.
>

Shouldn't they be merged with RVDTAH and DTWindow correspondingly?

>
> BUG=None
> TEST=Existing DevTools tests
>
>
> Please review this at http://codereview.chromium.org/7778010/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M chrome/browser/chrome_content_browser_client.cc
>  M chrome/browser/debugger/devtools_window.h
>  M chrome/browser/debugger/devtools_window.cc
>  A content/browser/debugger/devtools_agent_handler.h
>  A content/browser/debugger/devtools_agent_handler.cc
>  A + content/browser/debugger/devtools_client_handler.h
>  A content/browser/debugger/devtools_client_handler.cc
>  M content/browser/debugger/devtools_handler.h
>  M content/browser/debugger/devtools_handler.cc
>  M content/browser/debugger/devtools_manager.h
>  M content/browser/debugger/devtools_manager.cc
>  M content/browser/debugger/devtools_manager_unittest.cc
>  M content/browser/debugger/render_view_devtools_agent_host.h
>  M content/browser/debugger/render_view_devtools_agent_host.cc
>  M content/content_browser.gypi
>
>

[yurys, 2011-08-29 11:13:15]

On 2011/08/29 10:46:27, pfeldman wrote:
> On Aug 29, 2011 1:37 PM, <mailto:yurys@chromium.org> wrote:
> >
> > Reviewers: loislo, pfeldman,
> >
> > Description:
> > DevTools handler now handles messages from both client and agent and is
> created
> > for all new render_view_hosts. This patch breaks DevToolsHandler down into
> > DevToolsClientHandler and DevToolsAgentHandler. DevToolsClientHandler is
> owned
> > by corresponding DevToolsClientHost. DevToolsAgentHost owns
> > RenderViewDevToolsAgentHost.
> >
> 
> Shouldn't they be merged with RVDTAH and DTWindow correspondingly?
> 
Why would you want that? As we discussed with you offline, this way we have a
clear separation between IPC message handling and other logic. Additionally we
can pass any DevToolsClient into the client handler, the same is almost true for
DevToolsAgentHandler.

[pfeldman, 2011-08-29 11:26:08]

On Aug 29, 2011 2:13 PM, <yurys@chromium.org> wrote:
>
> On 2011/08/29 10:46:27, pfeldman wrote:
>
>> On Aug 29, 2011 1:37 PM, <mailto:yurys@chromium.org> wrote:
>> >
>> > Reviewers: loislo, pfeldman,
>> >
>> > Description:
>> > DevTools handler now handles messages from both client and agent and is
>> created
>> > for all new render_view_hosts. This patch breaks DevToolsHandler down
into
>> > DevToolsClientHandler and DevToolsAgentHandler. DevToolsClientHandler
is
>> owned
>> > by corresponding DevToolsClientHost. DevToolsAgentHost owns
>> > RenderViewDevToolsAgentHost.
>> >
>
>
>> Shouldn't they be merged with RVDTAH and DTWindow correspondingly?
>
>
> Why would you want that? As we discussed with you offline, this way we
have a
> clear separation between IPC message handling and other logic.
Additionally we
> can pass any DevToolsClient into the client handler, the same is almost
true for
> DevToolsAgentHandler.
>

From the remote debugging perspective, devtools window and its ipc is a
single entity. RVDevtools agent host and its ipc also make no sense without
each other.

Offline, I thought u wanted to split the handler and merge it into existing
classes.
>
> http://codereview.chromium.org/7778010/

[yurys, 2011-08-29 11:43:04]

On 2011/08/29 11:26:08, pfeldman wrote:
> On Aug 29, 2011 2:13 PM, <mailto:yurys@chromium.org> wrote:
> >
> > On 2011/08/29 10:46:27, pfeldman wrote:
> >
> >> On Aug 29, 2011 1:37 PM, <mailto:yurys@chromium.org> wrote:
> >> >
> >> > Reviewers: loislo, pfeldman,
> >> >
> >> > Description:
> >> > DevTools handler now handles messages from both client and agent and is
> >> created
> >> > for all new render_view_hosts. This patch breaks DevToolsHandler down
> into
> >> > DevToolsClientHandler and DevToolsAgentHandler. DevToolsClientHandler
> is
> >> owned
> >> > by corresponding DevToolsClientHost. DevToolsAgentHost owns
> >> > RenderViewDevToolsAgentHost.
> >> >
> >
> >
> >> Shouldn't they be merged with RVDTAH and DTWindow correspondingly?
> >
> >
> > Why would you want that? As we discussed with you offline, this way we
> have a
> > clear separation between IPC message handling and other logic.
> Additionally we
> > can pass any DevToolsClient into the client handler, the same is almost
> true for
> > DevToolsAgentHandler.
> >
> 
> From the remote debugging perspective, devtools window and its ipc is a
> single entity. RVDevtools agent host and its ipc also make no sense without
> each other.
> 
> Offline, I thought u wanted to split the handler and merge it into existing
> classes.
Merging DevToolsAgentHandler into DevToolsAgentHost will introduce a new new
dependency: DevToolsAgentHost->DevToolsManager in addition to existing
DevToolsManager->DevToolsAgentHost. Also DevToolsWindow is long enough to add
IPC logic in it while having DevToolsClientHandler in a separate class will
allow others to easily implement their own devtools window if they don't use
chrome/ and only depend on content/

[pfeldman, 2011-08-29 13:42:18]

On Aug 29, 2011 2:43 PM, <yurys@chromium.org> wrote:
>
> On 2011/08/29 11:26:08, pfeldman wrote:
>
>> On Aug 29, 2011 2:13 PM, <mailto:yurys@chromium.org> wrote:
>> >
>> > On 2011/08/29 10:46:27, pfeldman wrote:
>> >
>> >> On Aug 29, 2011 1:37 PM, <mailto:yurys@chromium.org> wrote:
>> >> >
>> >> > Reviewers: loislo, pfeldman,
>> >> >
>> >> > Description:
>> >> > DevTools handler now handles messages from both client and agent and
is
>> >> created
>> >> > for all new render_view_hosts. This patch breaks DevToolsHandler
down
>> into
>> >> > DevToolsClientHandler and DevToolsAgentHandler.
DevToolsClientHandler
>> is
>> >> owned
>> >> > by corresponding DevToolsClientHost. DevToolsAgentHost owns
>> >> > RenderViewDevToolsAgentHost.
>> >> >
>> >
>> >
>> >> Shouldn't they be merged with RVDTAH and DTWindow correspondingly?
>> >
>> >
>> > Why would you want that? As we discussed with you offline, this way we
>> have a
>> > clear separation between IPC message handling and other logic.
>> Additionally we
>> > can pass any DevToolsClient into the client handler, the same is almost
>> true for
>> > DevToolsAgentHandler.
>> >
>
>
>>  From the remote debugging perspective, devtools window and its ipc is a
>> single entity. RVDevtools agent host and its ipc also make no sense
without
>> each other.
>
>
>> Offline, I thought u wanted to split the handler and merge it into
existing
>> classes.
>
> Merging DevToolsAgentHandler into DevToolsAgentHost will introduce a new
new
> dependency: DevToolsAgentHost->DevToolsManager in addition to existing
> DevToolsManager->DevToolsAgentHost.

I was not suggesting this. I was suggesting merging it into RVDTAH.

Also DevToolsWindow is long enough to add
> IPC logic in it while having DevToolsClientHandler in a separate class
will
> allow others to easily implement their own devtools window if they don't
use
> chrome/ and only depend on content/

It is clear from what you extract that client handler only makes sense for
window implementation (all methods have Window suffix).

>
> http://codereview.chromium.org/7778010/
Y

[yurys, 2011-08-29 13:45:31]

On 2011/08/29 13:42:18, pfeldman wrote:
> On Aug 29, 2011 2:43 PM, <mailto:yurys@chromium.org> wrote:
> >
> > On 2011/08/29 11:26:08, pfeldman wrote:
> >
> >> On Aug 29, 2011 2:13 PM, <mailto:yurys@chromium.org> wrote:
> >> >
> >> > On 2011/08/29 10:46:27, pfeldman wrote:
> >> >
> >> >> On Aug 29, 2011 1:37 PM, <mailto:yurys@chromium.org> wrote:
> >> >> >
> >> >> > Reviewers: loislo, pfeldman,
> >> >> >
> >> >> > Description:
> >> >> > DevTools handler now handles messages from both client and agent and
> is
> >> >> created
> >> >> > for all new render_view_hosts. This patch breaks DevToolsHandler
> down
> >> into
> >> >> > DevToolsClientHandler and DevToolsAgentHandler.
> DevToolsClientHandler
> >> is
> >> >> owned
> >> >> > by corresponding DevToolsClientHost. DevToolsAgentHost owns
> >> >> > RenderViewDevToolsAgentHost.
> >> >> >
> >> >
> >> >
> >> >> Shouldn't they be merged with RVDTAH and DTWindow correspondingly?
> >> >
> >> >
> >> > Why would you want that? As we discussed with you offline, this way we
> >> have a
> >> > clear separation between IPC message handling and other logic.
> >> Additionally we
> >> > can pass any DevToolsClient into the client handler, the same is almost
> >> true for
> >> > DevToolsAgentHandler.
> >> >
> >
> >
> >>  From the remote debugging perspective, devtools window and its ipc is a
> >> single entity. RVDevtools agent host and its ipc also make no sense
> without
> >> each other.
> >
> >
> >> Offline, I thought u wanted to split the handler and merge it into
> existing
> >> classes.
> >
> > Merging DevToolsAgentHandler into DevToolsAgentHost will introduce a new
> new
> > dependency: DevToolsAgentHost->DevToolsManager in addition to existing
> > DevToolsManager->DevToolsAgentHost.
> 
> I was not suggesting this. I was suggesting merging it into RVDTAH.
> 
> Also DevToolsWindow is long enough to add
> > IPC logic in it while having DevToolsClientHandler in a separate class
> will
> > allow others to easily implement their own devtools window if they don't
> use
> > chrome/ and only depend on content/
> 
> It is clear from what you extract that client handler only makes sense for
> window implementation (all methods have Window suffix).
> 
Yes, but it doesn't have to be the window from chrome/.../devtools_window.cc it
may be something with much simpler implementation but not repeating IPC handling
logic.

What about the DevToolsAgentHost->DevToolsManager dependency, do you suggest we
introduce it in favor of merging message handler into RenderViewAgentHost?

[pfeldman, 2011-08-29 14:05:14]

On Aug 29, 2011 4:45 PM, <yurys@chromium.org> wrote:
>
> On 2011/08/29 13:42:18, pfeldman wrote:
>
>> On Aug 29, 2011 2:43 PM, <mailto:yurys@chromium.org> wrote:
>> >
>> > On 2011/08/29 11:26:08, pfeldman wrote:
>> >
>> >> On Aug 29, 2011 2:13 PM, <mailto:yurys@chromium.org> wrote:
>> >> >
>> >> > On 2011/08/29 10:46:27, pfeldman wrote:
>> >> >
>> >> >> On Aug 29, 2011 1:37 PM, <mailto:yurys@chromium.org> wrote:
>> >> >> >
>> >> >> > Reviewers: loislo, pfeldman,
>> >> >> >
>> >> >> > Description:
>> >> >> > DevTools handler now handles messages from both client and agent
and
>> is
>> >> >> created
>> >> >> > for all new render_view_hosts. This patch breaks DevToolsHandler
>> down
>> >> into
>> >> >> > DevToolsClientHandler and DevToolsAgentHandler.
>> DevToolsClientHandler
>> >> is
>> >> >> owned
>> >> >> > by corresponding DevToolsClientHost. DevToolsAgentHost owns
>> >> >> > RenderViewDevToolsAgentHost.
>> >> >> >
>> >> >
>> >> >
>> >> >> Shouldn't they be merged with RVDTAH and DTWindow correspondingly?
>> >> >
>> >> >
>> >> > Why would you want that? As we discussed with you offline, this way
we
>> >> have a
>> >> > clear separation between IPC message handling and other logic.
>> >> Additionally we
>> >> > can pass any DevToolsClient into the client handler, the same is
almost
>> >> true for
>> >> > DevToolsAgentHandler.
>> >> >
>> >
>> >
>> >>  From the remote debugging perspective, devtools window and its ipc is
a
>> >> single entity. RVDevtools agent host and its ipc also make no sense
>> without
>> >> each other.
>> >
>> >
>> >> Offline, I thought u wanted to split the handler and merge it into
>> existing
>> >> classes.
>> >
>> > Merging DevToolsAgentHandler into DevToolsAgentHost will introduce a
new
>> new
>> > dependency: DevToolsAgentHost->DevToolsManager in addition to existing
>> > DevToolsManager->DevToolsAgentHost.
>
>
>> I was not suggesting this. I was suggesting merging it into RVDTAH.
>
>
>> Also DevToolsWindow is long enough to add
>> > IPC logic in it while having DevToolsClientHandler in a separate class
>> will
>> > allow others to easily implement their own devtools window if they
don't
>> use
>> > chrome/ and only depend on content/
>
>
>> It is clear from what you extract that client handler only makes sense
for
>> window implementation (all methods have Window suffix).
>
>
> Yes, but it doesn't have to be the window from
chrome/.../devtools_window.cc it
> may be something with much simpler implementation but not repeating IPC
handling
> logic.
>

I don't see much 'logic' here.

> What about the DevToolsAgentHost->DevToolsManager dependency, do you
suggest we
> introduce it in favor of merging message handler into RenderViewAgentHost?
>

Sorry, don't see why you would need one.

>
> http://codereview.chromium.org/7778010/

[yurys, 2011-08-31 09:40:46]

Merged IPC handling into RenderViewDevToolsAgentHost and DevToolsWindow. Please
take a look.

[pfeldman, 2011-08-31 13:42:53]

lgtm

http://codereview.chromium.org/7778010/diff/41/chrome/browser/debugger/devtoo...
File chrome/browser/debugger/devtools_window.h (right):

http://codereview.chromium.org/7778010/diff/41/chrome/browser/debugger/devtoo...
chrome/browser/debugger/devtools_window.h:40: private RenderViewHostObserver {
All other observers use public inheritance, including the DTH you are removing.
I'd suggest keeping it public for consistency.

http://codereview.chromium.org/7778010/diff/41/content/browser/debugger/rende...
File content/browser/debugger/render_view_devtools_agent_host.h (right):

http://codereview.chromium.org/7778010/diff/41/content/browser/debugger/rende...
content/browser/debugger/render_view_devtools_agent_host.h:18: private
RenderViewHostObserver {
ditto

[yurys, 2011-08-31 13:59:16]

http://codereview.chromium.org/7778010/diff/41/chrome/browser/debugger/devtoo...
File chrome/browser/debugger/devtools_window.h (right):

http://codereview.chromium.org/7778010/diff/41/chrome/browser/debugger/devtoo...
chrome/browser/debugger/devtools_window.h:40: private RenderViewHostObserver {
On 2011/08/31 13:42:53, pfeldman wrote:
> All other observers use public inheritance, including the DTH you are
removing.
> I'd suggest keeping it public for consistency.

I made NotificationObserver and TabContentsDelegate private ancestors instead.
There is no reason to expose the fact that DevToolsWindow derives from them.