Extract debugger target enumeration into a separate class

content/public/browser/devtools_agent_host.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_PUBLIC_BROWSER_DEVTOOLS_AGENT_HOST_H_
 #define CONTENT_PUBLIC_BROWSER_DEVTOOLS_AGENT_HOST_H_
 
 #include <string>
+#include <vector>
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "content/common/content_export.h"
 
 namespace content {
 
 class RenderViewHost;
 class WebContents;
 
 // Describes interface for managing devtools agents from browser process.
 class CONTENT_EXPORT DevToolsAgentHost
     : public base::RefCounted<DevToolsAgentHost> {
  public:
   // Returns DevToolsAgentHost that can be used for inspecting |rvh|.
   // New DevToolsAgentHost will be created if it does not exist.
   static scoped_refptr<DevToolsAgentHost> GetFor(RenderViewHost* rvh);
 
   // Returns true iff an instance of DevToolsAgentHost for the |rvh|
   // does exist.
   static bool HasFor(RenderViewHost* rvh);
 
   // Returns DevToolsAgentHost that can be used for inspecting shared worker
   // with given worker process host id and routing id.
   static scoped_refptr<DevToolsAgentHost> GetForWorker(int worker_process_id,
                                                        int worker_route_id);
 
   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;
 
  protected:
   friend class base::RefCounted<DevToolsAgentHost>;
   virtual ~DevToolsAgentHost() {}
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_DEVTOOLS_AGENT_HOST_H_