DevTools: Partially redesigned DevToolsAndroidBridge and AndroidDeviceManager

chrome/browser/devtools/device/devtools_android_bridge.cc

Index: chrome/browser/devtools/device/devtools_android_bridge.cc
diff --git a/chrome/browser/devtools/device/devtools_android_bridge.cc b/chrome/browser/devtools/device/devtools_android_bridge.cc
index 71257c644e61d6098d91771b5b1df0bd67ac137d..affaab7887d8e55082bcfc54bcac2b6d87da959f 100644
--- a/chrome/browser/devtools/device/devtools_android_bridge.cc
+++ b/chrome/browser/devtools/device/devtools_android_bridge.cc
@@ -65,10 +65,14 @@ class DiscoveryRequest : public base::RefCountedThreadSafe<
     DiscoveryRequest,
     BrowserThread::DeleteOnUIThread> {
  public:
-  typedef base::Callback<void(DevToolsAndroidBridge::RemoteDevices*)> Callback;
+  typedef base::Callback<void(
+      scoped_ptr<DevToolsAndroidBridge::RemoteDevices>,
+      scoped_ptr<DevToolsAndroidBridge::DeviceHandleMap>,
+      scoped_ptr<DevToolsAndroidBridge::DeviceHandleMap>)> Callback;

[pfeldman, 2014/05/21 11:33:41]

This callback has complex signature, but very neutral name. Consider renaming?

 
   DiscoveryRequest(
       scoped_refptr<DevToolsAndroidBridge> android_bridge,
+      const DevToolsAndroidBridge::DeviceHandleMap& device_handles,
       AndroidDeviceManager* device_manager,
       base::MessageLoop* device_message_loop,
       const AndroidDeviceManager::DeviceProviders& device_providers,
@@ -98,6 +102,9 @@ class DiscoveryRequest : public base::RefCountedThreadSafe<
 
   void Respond();
 
+  scoped_refptr<DevToolsAndroidBridge::DeviceHandle>
+  FindDeviceHandle(const std::string& serial);
+
   scoped_refptr<DevToolsAndroidBridge> android_bridge_;
   AndroidDeviceManager* device_manager_;
   base::MessageLoop* device_message_loop_;
@@ -105,10 +112,13 @@ class DiscoveryRequest : public base::RefCountedThreadSafe<
   std::vector<std::string> serials_;
   DevToolsAndroidBridge::RemoteBrowsers browsers_;
   scoped_ptr<DevToolsAndroidBridge::RemoteDevices> remote_devices_;
+  scoped_ptr<DevToolsAndroidBridge::DeviceHandleMap> all_device_handles_;
+  scoped_ptr<DevToolsAndroidBridge::DeviceHandleMap> used_device_handles_;
 };
 
 DiscoveryRequest::DiscoveryRequest(
     scoped_refptr<DevToolsAndroidBridge> android_bridge,
+    const DevToolsAndroidBridge::DeviceHandleMap& device_handles,
     AndroidDeviceManager* device_manager,
     base::MessageLoop* device_message_loop,
     const AndroidDeviceManager::DeviceProviders& device_providers,
@@ -118,6 +128,9 @@ DiscoveryRequest::DiscoveryRequest(
       device_message_loop_(device_message_loop),
       callback_(callback) {
   remote_devices_.reset(new DevToolsAndroidBridge::RemoteDevices());
+  all_device_handles_.reset(
+      new DevToolsAndroidBridge::DeviceHandleMap(device_handles));
+  used_device_handles_.reset(new DevToolsAndroidBridge::DeviceHandleMap());
 
   device_message_loop_->PostTask(
       FROM_HERE, base::Bind(
@@ -154,7 +167,7 @@ void DiscoveryRequest::ProcessSerials() {
     AndroidDeviceManager::DeviceInfo offline_info;
     offline_info.model = kModelOffline;
     remote_devices_->push_back(new DevToolsAndroidBridge::RemoteDevice(
-        android_bridge_, current_serial(), offline_info, false));
+        FindDeviceHandle(current_serial()), offline_info, false));
     NextDevice();
   }
 }
@@ -162,7 +175,7 @@ void DiscoveryRequest::ProcessSerials() {
 void DiscoveryRequest::ReceivedDeviceInfo(
     const AndroidDeviceManager::DeviceInfo& device_info) {
   remote_devices_->push_back(new DevToolsAndroidBridge::RemoteDevice(
-          android_bridge_, current_serial(), device_info, true));
+      FindDeviceHandle(current_serial()), device_info, true));
   browsers_ = remote_devices_->back()->browsers();
   ProcessSockets();
 }
@@ -240,7 +253,25 @@ void DiscoveryRequest::NextDevice() {
 }
 
 void DiscoveryRequest::Respond() {
-  callback_.Run(remote_devices_.release());
+  callback_.Run(remote_devices_.Pass(),
+                used_device_handles_.Pass(),
+                all_device_handles_.Pass());
+}
+
+scoped_refptr<DevToolsAndroidBridge::DeviceHandle>
+DiscoveryRequest::FindDeviceHandle(const std::string& serial) {
+  DevToolsAndroidBridge::DeviceHandleMap::iterator it =
+      all_device_handles_->find(serial);
+  scoped_refptr<DevToolsAndroidBridge::DeviceHandle> device_handle;
+  if (it != all_device_handles_->end()) {
+    device_handle = it->second;
+  } else {
+    device_handle = new DevToolsAndroidBridge::DeviceHandle(android_bridge_,
+                                                            serial);
+    (*all_device_handles_)[serial] = device_handle;
+  }
+  (*used_device_handles_)[serial] = device_handle;

[pfeldman, 2014/05/21 11:33:41]

So FindDeviceHandle makes it "used"? This is not clear from the method name.
What is used_device_handles_?

+  return device_handle;
 }
 
 // ProtocolCommand ------------------------------------------------------------
@@ -582,11 +613,9 @@ void RemotePageTarget::Navigate(const std::string& url,
 // DevToolsAndroidBridge::RemoteBrowser ---------------------------------------
 
 DevToolsAndroidBridge::RemoteBrowser::RemoteBrowser(
-    scoped_refptr<DevToolsAndroidBridge> android_bridge,
-    const std::string& serial,
+    scoped_refptr<DeviceHandle> device_handle,
     const AndroidDeviceManager::BrowserInfo& browser_info)
-    : android_bridge_(android_bridge),
-      serial_(serial),
+    : device_handle_(device_handle),
       socket_(browser_info.socket_name),
       display_name_(browser_info.display_name),
       type_(browser_info.type),
@@ -636,7 +665,7 @@ void DevToolsAndroidBridge::RemoteBrowser::SetPageDescriptors(
 }
 
 static void RespondOnUIThread(
-    const DevToolsAndroidBridge::RemoteBrowser::JsonRequestCallback& callback,
+    const DevToolsAndroidBridge::JsonRequestCallback& callback,
     int result,
     const std::string& response) {
   if (callback.is_null())
@@ -647,12 +676,7 @@ static void RespondOnUIThread(
 
 void DevToolsAndroidBridge::RemoteBrowser::SendJsonRequest(
     const std::string& request, const JsonRequestCallback& callback) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  android_bridge_->device_message_loop()->PostTask(
-      FROM_HERE,
-      base::Bind(&AndroidDeviceManager::HttpQuery,
-                 android_bridge_->device_manager(), serial_, socket_, request,
-                 base::Bind(&RespondOnUIThread, callback)));
+  device_handle_->SendJsonRequest(socket_, request, callback);
 }
 
 void DevToolsAndroidBridge::RemoteBrowser::SendProtocolCommand(
@@ -678,7 +702,16 @@ void DevToolsAndroidBridge::RemoteBrowser::Open(
 scoped_refptr<content::DevToolsAgentHost>
 DevToolsAndroidBridge::RemoteBrowser::GetAgentHost() {
   return AgentHostDelegate::GetOrCreateAgentHost(
-      "adb:" + serial_ + ":" + socket_, this, kBrowserTargetSocket);
+      "adb:" + device_handle_->serial() + ":" + socket_,
+      this,
+      kBrowserTargetSocket);
+}
+
+scoped_refptr<DevToolsAndroidBridge::AndroidWebSocket>
+DevToolsAndroidBridge::RemoteBrowser::CreateWebSocket(
+    const std::string& url,
+    DevToolsAndroidBridge::AndroidWebSocket::Delegate* delegate) {
+  return device_handle_->CreateWebSocket(socket_, url, delegate);
 }
 
 void DevToolsAndroidBridge::RemoteBrowser::RespondToOpenOnUIThread(
@@ -758,16 +791,13 @@ void DevToolsAndroidBridge::RemoteBrowser::NavigatePageOnUIThread(
 DevToolsAndroidBridge::RemoteBrowser::~RemoteBrowser() {
 }
 
-
 // DevToolsAndroidBridge::RemoteDevice ----------------------------------------
 
 DevToolsAndroidBridge::RemoteDevice::RemoteDevice(
-    scoped_refptr<DevToolsAndroidBridge> android_bridge,
-    const std::string& serial,
+    scoped_refptr<DevToolsAndroidBridge::DeviceHandle> device_handle,
     const AndroidDeviceManager::DeviceInfo& device_info,
     bool connected)
-    : android_bridge_(android_bridge),
-      serial_(serial),
+    : device_handle_(device_handle),
       model_(device_info.model),
       connected_(connected),
       screen_size_(device_info.screen_size) {
@@ -776,19 +806,14 @@ DevToolsAndroidBridge::RemoteDevice::RemoteDevice(
       it != device_info.browser_info.end();
       ++it) {
     browsers_.push_back(new DevToolsAndroidBridge::RemoteBrowser(
-        android_bridge_, serial_, *it));
+        device_handle, *it));
   }
 }
 
 void DevToolsAndroidBridge::RemoteDevice::OpenSocket(
     const std::string& socket_name,
     const AndroidDeviceManager::SocketCallback& callback) {
-  android_bridge_->device_message_loop()->PostTask(FROM_HERE,
-      base::Bind(&AndroidDeviceManager::OpenSocket,
-                 android_bridge_->device_manager(),
-                 serial_,
-                 socket_name,
-                 callback));
+  device_handle_->OpenSocket(socket_name, callback);
 }
 
 DevToolsAndroidBridge::RemoteDevice::~RemoteDevice() {
@@ -842,6 +867,48 @@ DevToolsAndroidBridge::HandlerThread::~HandlerThread() {
       base::Bind(&HandlerThread::StopThread, thread_));
 }
 
+// DevToolsAndroidBridge::DeviceHandle ----------------------------------------
+
+DevToolsAndroidBridge::DeviceHandle::DeviceHandle(
+    scoped_refptr<DevToolsAndroidBridge> android_bridge,
+    const std::string& serial)
+    : android_bridge_(android_bridge),
+      serial_(serial) {
+}
+
+void DevToolsAndroidBridge::DeviceHandle::SendJsonRequest(

[pfeldman, 2014/05/21 11:33:41]

::HttpQuery?

+    const std::string& socket,
+    const std::string& request,
+    const JsonRequestCallback& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  android_bridge_->device_message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&AndroidDeviceManager::HttpQuery,
+                 android_bridge_->device_manager(),
+                 serial_,
+                 socket,
+                 request,
+                 base::Bind(&RespondOnUIThread, callback)));
+}
+
+void DevToolsAndroidBridge::DeviceHandle::OpenSocket(
+    const std::string& socket_name,
+    const AndroidDeviceManager::SocketCallback& callback) {
+  android_bridge_->device_message_loop()->PostTask(FROM_HERE,

[pfeldman, 2014/05/21 11:33:41]

DCHECK thread.

+      base::Bind(&AndroidDeviceManager::OpenSocket,
+                 android_bridge_->device_manager(),
+                 serial_,
+                 socket_name,
+                 callback));
+}
+
+DevToolsAndroidBridge::DeviceHandle::~DeviceHandle() {
+  android_bridge_->device_message_loop()->PostTask(FROM_HERE,

[pfeldman, 2014/05/21 11:33:41]

ditto

+      base::Bind(&AndroidDeviceManager::ReleaseDevice,
+                 android_bridge_->device_manager(),
+                 serial_));
+}
+
 // DevToolsAndroidBridge ------------------------------------------------------
 
 DevToolsAndroidBridge::DevToolsAndroidBridge(Profile* profile)
@@ -875,10 +942,8 @@ void DevToolsAndroidBridge::RemoveDeviceListListener(
       device_list_listeners_.begin(), device_list_listeners_.end(), listener);
   DCHECK(it != device_list_listeners_.end());
   device_list_listeners_.erase(it);
-  if (device_list_listeners_.empty() && device_manager_) {
-    device_message_loop()->PostTask(FROM_HERE,
-        base::Bind(&AndroidDeviceManager::Stop, device_manager_));
-  }
+  if (device_list_listeners_.empty() && device_manager_)
+    device_handles_.clear();
 }
 
 void DevToolsAndroidBridge::AddDeviceCountListener(
@@ -903,6 +968,15 @@ bool DevToolsAndroidBridge::HasDevToolsWindow(const std::string& agent_id) {
   return g_host_delegates.Get().find(agent_id) != g_host_delegates.Get().end();
 }
 
+void DevToolsAndroidBridge::IsConnectedForTest(const std::string& serial,
+    const base::Callback<void(bool)>& callback) {
+  base::PostTaskAndReplyWithResult(
+      device_message_loop()->message_loop_proxy(),
+      FROM_HERE,
+      base::Bind(&AndroidDeviceManager::IsConnected, device_manager_, serial),
+      callback);
+}
+
 DevToolsAndroidBridge::~DevToolsAndroidBridge() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(device_list_listeners_.empty());
@@ -924,6 +998,7 @@ void DevToolsAndroidBridge::RequestDeviceList() {
 
   new DiscoveryRequest(
       this,
+      device_handles_,

[pfeldman, 2014/05/21 11:33:41]

What is device_handles_? What is used_device_handles?

       device_manager(),
       device_message_loop(),
       device_providers_,
@@ -940,14 +1015,17 @@ void DevToolsAndroidBridge::CreatedDeviceManager(
     RequestDeviceCount();
 }
 
-void DevToolsAndroidBridge::ReceivedDeviceList(RemoteDevices* devices_ptr) {
+void DevToolsAndroidBridge::ReceivedDeviceList(
+    scoped_ptr<RemoteDevices> devices,
+    scoped_ptr<DeviceHandleMap> used_device_handles,
+    scoped_ptr<DeviceHandleMap> all_device_handles) {

[pfeldman, 2014/05/21 11:33:41]

Is this field unused?

   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  scoped_ptr<RemoteDevices> devices(devices_ptr);
-
   if (device_list_listeners_.empty())
     return;
 
+  device_handles_ = *used_device_handles.get();
+
   DeviceListListeners copy(device_list_listeners_);
   for (DeviceListListeners::iterator it = copy.begin(); it != copy.end(); ++it)
     (*it)->DeviceListChanged(*devices.get());