DevTools: Partially redesigned DevToolsAndroidBridge and AndroidDeviceManager

DevTools: Partially redesigned DevToolsAndroidBridge and AndroidDeviceManager.

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

[Vladislav Kaznacheev, 2014-05-19 10:32:11]

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
File chrome/browser/devtools/device/adb/adb_client_socket_browsertest.cc
(right):

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/adb/adb_client_socket_browsertest.cc:140:
chrome->SendJsonRequest("GET /json/version HTTP/1.1\r\n\r\n",
Heads up: you will need to rebase because
https://chromiumcodereview.appspot.com/288353005/ (about to land) will change
the semantics for SendJsonRequest parameter. Pass "json/version" instead.

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/adb/adb_client_socket_browsertest.cc:172:
content::RunMessageLoop();
We have discussed this but I forgot to write this down. Please move
content::RunMessageLoop() from here into StartTest. Same for line 174

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_device_manager.cc (left):

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_device_manager.cc:290: if (stopped_)
I doubt that you could just remove this. This check solved a real problem. If
the callback is called after the last listener's removal then the manager
retains the device indefinitely.

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:232:
device_handle_->android_bridge(),
Looks like this method wants to belong to DeviceHandle, don't you think?

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:311:
device_handles_(device_handles) {
So there is a scenario where DeviceHandle instances are released on different
threads, right?

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:1036:
device_handle_->android_bridge()->device_message_loop()->PostTask(
Lets consider moving it into DeviceHandle as well.

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:1163:
device_handle_->android_bridge()->device_message_loop()->PostTask(FROM_HERE,
Ditto

[vkuzkokov, 2014-05-20 12:34:29]

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
File chrome/browser/devtools/device/adb/adb_client_socket_browsertest.cc
(right):

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/adb/adb_client_socket_browsertest.cc:140:
chrome->SendJsonRequest("GET /json/version HTTP/1.1\r\n\r\n",
On 2014/05/19 10:32:11, Vladislav Kaznacheev wrote:
> Heads up: you will need to rebase because
> https://chromiumcodereview.appspot.com/288353005/ (about to land) will change
> the semantics for SendJsonRequest parameter. Pass "json/version" instead.

Done.

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/adb/adb_client_socket_browsertest.cc:172:
content::RunMessageLoop();
On 2014/05/19 10:32:11, Vladislav Kaznacheev wrote:
> We have discussed this but I forgot to write this down. Please move
> content::RunMessageLoop() from here into StartTest. Same for line 174

Done.

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_device_manager.cc (left):

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_device_manager.cc:290: if (stopped_)
On 2014/05/19 10:32:11, Vladislav Kaznacheev wrote:
> I doubt that you could just remove this. This check solved a real problem. If
> the callback is called after the last listener's removal then the manager
> retains the device indefinitely. 

Devices are freed because DeviceHandle will be destroyed after DiscoveryRequest
is finished.

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_web_socket.cc (right):

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_web_socket.cc:232:
device_handle_->android_bridge(),
On 2014/05/19 10:32:11, Vladislav Kaznacheev wrote:
> Looks like this method wants to belong to DeviceHandle, don't you think?

Done.

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:311:
device_handles_(device_handles) {
On 2014/05/19 10:32:11, Vladislav Kaznacheev wrote:
> So there is a scenario where DeviceHandle instances are released on different
> threads, right?

Yep. One possible solution would be to extend DiscoveryRequest::Callback to
accept 3 parameters: RemoteDevices, DeviceHandleMap, and a vector of
DeviceHandles that need to be freed.

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:1036:
device_handle_->android_bridge()->device_message_loop()->PostTask(
On 2014/05/19 10:32:11, Vladislav Kaznacheev wrote:
> Lets consider moving it into DeviceHandle as well.

Done.

https://codereview.chromium.org/287643002/diff/50001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:1163:
device_handle_->android_bridge()->device_message_loop()->PostTask(FROM_HERE,
On 2014/05/19 10:32:11, Vladislav Kaznacheev wrote:
> Ditto

Done.

[Vladislav Kaznacheev, 2014-05-20 13:06:11]

https://codereview.chromium.org/287643002/diff/70001/chrome/browser/devtools/...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://codereview.chromium.org/287643002/diff/70001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:68: typedef
base::Callback<void(DevToolsAndroidBridge::RemoteDevices*,
While we are at it lets change the types of both parameters scoped_ptr.
scoped_ptr is not copyable, so you will need to call base::Passed(&ptr) instead
of ptr.release()

https://codereview.chromium.org/287643002/diff/70001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:113:
DevToolsAndroidBridge::DeviceHandleMap device_handles_;
I am a bit confused about the names (device_handles_ and used_device_handles).
How about new_* and old_*?

[vkuzkokov, 2014-05-20 14:25:50]

I added all_device_handles to callback so that all DeviceHandles are destroyed
on UI thread.

https://codereview.chromium.org/287643002/diff/70001/chrome/browser/devtools/...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://codereview.chromium.org/287643002/diff/70001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:68: typedef
base::Callback<void(DevToolsAndroidBridge::RemoteDevices*,
On 2014/05/20 13:06:12, Vladislav Kaznacheev wrote:
> While we are at it lets change the types of both parameters scoped_ptr.
> scoped_ptr is not copyable, so you will need to call base::Passed(&ptr)
instead
> of ptr.release()

Done.

https://codereview.chromium.org/287643002/diff/70001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:113:
DevToolsAndroidBridge::DeviceHandleMap device_handles_;
On 2014/05/20 13:06:12, Vladislav Kaznacheev wrote:
> I am a bit confused about the names (device_handles_ and used_device_handles).
> How about new_* and old_*?

I renamed device_handles_ to all_device_handles_ (it contains both old and newly
created handles).

Not all used_device_handles_ are exactly new. I kept the name.

[Vladislav Kaznacheev, 2014-05-20 14:29:58]

lgtm

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

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
File chrome/browser/devtools/device/android_device_manager.cc (right):

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
chrome/browser/devtools/device/android_device_manager.cc:225: void
AndroidDeviceManager::ReleaseDevice(const std::string& serial) {
What if different clients query and release devices. How would that work?

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
File chrome/browser/devtools/device/devtools_android_bridge.cc (right):

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:71:
scoped_ptr<DevToolsAndroidBridge::DeviceHandleMap>)> Callback;
This callback has complex signature, but very neutral name. Consider renaming?

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:273:
(*used_device_handles_)[serial] = device_handle;
So FindDeviceHandle makes it "used"? This is not clear from the method name.
What is used_device_handles_?

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:879: void
DevToolsAndroidBridge::DeviceHandle::SendJsonRequest(
::HttpQuery?

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:897:
android_bridge_->device_message_loop()->PostTask(FROM_HERE,
DCHECK thread.

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:906:
android_bridge_->device_message_loop()->PostTask(FROM_HERE,
ditto

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:1001: device_handles_,
What is device_handles_? What is used_device_handles?

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.cc:1021:
scoped_ptr<DeviceHandleMap> all_device_handles) {
Is this field unused?

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
File chrome/browser/devtools/device/devtools_android_bridge.h (right):

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.h:112: class DeviceHandle
: public base::RefCounted<DeviceHandle> {
This does not need to be public.

https://codereview.chromium.org/287643002/diff/90001/chrome/browser/devtools/...
chrome/browser/devtools/device/devtools_android_bridge.h:135:
scoped_refptr<DevToolsAndroidBridge> android_bridge_;
Introducing devtools_manager field along this one would save you from accessing
it all the time.

[pfeldman, 2014-05-26 09:57:14]

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
File chrome/browser/devtools/device/adb/adb_device_provider.cc (right):

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/adb/adb_device_provider.cc:38: // bool offline =
tokens.size() > 1 && tokens[1] == "offline";
Remove these comments.

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/adb/adb_device_provider.cc:40:
result.push_back(tokens[0]);
You would not need to do this if you were returning the DeviceInfo arrays from
the start.

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
File chrome/browser/devtools/device/android_device_manager.h (right):

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.h:117: class Device :
public base::RefCountedThreadSafe<Device>,
Lets swap them back for better diff.

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.h:154:
scoped_refptr<DeviceProvider> provider_;
On what thread will this be released?

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/usb_device_provider.cc (right):

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/usb_device_provider.cc:21: static void
OnOpenSocket(const UsbDeviceProvider::SocketCallback& callback,
no need to declare as static in anonymous namespace.

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/usb_device_provider.cc:27: static void
OnRead(net::StreamSocket* socket,
ditto

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/usb_device_provider.cc:47: static void
OpenedForCommand(const UsbDeviceProvider::CommandCallback& callback,
ditto

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/usb_device_provider.cc:138: ++it) {
++it on the line above is fine

[vkuzkokov, 2014-05-26 10:54:03]

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
File chrome/browser/devtools/device/adb/adb_device_provider.cc (right):

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/adb/adb_device_provider.cc:38: // bool offline =
tokens.size() > 1 && tokens[1] == "offline";
On 2014/05/26 09:57:14, pfeldman wrote:
> Remove these comments.

Done.

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
File chrome/browser/devtools/device/android_device_manager.h (right):

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.h:117: class Device :
public base::RefCountedThreadSafe<Device>,
On 2014/05/26 09:57:14, pfeldman wrote:
> Lets swap them back for better diff.

Done.

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/android_device_manager.h:154:
scoped_refptr<DeviceProvider> provider_;
On 2014/05/26 09:57:14, pfeldman wrote:
> On what thread will this be released?

UI. Same way as in DevToolsAndroidBridge. It does seem dodgy though.

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/usb_device_provider.cc (right):

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/usb_device_provider.cc:21: static void
OnOpenSocket(const UsbDeviceProvider::SocketCallback& callback,
On 2014/05/26 09:57:14, pfeldman wrote:
> no need to declare as static in anonymous namespace.

Done.

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/usb_device_provider.cc:27: static void
OnRead(net::StreamSocket* socket,
On 2014/05/26 09:57:14, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/usb_device_provider.cc:47: static void
OpenedForCommand(const UsbDeviceProvider::CommandCallback& callback,
On 2014/05/26 09:57:14, pfeldman wrote:
> ditto

Done.

https://codereview.chromium.org/287643002/diff/130001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/usb_device_provider.cc:138: ++it) {
On 2014/05/26 09:57:14, pfeldman wrote:
> ++it on the line above is fine

Done.

[Vladislav Kaznacheev, 2014-05-30 11:44:40]

LGTM with nit

https://chromiumcodereview.appspot.com/287643002/diff/200001/chrome/browser/d...
File chrome/browser/devtools/device/android_device_manager.h (right):

https://chromiumcodereview.appspot.com/287643002/diff/200001/chrome/browser/d...
chrome/browser/devtools/device/android_device_manager.h:114: const
scoped_refptr<DeviceProvider>& provider,
This is not a typical usage, why not just pass scoped_refptr<>?

[vkuzkokov, 2014-05-30 11:46:10]

https://chromiumcodereview.appspot.com/287643002/diff/200001/chrome/browser/d...
File chrome/browser/devtools/device/android_device_manager.h (right):

https://chromiumcodereview.appspot.com/287643002/diff/200001/chrome/browser/d...
chrome/browser/devtools/device/android_device_manager.h:114: const
scoped_refptr<DeviceProvider>& provider,
On 2014/05/30 11:44:40, Vladislav Kaznacheev wrote:
> This is not a typical usage, why not just pass scoped_refptr<>?

Done.

[vkuzkokov, 2014-05-30 11:49:49]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-05-30 11:51:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/287643002/220001

[vkuzkokov, 2014-05-30 11:56:27]

The CQ bit was unchecked by vkuzkokov@chromium.org

[vkuzkokov, 2014-05-30 11:57:01]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-05-30 11:58:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/287643002/220001

[commit-bot: I haz the power, 2014-05-30 15:28:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10967)

[vkuzkokov, 2014-05-30 15:31:59]

The CQ bit was unchecked by vkuzkokov@chromium.org

[vkuzkokov, 2014-05-30 15:37:13]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-05-30 15:37:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/287643002/240001

[commit-bot: I haz the power, 2014-05-30 20:00:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-30 22:04:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-30 22:04:21]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[vkuzkokov, 2014-06-02 07:29:55]

The CQ bit was checked by vkuzkokov@chromium.org

[commit-bot: I haz the power, 2014-06-02 07:30:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vkuzkokov@chromium.org/287643002/260001

[commit-bot: I haz the power, 2014-06-02 12:21:23]

Change committed as 274211