Introduce DevToolsExternalAgentProxy interface

Introduced DevToolsExternalAgentProxy interface for connecting to an agent by means other than IPC (such as network connection).

BUG=None

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

[Vladislav Kaznacheev, 2013-03-20 08:19:33]

Please take a look at the general approach.

[pfeldman, 2013-03-20 12:35:46]

https://codereview.chromium.org/12950002/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/12950002/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_agent_host_impl.cc:49: void
DevToolsAgentHostImpl::AgentDisconnected() {
I'd leave this as is.

https://codereview.chromium.org/12950002/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_agent_host_impl.h (right):

https://codereview.chromium.org/12950002/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_agent_host_impl.h:56: class CONTENT_EXPORT
IPCDevToolsAgentHost : public DevToolsAgentHostImpl {
It now needs its own file.

https://codereview.chromium.org/12950002/diff/1/content/browser/devtools/forw...
File content/browser/devtools/forwarding_devtools_agent_host.h (right):

https://codereview.chromium.org/12950002/diff/1/content/browser/devtools/forw...
content/browser/devtools/forwarding_devtools_agent_host.h:13: class
CONTENT_EXPORT ForwardingDevToolsAgentHost: public DevToolsAgentHostImpl {
RemoteDevToolsAgentHostImpl?

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
File content/public/browser/devtools_agent_connection.h (right):

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_connection.h:13: class CONTENT_EXPORT
DevToolsAgentConnection {
RemoteDevToolsAgentHost::Delegate (to emphasize it is only used for external
hosts).

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_connection.h:17: virtual void Attach() =
0;
This is a stub implemented externally - must have default implementation.

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_connection.h:20: virtual void
Reattach(const std::string& saved_agent_state) = 0;
We don't need it here.

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_connection.h:26: virtual void
DispatchOnInspectorBackend (const std::string& message) = 0;
Extra space before (

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_connection.h:29: virtual void
InspectElement(int x, int y) = 0;
I would not add this one here.

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_host.h:33: DevToolsAgentConnection*
connection);
DevToolsAgentTransport ?

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_host.h:69: virtual void
AgentDisconnected() = 0;
It would be great if it was only exposed to certain hosts
(RemoteDevToolsAgentHost instances)

[Vladislav Kaznacheev, 2013-03-20 15:23:20]

PTAL

https://codereview.chromium.org/12950002/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_agent_host_impl.cc (right):

https://codereview.chromium.org/12950002/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_agent_host_impl.cc:49: void
DevToolsAgentHostImpl::AgentDisconnected() {
On 2013/03/20 12:35:46, pfeldman wrote:
> I'd leave this as is.

Done.

https://codereview.chromium.org/12950002/diff/1/content/browser/devtools/devt...
File content/browser/devtools/devtools_agent_host_impl.h (right):

https://codereview.chromium.org/12950002/diff/1/content/browser/devtools/devt...
content/browser/devtools/devtools_agent_host_impl.h:56: class CONTENT_EXPORT
IPCDevToolsAgentHost : public DevToolsAgentHostImpl {
On 2013/03/20 12:35:46, pfeldman wrote:
> It now needs its own file.

Done.

https://codereview.chromium.org/12950002/diff/1/content/browser/devtools/forw...
File content/browser/devtools/forwarding_devtools_agent_host.h (right):

https://codereview.chromium.org/12950002/diff/1/content/browser/devtools/forw...
content/browser/devtools/forwarding_devtools_agent_host.h:13: class
CONTENT_EXPORT ForwardingDevToolsAgentHost: public DevToolsAgentHostImpl {
On 2013/03/20 12:35:46, pfeldman wrote:
> RemoteDevToolsAgentHostImpl?

Done.

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
File content/public/browser/devtools_agent_connection.h (right):

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_connection.h:13: class CONTENT_EXPORT
DevToolsAgentConnection {
Not sure I understand. There is no RemoteDevToolsAgentHost.
On 2013/03/20 12:35:46, pfeldman wrote:
> RemoteDevToolsAgentHost::Delegate (to emphasize it is only used for external
> hosts).

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_connection.h:17: virtual void Attach() =
0;
On 2013/03/20 12:35:46, pfeldman wrote:
> This is a stub implemented externally - must have default implementation.

Done.

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_connection.h:20: virtual void
Reattach(const std::string& saved_agent_state) = 0;
On 2013/03/20 12:35:46, pfeldman wrote:
> We don't need it here.

Done.

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_connection.h:26: virtual void
DispatchOnInspectorBackend (const std::string& message) = 0;
Done. And renamed to Send for a good measure.
On 2013/03/20 12:35:46, pfeldman wrote:
> Extra space before (

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_connection.h:29: virtual void
InspectElement(int x, int y) = 0;
On 2013/03/20 12:35:46, pfeldman wrote:
> I would not add this one here.

Done.

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
File content/public/browser/devtools_agent_host.h (right):

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_host.h:33: DevToolsAgentConnection*
connection);
On 2013/03/20 12:35:46, pfeldman wrote:
> DevToolsAgentTransport ?

Done.

https://codereview.chromium.org/12950002/diff/1/content/public/browser/devtoo...
content/public/browser/devtools_agent_host.h:69: virtual void
AgentDisconnected() = 0;
On 2013/03/20 12:35:46, pfeldman wrote:
> It would be great if it was only exposed to certain hosts
> (RemoteDevToolsAgentHost instances)

Done.

[pfeldman, 2013-03-21 06:24:32]

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
File content/browser/devtools/devtools_remote_agent_connector_impl.cc (right):

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_remote_agent_connector_impl.cc:11: class
ForwardingDevToolsAgentHost: public content::DevToolsAgentHostImpl {
Space before : missing

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_remote_agent_connector_impl.cc:12: friend
class DevToolsRemoteAgentConnectorImpl;
Why friends?

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
File content/browser/devtools/devtools_remote_agent_connector_impl.h (right):

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_remote_agent_connector_impl.h:22: friend class
DevToolsRemoteAgentConnector;
Your base is your friend? Why?

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_remote_agent_connector_impl.h:23:
ForwardingDevToolsAgentHost* agent_host_;
This should be scoped_refptr

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_remote_agent_connector_impl.h:24:
DevToolsRemoteAgentConnectorImpl(Delegate* delegate);
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
File content/public/browser/devtools_remote_agent_connector.h (right):

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
content/public/browser/devtools_remote_agent_connector.h:15: class
CONTENT_EXPORT DevToolsRemoteAgentConnector {
"Connector" does not give me enough information, I would not have it in
content/public. ExternalDevToolsAgentHost would sound bever even though it is
not inherited from DevToolsAgentHost.

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
content/public/browser/devtools_remote_agent_connector.h:17: 
Extra blank line.

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
content/public/browser/devtools_remote_agent_connector.h:27: virtual void
Send(const std::string& message) {};
SendMessageToBackend

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
content/public/browser/devtools_remote_agent_connector.h:35: // The |delegate|
object is not owned by the connector.
It is not clear it creates DevToolsAgentHost from its signature. You only say
"takes ownership" when it does. So you should say "caller takes ownership".

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
content/public/browser/devtools_remote_agent_connector.h:39: virtual
DevToolsAgentHost* GetAgentHost() = 0;
What does associated with the connector mean?

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
content/public/browser/devtools_remote_agent_connector.h:42: virtual void
ConnectionClosed() = 0;
Adding convenience DispatchMessageFromFrontend would be useful to the client. We
would not even need a getter above them (at least for now), exactly as you
originally suggested.

[Vladislav Kaznacheev, 2013-03-21 06:55:53]

PTAL. Still not sure about the new interface name. (Remote)DevToolsAgentProxy?

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
File content/browser/devtools/devtools_remote_agent_connector_impl.cc (right):

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_remote_agent_connector_impl.cc:11: class
ForwardingDevToolsAgentHost: public content::DevToolsAgentHostImpl {
On 2013/03/21 06:24:32, pfeldman wrote:
> Space before : missing

Done.

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_remote_agent_connector_impl.cc:12: friend
class DevToolsRemoteAgentConnectorImpl;
Its constructor is private. But I have a better idea: why don't I make it an
inner class of DevToolsRemoteAgentConnectorImpl?
On 2013/03/21 06:24:32, pfeldman wrote:
> Why friends?

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
File content/browser/devtools/devtools_remote_agent_connector_impl.h (right):

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_remote_agent_connector_impl.h:22: friend class
DevToolsRemoteAgentConnector;
Because it is being instantiated by a static method of the base (the same reason
DevToolsAgentHost is a friend of RenderViewDevToolsAgentHost).

On 2013/03/21 06:24:32, pfeldman wrote:
> Your base is your friend? Why?

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_remote_agent_connector_impl.h:23:
ForwardingDevToolsAgentHost* agent_host_;
On 2013/03/21 06:24:32, pfeldman wrote:
> This should be scoped_refptr

Done. Also made the base refcounted.

https://codereview.chromium.org/12950002/diff/8001/content/browser/devtools/d...
content/browser/devtools/devtools_remote_agent_connector_impl.h:24:
DevToolsRemoteAgentConnectorImpl(Delegate* delegate);
On 2013/03/21 06:24:32, pfeldman wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
File content/public/browser/devtools_remote_agent_connector.h (right):

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
content/public/browser/devtools_remote_agent_connector.h:15: class
CONTENT_EXPORT DevToolsRemoteAgentConnector {
It does sound better but still not good enough. How about
DevToolsAgentHostProxy?
On 2013/03/21 06:24:32, pfeldman wrote:
> "Connector" does not give me enough information, I would not have it in
> content/public. ExternalDevToolsAgentHost would sound bever even though it is
> not inherited from DevToolsAgentHost.

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
content/public/browser/devtools_remote_agent_connector.h:17: 
On 2013/03/21 06:24:32, pfeldman wrote:
> Extra blank line.

Done.

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
content/public/browser/devtools_remote_agent_connector.h:27: virtual void
Send(const std::string& message) {};
On 2013/03/21 06:24:32, pfeldman wrote:
> SendMessageToBackend

Done.

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
content/public/browser/devtools_remote_agent_connector.h:35: // The |delegate|
object is not owned by the connector.
"creates DevToolsAgentHost" was a typo. Changed the second sentence to "caller
keeps the |delegate| ownership"
On 2013/03/21 06:24:32, pfeldman wrote:
> It is not clear it creates DevToolsAgentHost from its signature. You only say
> "takes ownership" when it does. So you should say "caller takes ownership".

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
content/public/browser/devtools_remote_agent_connector.h:39: virtual
DevToolsAgentHost* GetAgentHost() = 0;
Changed to "Returns the local DevToolsAgentHost for the remote agent."

On 2013/03/21 06:24:32, pfeldman wrote:
> What does associated with the connector mean?

https://codereview.chromium.org/12950002/diff/8001/content/public/browser/dev...
content/public/browser/devtools_remote_agent_connector.h:42: virtual void
ConnectionClosed() = 0;
Cannot remove the getter as we need to pass DevToolsAgentHost to
DevToolsManager::RegisterDevToolsClientHostFor attaching the client.

Why do we need DispatchMessageFromFrontend? DevToolsAgentHost does not have
that, only its *Impl does. We only ever send anything through
DevToolsClientHost.


On 2013/03/21 06:24:32, pfeldman wrote:
> Adding convenience DispatchMessageFromFrontend would be useful to the client.
We
> would not even need a getter above them (at least for now), exactly as you
> originally suggested.

[pfeldman, 2013-03-21 12:18:06]

https://codereview.chromium.org/12950002/diff/22001/content/browser/devtools/...
File content/browser/devtools/devtools_external_agent_proxy_impl.cc (right):

https://codereview.chromium.org/12950002/diff/22001/content/browser/devtools/...
content/browser/devtools/devtools_external_agent_proxy_impl.cc:33:
delegate_->SendMessageToBacked(message);
ToBackend

https://codereview.chromium.org/12950002/diff/22001/content/browser/devtools/...
content/browser/devtools/devtools_external_agent_proxy_impl.cc:50:
scoped_refptr<DevToolsAgentHost> DevToolsExternalAgentProxyImpl::
scoped_refptr<DevToolsAgentHost>
DevToolsExternalAgentProxyImpl::GetAgentHost() {

https://codereview.chromium.org/12950002/diff/22001/content/browser/devtools/...
File content/browser/devtools/devtools_external_agent_proxy_impl.h (right):

https://codereview.chromium.org/12950002/diff/22001/content/browser/devtools/...
content/browser/devtools/devtools_external_agent_proxy_impl.h:20: friend class
DevToolsExternalAgentProxy;
I think I was asking this already: why is your implementation makes friendship
to its base?

https://codereview.chromium.org/12950002/diff/22001/content/public/browser/de...
File content/public/browser/devtools_external_agent_proxy.h (right):

https://codereview.chromium.org/12950002/diff/22001/content/public/browser/de...
content/public/browser/devtools_external_agent_proxy.h:18: class CONTENT_EXPORT
DevToolsExternalAgentProxy
Why not ExternalDevToolsAgentProxy (as we agreed offline).

[Vladislav Kaznacheev, 2013-03-21 13:00:41]

https://codereview.chromium.org/12950002/diff/22001/content/browser/devtools/...
File content/browser/devtools/devtools_external_agent_proxy_impl.cc (right):

https://codereview.chromium.org/12950002/diff/22001/content/browser/devtools/...
content/browser/devtools/devtools_external_agent_proxy_impl.cc:33:
delegate_->SendMessageToBacked(message);
On 2013/03/21 12:18:06, pfeldman wrote:
> ToBackend

Done.

https://codereview.chromium.org/12950002/diff/22001/content/browser/devtools/...
content/browser/devtools/devtools_external_agent_proxy_impl.cc:50:
scoped_refptr<DevToolsAgentHost> DevToolsExternalAgentProxyImpl::
On 2013/03/21 12:18:06, pfeldman wrote:
> scoped_refptr<DevToolsAgentHost>
> DevToolsExternalAgentProxyImpl::GetAgentHost() {

Done.

https://codereview.chromium.org/12950002/diff/22001/content/browser/devtools/...
File content/browser/devtools/devtools_external_agent_proxy_impl.h (right):

https://codereview.chromium.org/12950002/diff/22001/content/browser/devtools/...
content/browser/devtools/devtools_external_agent_proxy_impl.h:20: friend class
DevToolsExternalAgentProxy;
On 2013/03/21 12:18:06, pfeldman wrote:
> I think I was asking this already: why is your implementation makes friendship
> to its base?

Done.

https://codereview.chromium.org/12950002/diff/22001/content/public/browser/de...
File content/public/browser/devtools_external_agent_proxy.h (right):

https://codereview.chromium.org/12950002/diff/22001/content/public/browser/de...
content/public/browser/devtools_external_agent_proxy.h:18: class CONTENT_EXPORT
DevToolsExternalAgentProxy
Re-agreed offline on DevToolsExternalAgentProxy
On 2013/03/21 12:18:06, pfeldman wrote:
> Why not ExternalDevToolsAgentProxy (as we agreed offline).

[Vladislav Kaznacheev, 2013-03-21 13:03:39]

Hi Johh,

Please review the new interface in
content/public/browser/devtools_external_agent_proxy.h

Best regards,

  Vlad

[jam, 2013-03-21 16:46:51]

On 2013/03/21 13:03:39, Vladislav Kaznacheev wrote:
> Hi Johh,
> 
> Please review the new interface in
> content/public/browser/devtools_external_agent_proxy.h

I'm missing context (since the cl description is minimal and there's no attached
bug with more background).

Can you explain what this will be used for? I presume this will eventually be
called by code in src\chrome?

by convention there are no inner classes in the content api, so
DevToolsExternalAgentProxy::Delegate will have to be a separate class in its own
header

[pfeldman, 2013-03-22 16:01:13]

Here is the context:

DevToolsAgentHost(Impl) is everything devtools needs in order to debug
something. We build them around RVHs and around shared workers in content. So
far, all the implementations for DTAH were located in content. Now we want to
build DTAH for a remote debugging scenario (where page is running on the device
and we represent it in Content using the same DTAH abstraction). As we
understood, we can't implement DTAH interface both in Content (for RVH and
Shared worker) and in Chrome (for ADB-based remote debugging). So we introduce a
factory that creates DTAHImpls for us in content. We feed it with the remote
debugging channel (a simple Attach/Detach/SendMessage interface) that is
sufficient for our needs. The factory is called DevToolsExternalAgentProxy, the
transport interface is its delegate. The DTAH implementation it creates resides
in devtools_external_agent_proxy_impl.

[jam, 2013-03-25 06:21:49]

On 2013/03/22 16:01:13, pfeldman wrote:
> Here is the context:
> 
> DevToolsAgentHost(Impl) is everything devtools needs in order to debug
> something. We build them around RVHs and around shared workers in content. So
> far, all the implementations for DTAH were located in content. Now we want to
> build DTAH for a remote debugging scenario (where page is running on the
device
> and we represent it in Content using the same DTAH abstraction). As we
> understood, we can't implement DTAH interface both in Content (for RVH and
> Shared worker) and in Chrome (for ADB-based remote debugging). So we introduce
a
> factory that creates DTAHImpls for us in content. We feed it with the remote
> debugging channel (a simple Attach/Detach/SendMessage interface) that is
> sufficient for our needs. The factory is called DevToolsExternalAgentProxy,
the
> transport interface is its delegate. The DTAH implementation it creates
resides
> in devtools_external_agent_proxy_impl.

Thanks for the background.

You're right that interfaces that are implemented by content are usually not
implemented by chrome. If I'm understanding correctly, it sounds that in this
case a duplicate interface is being created for implementation by chrome? Ifo
so, it seems preferable in this rare case to have only one interface that is
implemented by both content + chrome. DTAH only has three methods from what I
see (inspect/getid/getrvh). Are those sufficient if the interface is implemented
by chrome?

[pfeldman, 2013-03-25 06:43:31]

No, not enough - three additional methods (Attach, Detach and SendMessage) would
need to be pulled into DTAH from DTAHI.

Now that implementing it from Chrome is an option, let us assess what we like
better long term. As I understand your preference is smaller API surface.

We somewhat liked it this way since we could reuse more code from content. Like
we get DTAH registry for free this way (DTAHI register themselves there).

[Vladislav Kaznacheev, 2013-03-25 10:44:08]

Inheriting from DTAH in Chrome would not really work because the first thing we
would do with it after the construction is passing it to
DevToolsManagerImpl::RegisterDevToolsClientHostFor which will cast it to DTAHI
and fail immediately.

The proposed new interface is not really a duplicate. It describes a (very
basic) communication channel while DTAH is much more and expresses our exact
intent: we want the DTAH to be able to represent an agent which exists somewhere
only reachable by some asynchronous protocol (not IPC).


On 2013/03/25 06:43:31, pfeldman wrote:
> No, not enough - three additional methods (Attach, Detach and SendMessage)
would
> need to be pulled into DTAH from DTAHI.
> 
> Now that implementing it from Chrome is an option, let us assess what we like
> better long term. As I understand your preference is smaller API surface.
> 
> We somewhat liked it this way since we could reuse more code from content.
Like
> we get DTAH registry for free this way (DTAHI register themselves there).

[jam, 2013-03-25 16:25:21]

I'm not really following all this.

Is there more documentation about what this stuff is for? comment #10 mentions
"remote debugging scenario (where page is running on the device and we represent
it in Content using the same DTAH abstraction)". What is this scenario exactly?
Any documents or bugs would be helpful.

Comment #10 says you guys want to build a DTAH for something that's outside of
content. My first question is (above) is what is this thing? Does it really need
to be outside of content? Do you have the code for it already? If so that'd be
helpful to look at. It's sometimes hard to review abstract changes to the
content api without knowing what the consumer looks like.

Comment #10 also says that the intent is to have DTAH that's outside of chrome.
That seems different from what comment #13 says? Am I misunderstanding these
comments?

re the cast: if we do reuse DTAH, then I assume that also involves making
content not freely cast it to DTAHI.

On 2013/03/25 10:44:08, Vladislav Kaznacheev wrote:
> Inheriting from DTAH in Chrome would not really work because the first thing
we
> would do with it after the construction is passing it to
> DevToolsManagerImpl::RegisterDevToolsClientHostFor which will cast it to DTAHI
> and fail immediately.
> 
> The proposed new interface is not really a duplicate. It describes a (very
> basic) communication channel while DTAH is much more and expresses our exact
> intent: we want the DTAH to be able to represent an agent which exists
somewhere
> only reachable by some asynchronous protocol (not IPC).
> 
> 
> On 2013/03/25 06:43:31, pfeldman wrote:
> > No, not enough - three additional methods (Attach, Detach and SendMessage)
> would
> > need to be pulled into DTAH from DTAHI.
> > 
> > Now that implementing it from Chrome is an option, let us assess what we
like
> > better long term. As I understand your preference is smaller API surface.
> > 
> > We somewhat liked it this way since we could reuse more code from content.
> Like
> > we get DTAH registry for free this way (DTAHI register themselves there).

[jam, 2013-03-25 17:03:48]

(Pavel explained more over VC, thanks)

lgtm

https://codereview.chromium.org/12950002/diff/39001/content/browser/devtools/...
File content/browser/devtools/devtools_external_agent_proxy_impl.h (right):

https://codereview.chromium.org/12950002/diff/39001/content/browser/devtools/...
content/browser/devtools/devtools_external_agent_proxy_impl.h:13: class
CONTENT_EXPORT DevToolsExternalAgentProxyImpl
if you only export the one static method in the interface, then you don't need
to export this class

https://codereview.chromium.org/12950002/diff/39001/content/public/browser/de...
File content/public/browser/devtools_external_agent_proxy.h (right):

https://codereview.chromium.org/12950002/diff/39001/content/public/browser/de...
content/public/browser/devtools_external_agent_proxy.h:18: class CONTENT_EXPORT
DevToolsExternalAgentProxy {
nit: just export the static method instead of the whole class

https://codereview.chromium.org/12950002/diff/39001/content/public/browser/de...
File content/public/browser/devtools_external_agent_proxy_delegate.h (right):

https://codereview.chromium.org/12950002/diff/39001/content/public/browser/de...
content/public/browser/devtools_external_agent_proxy_delegate.h:15: class
CONTENT_EXPORT DevToolsExternalAgentProxyDelegate {
nit: CONTENT_EXPORT not needed i think

https://codereview.chromium.org/12950002/diff/39001/content/public/browser/de...
content/public/browser/devtools_external_agent_proxy_delegate.h:18: virtual void
Attach() {};
does it make sense to have empty implementations for attach/detach? it seems the
whole point of this interface is that one needs to override these methods?

if yes, nit: here and below, no semicolon

[Vladislav Kaznacheev, 2013-03-26 06:11:00]

Thanks for the review!

https://codereview.chromium.org/12950002/diff/39001/content/browser/devtools/...
File content/browser/devtools/devtools_external_agent_proxy_impl.h (right):

https://codereview.chromium.org/12950002/diff/39001/content/browser/devtools/...
content/browser/devtools/devtools_external_agent_proxy_impl.h:13: class
CONTENT_EXPORT DevToolsExternalAgentProxyImpl
On 2013/03/25 17:03:48, jam wrote:
> if you only export the one static method in the interface, then you don't need
> to export this class

Done.

https://codereview.chromium.org/12950002/diff/39001/content/public/browser/de...
File content/public/browser/devtools_external_agent_proxy.h (right):

https://codereview.chromium.org/12950002/diff/39001/content/public/browser/de...
content/public/browser/devtools_external_agent_proxy.h:18: class CONTENT_EXPORT
DevToolsExternalAgentProxy {
On 2013/03/25 17:03:48, jam wrote:
> nit: just export the static method instead of the whole class

Done.

https://codereview.chromium.org/12950002/diff/39001/content/public/browser/de...
File content/public/browser/devtools_external_agent_proxy_delegate.h (right):

https://codereview.chromium.org/12950002/diff/39001/content/public/browser/de...
content/public/browser/devtools_external_agent_proxy_delegate.h:15: class
CONTENT_EXPORT DevToolsExternalAgentProxyDelegate {
On 2013/03/25 17:03:48, jam wrote:
> nit: CONTENT_EXPORT not needed i think

Done.

https://codereview.chromium.org/12950002/diff/39001/content/public/browser/de...
content/public/browser/devtools_external_agent_proxy_delegate.h:18: virtual void
Attach() {};
SendMessageToBackend must be overridden. I can imagine an inheritor that does
not override Attach or Detach but I agree that keeping them pure is cleaner.

On 2013/03/25 17:03:48, jam wrote:
> does it make sense to have empty implementations for attach/detach? it seems
the
> whole point of this interface is that one needs to override these methods?
> 
> if yes, nit: here and below, no semicolon

[commit-bot: I haz the power, 2013-03-26 07:07:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/12950002/21002

[commit-bot: I haz the power, 2013-03-26 12:46:46]

Change committed as 190648