DevTools: DevToolsAndroidBridge polling loops

Based on:
https://codereview.chromium.org/286993003/

BUG=

Review URL: https://codereview.chromium.org/305013013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@274611 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/devtools/device/android_device_manager.h

@@ -18,7 +18,9 @@ class StreamSocket;
 }
 
 class AndroidDeviceManager
-    : public base::RefCountedThreadSafe<AndroidDeviceManager>,
+    : public base::RefCountedThreadSafe<
+          AndroidDeviceManager,
+          content::BrowserThread::DeleteOnUIThread>,
       public base::NonThreadSafe {
  public:
   typedef base::Callback<void(int, const std::string&)> CommandCallback;
@@ -196,7 +198,9 @@ class AndroidDeviceManager
     base::Thread* thread_;
   };
 
-  friend class base::RefCountedThreadSafe<AndroidDeviceManager>;
+  friend struct content::BrowserThread::DeleteOnThread<
+      content::BrowserThread::UI>;
+  friend class base::DeleteHelper<AndroidDeviceManager>;
   AndroidDeviceManager();
   virtual ~AndroidDeviceManager();
 

chrome/browser/devtools/device/devtools_android_bridge.cc

@@ -63,7 +63,7 @@ class DiscoveryRequest : public base::RefCountedThreadSafe<
     DiscoveryRequest,
     BrowserThread::DeleteOnUIThread> {
  public:
-  typedef base::Callback<void(scoped_ptr<DevToolsAndroidBridge::RemoteDevices>)>
+  typedef base::Callback<void(const DevToolsAndroidBridge::RemoteDevices&)>
       DiscoveryCallback;
   typedef AndroidDeviceManager::Device Device;
   typedef AndroidDeviceManager::Devices Devices;
@@ -99,7 +99,7 @@ class DiscoveryRequest : public base::RefCountedThreadSafe<
   DiscoveryCallback callback_;
   Devices devices_;
   DevToolsAndroidBridge::RemoteBrowsers browsers_;
-  scoped_ptr<DevToolsAndroidBridge::RemoteDevices> remote_devices_;
+  DevToolsAndroidBridge::RemoteDevices remote_devices_;
 };
 
 DiscoveryRequest::DiscoveryRequest(
@@ -107,7 +107,6 @@ DiscoveryRequest::DiscoveryRequest(
     const DiscoveryCallback& callback)
     : callback_(callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  remote_devices_.reset(new DevToolsAndroidBridge::RemoteDevices());
 
   device_manager->QueryDevices(
       base::Bind(&DiscoveryRequest::ReceivedDevices, this));
@@ -136,9 +135,9 @@ void DiscoveryRequest::ProcessDevices() {
 void DiscoveryRequest::ReceivedDeviceInfo(
     const AndroidDeviceManager::DeviceInfo& device_info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  remote_devices_->push_back(
+  remote_devices_.push_back(
       new DevToolsAndroidBridge::RemoteDevice(current_device(), device_info));
-  browsers_ = remote_devices_->back()->browsers();
+  browsers_ = remote_devices_.back()->browsers();
   ProcessSockets();
 }
 
@@ -212,7 +211,7 @@ void DiscoveryRequest::NextDevice() {
 }
 
 void DiscoveryRequest::Respond() {
-  callback_.Run(remote_devices_.Pass());
+  callback_.Run(remote_devices_);
 }
 
 // ProtocolCommand ------------------------------------------------------------
@@ -766,7 +765,8 @@ DevToolsAndroidBridge::RemoteDevice::~RemoteDevice() {
 
 DevToolsAndroidBridge::DevToolsAndroidBridge(Profile* profile)
     : profile_(profile),
-      device_manager_(AndroidDeviceManager::Create()) {
+      device_manager_(AndroidDeviceManager::Create()),
+      task_scheduler_(base::Bind(&DevToolsAndroidBridge::ScheduleTaskDefault)) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   pref_change_registrar_.Init(profile_->GetPrefs());
   pref_change_registrar_.Add(prefs::kDevToolsDiscoverUsbDevicesEnabled,
@@ -780,7 +780,7 @@ void DevToolsAndroidBridge::AddDeviceListListener(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   device_list_listeners_.push_back(listener);
   if (device_list_listeners_.size() == 1)
-    RequestDeviceList();
+    StartDeviceListPolling();
 }
 
 void DevToolsAndroidBridge::RemoveDeviceListListener(
@@ -791,14 +791,14 @@ void DevToolsAndroidBridge::RemoveDeviceListListener(
   DCHECK(it != device_list_listeners_.end());
   device_list_listeners_.erase(it);
   if (device_list_listeners_.empty())
-    devices_.clear();
+    StopDeviceListPolling();
 }
 
 void DevToolsAndroidBridge::AddDeviceCountListener(
     DeviceCountListener* listener) {
   device_count_listeners_.push_back(listener);
   if (device_count_listeners_.size() == 1)
-    RequestDeviceCount();
+    StartDeviceCountPolling();
 }
 
 void DevToolsAndroidBridge::RemoveDeviceCountListener(
@@ -808,6 +808,8 @@ void DevToolsAndroidBridge::RemoveDeviceCountListener(
       device_count_listeners_.begin(), device_count_listeners_.end(), listener);
   DCHECK(it != device_count_listeners_.end());
   device_count_listeners_.erase(it);
+  if (device_count_listeners_.empty())
+    StopDeviceCountPolling();
 }
 
 // static
@@ -822,61 +824,87 @@ DevToolsAndroidBridge::~DevToolsAndroidBridge() {
   DCHECK(device_count_listeners_.empty());
 }
 
-void DevToolsAndroidBridge::RequestDeviceList() {
+void DevToolsAndroidBridge::StartDeviceListPolling() {
+  device_list_callback_.Reset(
+    base::Bind(&DevToolsAndroidBridge::ReceivedDeviceList, this));
+  RequestDeviceList(device_list_callback_.callback());
+}
+
+void DevToolsAndroidBridge::StopDeviceListPolling() {
+  device_list_callback_.Cancel();
+  devices_.clear();
+}
+
+void DevToolsAndroidBridge::RequestDeviceList(
+    const base::Callback<void(const RemoteDevices&)>& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (device_list_listeners_.empty())
+  if (device_list_listeners_.empty() ||
+      !callback.Equals(device_list_callback_.callback()))
     return;
 
-  new DiscoveryRequest(
-      device_manager_.get(),
-      base::Bind(&DevToolsAndroidBridge::ReceivedDeviceList, this));
+  new DiscoveryRequest(device_manager_.get(), callback);
 }
 
-void DevToolsAndroidBridge::ReceivedDeviceList(
-    scoped_ptr<RemoteDevices> devices) {
+void DevToolsAndroidBridge::ReceivedDeviceList(const RemoteDevices& devices) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  DeviceListListeners copy(device_list_listeners_);
+  for (DeviceListListeners::iterator it = copy.begin(); it != copy.end(); ++it)
+    (*it)->DeviceListChanged(devices);
+
   if (device_list_listeners_.empty())
     return;
 
-  devices_ = *devices;
+  devices_ = devices;
 
-  DeviceListListeners copy(device_list_listeners_);
-  for (DeviceListListeners::iterator it = copy.begin(); it != copy.end(); ++it)
-    (*it)->DeviceListChanged(*devices.get());
+  task_scheduler_.Run(
+      base::Bind(&DevToolsAndroidBridge::RequestDeviceList,
+                 this, device_list_callback_.callback()));
+}
 
-  BrowserThread::PostDelayedTask(
-      BrowserThread::UI,
-      FROM_HERE,
-      base::Bind(&DevToolsAndroidBridge::RequestDeviceList, this),
-      base::TimeDelta::FromMilliseconds(kAdbPollingIntervalMs));
+void DevToolsAndroidBridge::StartDeviceCountPolling() {
+  device_count_callback_.Reset(
+      base::Bind(&DevToolsAndroidBridge::ReceivedDeviceCount, this));
+  RequestDeviceCount(device_count_callback_.callback());
 }
 
-void DevToolsAndroidBridge::RequestDeviceCount() {
+void DevToolsAndroidBridge::StopDeviceCountPolling() {
+  device_count_callback_.Cancel();
+}
+
+void DevToolsAndroidBridge::RequestDeviceCount(
+    const base::Callback<void(int)>& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (device_count_listeners_.empty())
+  if (device_count_listeners_.empty() ||
+      !callback.Equals(device_count_callback_.callback()))
     return;
 
-  UsbDeviceProvider::CountDevices(
-      base::Bind(&DevToolsAndroidBridge::ReceivedDeviceCount, this));
+  UsbDeviceProvider::CountDevices(callback);
 }
 
 void DevToolsAndroidBridge::ReceivedDeviceCount(int count) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  if (device_count_listeners_.empty())
-     return;
-
   DeviceCountListeners copy(device_count_listeners_);
   for (DeviceCountListeners::iterator it = copy.begin(); it != copy.end(); ++it)
     (*it)->DeviceCountChanged(count);
 
+  if (device_count_listeners_.empty())
+     return;
+
+  task_scheduler_.Run(
+      base::Bind(&DevToolsAndroidBridge::RequestDeviceCount,
+                 this, device_count_callback_.callback()));
+}
+
+// static
+void DevToolsAndroidBridge::ScheduleTaskDefault(const base::Closure& task) {
   BrowserThread::PostDelayedTask(
       BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&DevToolsAndroidBridge::RequestDeviceCount, this),
+      task,
       base::TimeDelta::FromMilliseconds(kAdbPollingIntervalMs));
 }
 
@@ -901,4 +929,8 @@ void DevToolsAndroidBridge::CreateDeviceProviders() {
     device_providers.push_back(new UsbDeviceProvider(profile_));
   }
   device_manager_->SetDeviceProviders(device_providers);
+  if (!device_list_listeners_.empty()) {
+    StopDeviceListPolling();
+    StartDeviceListPolling();
+  }
 }

chrome/browser/devtools/device/devtools_android_bridge.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/callback.h"
+#include "base/cancelable_callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/prefs/pref_change_registrar.h"
@@ -212,6 +213,11 @@ class DevToolsAndroidBridge
     device_manager_->SetDeviceProviders(device_providers);
   }
 
+  void set_task_scheduler_for_test(
+      base::Callback<void(const base::Closure&)> scheduler) {
+    task_scheduler_ = scheduler;
+  }
+
   static bool HasDevToolsWindow(const std::string& agent_id);
 
  private:
@@ -221,11 +227,18 @@ class DevToolsAndroidBridge
 
   virtual ~DevToolsAndroidBridge();
 
-  void RequestDeviceList();
-  void ReceivedDeviceList(scoped_ptr<RemoteDevices> devices);
-  void RequestDeviceCount();
+  void StartDeviceListPolling();
+  void StopDeviceListPolling();
+  void RequestDeviceList(
+      const base::Callback<void(const RemoteDevices&)>& callback);
+  void ReceivedDeviceList(const RemoteDevices& devices);
+  void StartDeviceCountPolling();
+  void StopDeviceCountPolling();
+  void RequestDeviceCount(const base::Callback<void(int)>& callback);
   void ReceivedDeviceCount(int count);
 
+  static void ScheduleTaskDefault(const base::Closure& task);
+
   void CreateDeviceProviders();
 
   Profile* profile_;
@@ -234,9 +247,12 @@ class DevToolsAndroidBridge
 
   typedef std::vector<DeviceListListener*> DeviceListListeners;
   DeviceListListeners device_list_listeners_;
+  base::CancelableCallback<void(const RemoteDevices&)> device_list_callback_;
 
   typedef std::vector<DeviceCountListener*> DeviceCountListeners;
   DeviceCountListeners device_count_listeners_;
+  base::CancelableCallback<void(int)> device_count_callback_;
+  base::Callback<void(const base::Closure&)> task_scheduler_;
 
   PrefChangeRegistrar pref_change_registrar_;
   DISALLOW_COPY_AND_ASSIGN(DevToolsAndroidBridge);

chrome/browser/devtools/device/usb/devtools_android_bridge_browsertest.cc

+// Copyright 2014 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.
+
+#include <algorithm>
+
+#include "base/message_loop/message_loop.h"
+#include "chrome/browser/devtools/device/devtools_android_bridge.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/usb_service/usb_device.h"
+#include "components/usb_service/usb_service.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_utils.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::_;
+using content::BrowserThread;
+using usb_service::UsbService;
+using usb_service::UsbDevice;
+
+class DevToolsAndroidBridgeWarmUp
+    : public DevToolsAndroidBridge::DeviceCountListener {
+ public:
+  DevToolsAndroidBridgeWarmUp(base::Closure closure,
+                              scoped_refptr<DevToolsAndroidBridge> adb_bridge)
+      : closure_(closure), adb_bridge_(adb_bridge) {}
+
+  virtual void DeviceCountChanged(int count) OVERRIDE {
+    adb_bridge_->RemoveDeviceCountListener(this);
+    closure_.Run();
+  }
+
+  base::Closure closure_;
+  scoped_refptr<DevToolsAndroidBridge> adb_bridge_;
+};
+
+class MockCountListener : public DevToolsAndroidBridge::DeviceCountListener {
+ public:
+  explicit MockCountListener(scoped_refptr<DevToolsAndroidBridge> adb_bridge)
+      : adb_bridge_(adb_bridge),
+        reposts_left_(10),
+        invoked_(0) {
+  }
+
+  virtual void DeviceCountChanged(int count) OVERRIDE {
+    ++invoked_;
+    adb_bridge_->RemoveDeviceCountListener(this);
+    Shutdown();
+  }
+
+  void Shutdown() {
+    ShutdownOnUIThread();
+  };
+
+  void ShutdownOnUIThread() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    if (reposts_left_-- == 0) {
+      base::MessageLoop::current()->Quit();
+    } else {
+      BrowserThread::PostTask(
+          BrowserThread::FILE,
+          FROM_HERE,
+          base::Bind(&MockCountListener::ShutdownOnFileThread,
+                     base::Unretained(this)));
+    }
+  }
+
+  void ShutdownOnFileThread() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
+    BrowserThread::PostTask(BrowserThread::UI,
+                            FROM_HERE,
+                            base::Bind(&MockCountListener::ShutdownOnUIThread,
+                                       base::Unretained(this)));
+  }
+
+  scoped_refptr<DevToolsAndroidBridge> adb_bridge_;
+  int reposts_left_;
+  int invoked_;
+};
+
+class MockCountListenerWithReAdd : public MockCountListener {
+ public:
+  explicit MockCountListenerWithReAdd(
+      scoped_refptr<DevToolsAndroidBridge> adb_bridge)
+      : MockCountListener(adb_bridge),
+        readd_count_(2) {
+  }
+
+  virtual void DeviceCountChanged(int count) OVERRIDE {
+    ++invoked_;
+    adb_bridge_->RemoveDeviceCountListener(this);
+    if (readd_count_ > 0) {
+      readd_count_--;
+      adb_bridge_->AddDeviceCountListener(this);
+      adb_bridge_->RemoveDeviceCountListener(this);
+      adb_bridge_->AddDeviceCountListener(this);
+    } else {
+      Shutdown();
+    }
+  }
+
+  int readd_count_;
+};
+
+class MockCountListenerWithReAddWhileQueued : public MockCountListener {
+ public:
+  MockCountListenerWithReAddWhileQueued(
+      scoped_refptr<DevToolsAndroidBridge> adb_bridge)
+      : MockCountListener(adb_bridge),
+        readded_(false) {
+  }
+
+  virtual void DeviceCountChanged(int count) OVERRIDE {
+    ++invoked_;
+    if (!readded_) {
+      readded_ = true;
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&MockCountListenerWithReAddWhileQueued::ReAdd,
+                     base::Unretained(this)));
+    } else {
+      adb_bridge_->RemoveDeviceCountListener(this);
+      Shutdown();
+    }
+  }
+
+  void ReAdd() {
+    adb_bridge_->RemoveDeviceCountListener(this);
+    adb_bridge_->AddDeviceCountListener(this);
+  }
+
+  bool readded_;
+};
+
+class MockUsbService : public UsbService {
+ public:
+  MOCK_METHOD1(GetDeviceById, scoped_refptr<UsbDevice>(uint32 unique_id));
+  virtual void GetDevices(
+      std::vector<scoped_refptr<UsbDevice> >* devices) OVERRIDE {}
+};
+
+class DevtoolsAndroidBridgeBrowserTest : public InProcessBrowserTest {
+ protected:
+  DevtoolsAndroidBridgeBrowserTest()
+      : scheduler_invoked_(0) {
+  }
+
+  virtual void SetUpOnMainThread() OVERRIDE {
+    scoped_refptr<content::MessageLoopRunner> runner =
+        new content::MessageLoopRunner;
+
+    BrowserThread::PostTaskAndReply(
+        BrowserThread::FILE,
+        FROM_HERE,
+        base::Bind(&DevtoolsAndroidBridgeBrowserTest::SetUpService, this),
+        runner->QuitClosure());
+    runner->Run();
+
+    adb_bridge_ =
+        DevToolsAndroidBridge::Factory::GetForProfile(browser()->profile());
+    DCHECK(adb_bridge_);
+    adb_bridge_->set_task_scheduler_for_test(base::Bind(
+        &DevtoolsAndroidBridgeBrowserTest::ScheduleDeviceCountRequest, this));
+
+    runner = new content::MessageLoopRunner;
+
+    DevToolsAndroidBridgeWarmUp warmup(runner->QuitClosure(), adb_bridge_);
+    adb_bridge_->AddDeviceCountListener(&warmup);
+    runner->Run();
+
+    runner_ = new content::MessageLoopRunner;
+  }
+
+  void ScheduleDeviceCountRequest(const base::Closure& request) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    scheduler_invoked_++;
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, request);
+  }
+
+  void SetUpService() {
+    service_ = new MockUsbService();
+    UsbService::SetInstanceForTest(service_);
+  }
+
+  virtual void CleanUpOnMainThread() OVERRIDE {
+    scoped_refptr<content::MessageLoopRunner> runner =
+        new content::MessageLoopRunner;
+    UsbService* service = NULL;
+    BrowserThread::PostTaskAndReply(
+        BrowserThread::FILE,
+        FROM_HERE,
+        base::Bind(&UsbService::SetInstanceForTest, service),
+        runner->QuitClosure());
+    runner->Run();
+  }
+
+  MockUsbService* service_;
+  int scheduler_invoked_;
+  scoped_refptr<content::MessageLoopRunner> runner_;
+  scoped_refptr<DevToolsAndroidBridge> adb_bridge_;
+};
+
+IN_PROC_BROWSER_TEST_F(DevtoolsAndroidBridgeBrowserTest,
+                       TestNoMultipleCallsRemoveInCallback) {
+  scoped_ptr<MockCountListener> listener(new MockCountListener(adb_bridge_));
+
+  adb_bridge_->AddDeviceCountListener(listener.get());
+
+  runner_->Run();
+  EXPECT_EQ(1, listener->invoked_);
+  EXPECT_EQ(listener->invoked_ - 1, scheduler_invoked_);
+}
+
+IN_PROC_BROWSER_TEST_F(DevtoolsAndroidBridgeBrowserTest,
+                       TestNoMultipleCallsRemoveAddInCallback) {
+  scoped_ptr<MockCountListener> listener(
+      new MockCountListenerWithReAdd(adb_bridge_));
+
+  adb_bridge_->AddDeviceCountListener(listener.get());
+
+  runner_->Run();
+  EXPECT_EQ(3, listener->invoked_);
+  EXPECT_EQ(listener->invoked_ - 1, scheduler_invoked_);
+}
+
+IN_PROC_BROWSER_TEST_F(DevtoolsAndroidBridgeBrowserTest,
+                       TestNoMultipleCallsRemoveAddOnStart) {
+  scoped_ptr<MockCountListener> listener(new MockCountListener(adb_bridge_));
+
+  adb_bridge_->AddDeviceCountListener(listener.get());
+  adb_bridge_->RemoveDeviceCountListener(listener.get());
+  adb_bridge_->AddDeviceCountListener(listener.get());
+
+  runner_->Run();
+  EXPECT_EQ(1, listener->invoked_);
+  EXPECT_EQ(listener->invoked_ - 1, scheduler_invoked_);
+}
+
+IN_PROC_BROWSER_TEST_F(DevtoolsAndroidBridgeBrowserTest,
+                       TestNoMultipleCallsRemoveAddWhileQueued) {
+  scoped_ptr<MockCountListener> listener(
+      new MockCountListenerWithReAddWhileQueued(adb_bridge_));
+
+  adb_bridge_->AddDeviceCountListener(listener.get());
+
+  runner_->Run();
+  EXPECT_EQ(2, listener->invoked_);
+  EXPECT_EQ(listener->invoked_ - 1, scheduler_invoked_);
+}

chrome/chrome_tests.gypi

@@ -1027,6 +1027,7 @@
         'browser/devtools/device/adb/mock_adb_server.cc',
         'browser/devtools/device/adb/mock_adb_server.h',
         'browser/devtools/device/port_forwarding_browsertest.cc',
+        'browser/devtools/device/usb/devtools_android_bridge_browsertest.cc',
         'browser/devtools/devtools_sanity_browsertest.cc',
         'browser/dom_distiller/dom_distiller_viewer_source_browsertest.cc',
         'browser/dom_distiller/tab_utils_browsertest.cc',