[DevTools] Make DevTools clients talk directly to DevToolsAgentHost instead of using DevToolsManage…

[DevTools] Make DevTools clients talk directly to DevToolsAgentHost instead of using DevToolsManager.

Previously, client used methods on DevToolsManager passing |this| to most of them.
Instead, it should hold a scoped_refptr to inspected DevToolsAgentHost and talk
to it directly. This creates a single clean entry point for the client, and allows
in the future to handle browser-wide protocol messages and debug multiple hosts at once.

BUG=401073
TBR=kalman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290236

[pfeldman, 2014-08-07 14:41:58]

https://codereview.chromium.org/449043002/diff/20001/content/public/browser/d...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/449043002/diff/20001/content/public/browser/d...
content/public/browser/devtools_agent_host.h:33: class Client {
It would be great if Detach and DTAH::DispatchProtocolMessage were a part of the
"service" API that is only accessible to the attached client.

class ProtocolConnectionPeer {
    virtual void DispatchMessage(ConnectPeer* other);
    virtual void PeerDetached(ConnectionPeer* other);
}

class Client : public ProtocolConnectionPeer {
    virtual void PeerAttached(ConnectionPeer* agent);
}

class AgentImpl {
    Attach(Client* client) {
        client_ = client;
        client_->PeerAttached(this);
    }
}

wdyt?

[pfeldman, 2014-08-07 15:40:30]

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_ui_bindings.h (right):

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.h:68: content::DevToolsAgentHost*
agent_host() { return agent_host_.get(); }
Lets not expose it.

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.h:76: void
InspectAgentHost(content::DevToolsAgentHost* agent_host);
AttachTo(DTAH)

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.h:90: virtual void
SendMessageFromAgentHost(
DispatchProtocolMessage

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.h:93: virtual void
AgentHostDetached(
AgentHostClosed

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:794: if
((*it)->bindings_->agent_host() == agent_host)
You could try matching bindings client here instead.

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:826:
bindings_->InspectAgentHost(bindings_->agent_host());
bindings->Reattach()

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:904:
bindings_->InspectAgentHost(NULL);
bindings_->Detach()

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/debugger/debugger_api.cc:378: delete this;
Are you now deleting things twice?

https://codereview.chromium.org/449043002/diff/20001/content/browser/devtools...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/449043002/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_agent_host_impl.cc:66: AddRef(); // Balanced
in DetachInternal.
Why is this necessary? Clients will hold agents to send them messages anyways.

https://codereview.chromium.org/449043002/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_agent_host_impl.cc:113: DetachReason reason,
bool need_detach) {
This seems error-prone. The reason should be enough to make this decision.

https://codereview.chromium.org/449043002/diff/20001/content/browser/devtools...
File content/browser/devtools/devtools_http_handler_impl.cc (right):

https://codereview.chromium.org/449043002/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:95: if (reason ==
content::DevToolsAgentHost::DETACHED_BY_CLIENT)
Lets not make this change as a part of the large one. It is really hard to
follow the change to the semantics.

[dgozman, 2014-08-07 16:51:53]

PTAL

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_ui_bindings.h (right):

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.h:68: content::DevToolsAgentHost*
agent_host() { return agent_host_.get(); }
On 2014/08/07 15:40:30, pfeldman wrote:
> Lets not expose it.

Done.

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.h:76: void
InspectAgentHost(content::DevToolsAgentHost* agent_host);
On 2014/08/07 15:40:30, pfeldman wrote:
> AttachTo(DTAH)

Done.

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.h:90: virtual void
SendMessageFromAgentHost(
On 2014/08/07 15:40:30, pfeldman wrote:
> DispatchProtocolMessage

Done.

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_ui_bindings.h:93: virtual void
AgentHostDetached(
On 2014/08/07 15:40:30, pfeldman wrote:
> AgentHostClosed

Done.

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:794: if
((*it)->bindings_->agent_host() == agent_host)
On 2014/08/07 15:40:30, pfeldman wrote:
> You could try matching bindings client here instead.

Using IsAttachedTo.

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:826:
bindings_->InspectAgentHost(bindings_->agent_host());
On 2014/08/07 15:40:30, pfeldman wrote:
> bindings->Reattach()

Done.

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/devtools_window.cc:904:
bindings_->InspectAgentHost(NULL);
On 2014/08/07 15:40:30, pfeldman wrote:
> bindings_->Detach()

Done.

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/debugger/debugger_api.cc (right):

https://codereview.chromium.org/449043002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/debugger/debugger_api.cc:378: delete this;
On 2014/08/07 15:40:30, pfeldman wrote:
> Are you now deleting things twice?

Yes, fixed.

https://codereview.chromium.org/449043002/diff/20001/content/browser/devtools...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/449043002/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_agent_host_impl.cc:66: AddRef(); // Balanced
in DetachInternal.
On 2014/08/07 15:40:30, pfeldman wrote:
> Why is this necessary? Clients will hold agents to send them messages anyways.

Done.

https://codereview.chromium.org/449043002/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_agent_host_impl.cc:113: DetachReason reason,
bool need_detach) {
On 2014/08/07 15:40:30, pfeldman wrote:
> This seems error-prone. The reason should be enough to make this decision.

Done.

https://codereview.chromium.org/449043002/diff/20001/content/browser/devtools...
File content/browser/devtools/devtools_http_handler_impl.cc (right):

https://codereview.chromium.org/449043002/diff/20001/content/browser/devtools...
content/browser/devtools/devtools_http_handler_impl.cc:95: if (reason ==
content::DevToolsAgentHost::DETACHED_BY_CLIENT)
On 2014/08/07 15:40:30, pfeldman wrote:
> Lets not make this change as a part of the large one. It is really hard to
> follow the change to the semantics.

Done.

[pfeldman, 2014-08-07 17:31:15]

lgtm

https://codereview.chromium.org/449043002/diff/80001/content/public/browser/d...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/449043002/diff/80001/content/public/browser/d...
content/public/browser/devtools_agent_host.h:82: virtual void
DispatchOnInspectorBackend(const std::string& message) = 0;
I wish we could rename it to DispatchProtocolMessage as well.

[dgozman, 2014-08-07 18:30:13]

@jam: Take a look at content, especially content/public please. I'm removing
DevToolsManager and DevToolsClientHost in favor of explicit interaction with
DevToolsAgentHost. The rest of the patch just moves all the clients to new API.

[jam, 2014-08-08 01:23:28]

lgtm with nit

https://codereview.chromium.org/449043002/diff/80001/content/public/browser/d...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/449043002/diff/80001/content/public/browser/d...
content/public/browser/devtools_agent_host.h:27: class Client {
nit: move this to a separate header and call it DevToolsAgentHostClient per
http://www.chromium.org/developers/content-module/content-api

[dgozman, 2014-08-08 11:51:56]

https://codereview.chromium.org/449043002/diff/80001/content/public/browser/d...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/449043002/diff/80001/content/public/browser/d...
content/public/browser/devtools_agent_host.h:27: class Client {
On 2014/08/08 01:23:28, jam wrote:
> nit: move this to a separate header and call it DevToolsAgentHostClient per
> http://www.chromium.org/developers/content-module/content-api

Done.

https://codereview.chromium.org/449043002/diff/80001/content/public/browser/d...
content/public/browser/devtools_agent_host.h:82: virtual void
DispatchOnInspectorBackend(const std::string& message) = 0;
On 2014/08/07 17:31:14, pfeldman wrote:
> I wish we could rename it to DispatchProtocolMessage as well.

Done.

[dgozman, 2014-08-08 12:06:08]

TBR'ing kalman@ for apps/ and extensions/.

[dgozman, 2014-08-08 17:06:21]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-08-08 17:07:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dgozman@chromium.org/449043002/140001

[commit-bot: I haz the power, 2014-08-08 22:46:25]

Change committed as 288452

[pfeldman, 2014-08-12 12:28:43]

A revert of this CL has been created in
https://codereview.chromium.org/467653002/ by pfeldman@chromium.org.

The reason for reverting is:
https://code.google.com/p/chromium/issues/detail?id=402835.

[dgozman, 2014-08-18 09:59:23]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2014-08-18 10:00:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dgozman@chromium.org/449043002/160001

[commit-bot: I haz the power, 2014-08-18 12:31:50]

Committed patchset #9 (160001) as 290236