Allow different types of devtools client

Currently we have two types of devtools UI: Chrome built in developer tools window and remote debugger connected over TCP(apavlov is working on it). To allow DevToolsManager coordinate both types of devtools uniformly their API is extracted into DevToolsClientHost interface.

Fix purify errors in DevToolsManager unit tests.
BUG=9150
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12274

[pfeldman, 2009-03-19 13:58:46]

http://codereview.chromium.org/50009/diff/1001/1002
File chrome/browser/debugger/devtools_client_host.h (right):

http://codereview.chromium.org/50009/diff/1001/1002#newcode16
Line 16: // An opaque wrapper around pointer that allows to use them as
identifiers when
nite: around pointerS

http://codereview.chromium.org/50009/diff/1001/1002#newcode21
Line 21: // but we don't want to keep reference on it in say
RenderViewHost(which is
nite: reference TO

http://codereview.chromium.org/50009/diff/1001/1002#newcode21
Line 21: // but we don't want to keep reference on it in say
RenderViewHost(which is
Could you explain it in greater detail, why can't we use DevToolsClientHost*?

http://codereview.chromium.org/50009/diff/1001/1002#newcode22
Line 22: // object that receives the message first and needs to forward it
farther). To
further

http://codereview.chromium.org/50009/diff/1001/1002#newcode38
Line 38: // Describes interface to devtools client. There are currently two
types of
to devtools client host?

http://codereview.chromium.org/50009/diff/1001/1002#newcode50
Line 50: // Whether this id corresponds to this DevToolsClientHost instance.
Returns true iff given id ...

http://codereview.chromium.org/50009/diff/1001/1002#newcode68
Line 68: 
nit: many blank lines

http://codereview.chromium.org/50009/diff/1001/1008
File chrome/browser/debugger/devtools_manager.cc (right):

http://codereview.chromium.org/50009/diff/1001/1008#newcode32
Line 32: void set_devtools_client_host(DevToolsClientHost* host) {
nit: accessors are typically implemented in the header file.

http://codereview.chromium.org/50009/diff/1001/1009
File chrome/browser/debugger/devtools_manager.h (right):

http://codereview.chromium.org/50009/diff/1001/1009#newcode42
Line 42: // DevToolsWindow::Create static method.
Although you are saying that "The manager doesn't distinguish between them",
this class is still DevToolsWindow-aware. Why is this so? Should the factory be
passed in all cases?

[pfeldman, 2009-03-19 15:50:28]

http://codereview.chromium.org/50009/diff/1001/1013
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/50009/diff/1001/1013#newcode952
Line 952: NULL /* Use default factory for developer tools windows*/ );
This sounds like an excuse not to use it explicitly. Should we remove the
'default factory capability' from manager?

http://codereview.chromium.org/50009/diff/1001/1002
File chrome/browser/debugger/devtools_client_host.h (right):

http://codereview.chromium.org/50009/diff/1001/1002#newcode29
Line 29: bool operator==(const DevToolsClientHostID& rhs) const {
Equals?

http://codereview.chromium.org/50009/diff/1001/1002#newcode45
Line 45: virtual void Show() = 0;
Given that Show is action-specific, what is it doing in this interface?

http://codereview.chromium.org/50009/diff/1001/1002#newcode46
Line 46: virtual void Close() = 0;
Please add comment like "Is called by the manager when tab has been closed".

http://codereview.chromium.org/50009/diff/1001/1002#newcode48
Line 48: virtual void SendMessageToClient(const IPC::Message& msg) = 0;
Comment here as well please.

http://codereview.chromium.org/50009/diff/1001/1008
File chrome/browser/debugger/devtools_manager.cc (right):

http://codereview.chromium.org/50009/diff/1001/1008#newcode60
Line 60: DevToolsClientHost* DevToolsManager::CreateDevToolsClientHost(
Do we need this?

http://codereview.chromium.org/50009/diff/1001/1008#newcode90
Line 90: void DevToolsManager::ShowDevToolsForWebContents(
It does not "show" dev tools anymore, correct? Consider renaming.

http://codereview.chromium.org/50009/diff/1001/1007
File chrome/browser/debugger/devtools_view.cc (right):

http://codereview.chromium.org/50009/diff/1001/1007#newcode72
Line 72: web_contents_->CloseContents();  // destroy the tab and navigation
controller
nit: 80 chars

http://codereview.chromium.org/50009/diff/1001/1006
File chrome/browser/debugger/devtools_window.h (right):

http://codereview.chromium.org/50009/diff/1001/1006#newcode13
Line 13: class DevToolsWindow {
DevToolsWindowFactory?

[yurys, 2009-03-20 15:08:18]

Pavel I addressed all your comments. Can you look at the updated version?

http://codereview.chromium.org/50009/diff/1001/1002
File chrome/browser/debugger/devtools_client_host.h (right):

http://codereview.chromium.org/50009/diff/1001/1002#newcode16
Line 16: // An opaque wrapper around pointer that allows to use them as
identifiers when
On 2009/03/19 13:58:46, pfeldman wrote:
> nite: around pointerS

Done.

http://codereview.chromium.org/50009/diff/1001/1002#newcode21
Line 21: // but we don't want to keep reference on it in say
RenderViewHost(which is
On 2009/03/19 13:58:46, pfeldman wrote:
> nite: reference TO

Done.

http://codereview.chromium.org/50009/diff/1001/1002#newcode22
Line 22: // object that receives the message first and needs to forward it
farther). To
On 2009/03/19 13:58:46, pfeldman wrote:
> further

Done.

http://codereview.chromium.org/50009/diff/1001/1002#newcode38
Line 38: // Describes interface to devtools client. There are currently two
types of
On 2009/03/19 13:58:46, pfeldman wrote:
> to devtools client host?

Done.

http://codereview.chromium.org/50009/diff/1001/1002#newcode50
Line 50: // Whether this id corresponds to this DevToolsClientHost instance.
On 2009/03/19 13:58:46, pfeldman wrote:
> Returns true iff given id ...

Done.

http://codereview.chromium.org/50009/diff/1001/1002#newcode68
Line 68: 
On 2009/03/19 13:58:46, pfeldman wrote:
> nit: many blank lines

Done.

http://codereview.chromium.org/50009/diff/1001/1009
File chrome/browser/debugger/devtools_manager.h (right):

http://codereview.chromium.org/50009/diff/1001/1009#newcode42
Line 42: // DevToolsWindow::Create static method.
Done. Factory is required.

On 2009/03/19 13:58:46, pfeldman wrote:
> Although you are saying that "The manager doesn't distinguish between them",
> this class is still DevToolsWindow-aware. Why is this so? Should the factory
be
> passed in all cases?

[pfeldman, 2009-03-20 16:48:00]

LGTM

http://codereview.chromium.org/50009/diff/40/52
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/50009/diff/40/52#newcode955
Line 955: DevToolsWindow* window = host->asDevToolsWindow();
style: AsDevToolsWindow

http://codereview.chromium.org/50009/diff/40/41
File chrome/browser/debugger/devtools_client_host.h (right):

http://codereview.chromium.org/50009/diff/40/41#newcode17
Line 17: // are currently two types of clients: devtools windows and TCP socket
debuggers.
nit: 80 chars

http://codereview.chromium.org/50009/diff/40/41#newcode20
Line 20: class CloseListener {
style: Constructor and DISALLOW_COPY_AND_ASSIGN required.

http://codereview.chromium.org/50009/diff/40/41#newcode23
Line 23: virtual ~CloseListener() {}
style: declare destructor before methods.

http://codereview.chromium.org/50009/diff/40/41#newcode27
Line 27: virtual void InspectedTabClosing() = 0;
Could you provide docs for the methods?

http://codereview.chromium.org/50009/diff/40/47
File chrome/browser/debugger/devtools_manager.cc (right):

http://codereview.chromium.org/50009/diff/40/47#newcode73
Line 73: navcontroller_to_client_host_.begin();
nit: indent

http://codereview.chromium.org/50009/diff/40/48
File chrome/browser/debugger/devtools_manager.h (right):

http://codereview.chromium.org/50009/diff/40/48#newcode26
Line 26: // routes messages between developer tools clients and agents. There
are two
nit: You mention two types of clients all over the place. If we add another
type, one would need to update all the docs...

http://codereview.chromium.org/50009/diff/40/46
File chrome/browser/debugger/devtools_view.cc (right):

http://codereview.chromium.org/50009/diff/40/46#newcode72
Line 72: web_contents_->CloseContents();  // destroy the tab and navigation
controller
nit: 80chars

[yurys, 2009-03-20 16:56:33]

http://codereview.chromium.org/50009/diff/40/52
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/50009/diff/40/52#newcode955
Line 955: DevToolsWindow* window = host->asDevToolsWindow();
On 2009/03/20 16:48:00, pfeldman wrote:
> style: AsDevToolsWindow

Done.

http://codereview.chromium.org/50009/diff/40/41
File chrome/browser/debugger/devtools_client_host.h (right):

http://codereview.chromium.org/50009/diff/40/41#newcode17
Line 17: // are currently two types of clients: devtools windows and TCP socket
debuggers.
On 2009/03/20 16:48:00, pfeldman wrote:
> nit: 80 chars

Done.

http://codereview.chromium.org/50009/diff/40/41#newcode20
Line 20: class CloseListener {
On 2009/03/20 16:48:00, pfeldman wrote:
> style: Constructor and DISALLOW_COPY_AND_ASSIGN required.

Done.

http://codereview.chromium.org/50009/diff/40/41#newcode23
Line 23: virtual ~CloseListener() {}
On 2009/03/20 16:48:00, pfeldman wrote:
> style: declare destructor before methods.

Done.

http://codereview.chromium.org/50009/diff/40/41#newcode27
Line 27: virtual void InspectedTabClosing() = 0;
On 2009/03/20 16:48:00, pfeldman wrote:
> Could you provide docs for the methods?

Done.

http://codereview.chromium.org/50009/diff/40/47
File chrome/browser/debugger/devtools_manager.cc (right):

http://codereview.chromium.org/50009/diff/40/47#newcode73
Line 73: navcontroller_to_client_host_.begin();
On 2009/03/20 16:48:00, pfeldman wrote:
> nit: indent

Done.

http://codereview.chromium.org/50009/diff/40/48
File chrome/browser/debugger/devtools_manager.h (right):

http://codereview.chromium.org/50009/diff/40/48#newcode26
Line 26: // routes messages between developer tools clients and agents. There
are two
Removed that sentence.

On 2009/03/20 16:48:00, pfeldman wrote:
> nit: You mention two types of clients all over the place. If we add another
> type, one would need to update all the docs...

http://codereview.chromium.org/50009/diff/40/46
File chrome/browser/debugger/devtools_view.cc (right):

http://codereview.chromium.org/50009/diff/40/46#newcode72
Line 72: web_contents_->CloseContents();  // destroy the tab and navigation
controller
On 2009/03/20 16:48:00, pfeldman wrote:
> nit: 80chars

Done.