Extract debugger target enumeration into a separate class

content/public/browser/devtools_agent_host.h

Index: content/public/browser/devtools_agent_host.h
diff --git a/content/public/browser/devtools_agent_host.h b/content/public/browser/devtools_agent_host.h
index 533c31df79f1545ef84ab2f450590b77339aa4a0..7b9f91bbe265eb25a69e78e4471b4cc01768280c 100644
--- a/content/public/browser/devtools_agent_host.h
+++ b/content/public/browser/devtools_agent_host.h
@@ -6,6 +6,7 @@
 #define CONTENT_PUBLIC_BROWSER_DEVTOOLS_AGENT_HOST_H_
 
 #include <string>
+#include <vector>
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
@@ -35,16 +36,31 @@ class CONTENT_EXPORT DevToolsAgentHost
 
   static bool IsDebuggerAttached(WebContents* web_contents);
 
-  // Detaches given |rvh| from the agent host temporarily and returns a cookie
-  // that allows to reattach another rvh to that agen thost later. Returns -1 if
-  // there is no agent host associated with the |rvh|.
-  static int DisconnectRenderViewHost(RenderViewHost* rvh);
+  // Detaches given |rvh| from the agent host temporarily and returns the agent
+  // host id that allows to reattach another rvh to that agent host later.
+  // Returns NullId if there is no agent host associated with the |rvh|.
+  static std::string DisconnectRenderViewHost(RenderViewHost* rvh);

[jam, 2013/03/06 17:41:52]

why is this changing from an integer to a string that's just the integer
formatted as a string? 

[Vladislav Kaznacheev, 2013/03/06 18:26:15]

Several reasons:
1. It does not have to be an int, we just need a unique string. 
2. We only ever compare ids with other ids or with strings
3. We are currently considering replacing the sequential integers with something
harder to guess (GUID or similar) so that pages in the browser cannot abuse
ws:localhost connections. 
On 2013/03/06 17:41:52, jam wrote:

[jam, 2013/03/06 22:45:27]

So a random integer wouldn't be enough? I ask because it'd be nice to avoid this
NullId concept

 
   // Reattaches agent host detached with DisconnectRenderViewHost method above
   // to |rvh|.
-  static void ConnectRenderViewHost(int agent_host_cookie,
+  static void ConnectRenderViewHost(const std::string& agent_host_cookie,
                                     RenderViewHost* rvh);
 
+  // Returns a list of all existing RenderViewHost's that can be debugged.
+  static std::vector<RenderViewHost*> GetValidRenderViewHosts();
+
+  // Creates or find DevToolsAgentHosts for RenderViewHosts in a list and
+  // retains them as long as the corresponding RenderViewHosts are valid.
+  static void RetainFor(std::vector<RenderViewHost*> rvh_list);

[jam, 2013/03/06 17:41:52]

RetainFor, GetRetained, and GetId() are only called from inside content. so can
you move them to DevToolsAgentHostImpl or are you planning on using these later
from chrome?

[Vladislav Kaznacheev, 2013/03/06 18:26:15]

Yes, these will be called from at least two places in chrome
On 2013/03/06 17:41:52, jam wrote:

[jam, 2013/03/06 22:45:27]

Can you explain how they're called from chrome? It's hard to evaluate this API
without knowing that. In a previous patchset (8) this used to be called
Register. It's not clear to me why Chrome would have to register
DevToolsAgentHost with content, since content is what creates these objects

also if all these methods are needed in the public API, it seems they do not
belong on DevToolsAgentHost. They seem then to only get moved to this interface
just to avoid creating another one in content/public. It's also confusing that
these static methods are really mapping to a singleton.

+
+  // Returns retained DevToolsAgentHost for a given id.
+  static DevToolsAgentHost* GetRetained(const std::string& id);
+
+  static const std::string NullId;
+
+  // Returns the unique id of the agent.
+  virtual std::string GetId() = 0;
+
   // Returns render view host instance for this host if any.
   virtual RenderViewHost* GetRenderViewHost() = 0;