DevTools: do not retain android_usb_devices in their manager.

DevTools: do not retain android_usb_devices in their manager.

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

[pfeldman, 2014-04-09 16:09:32]

PTAL

[Vladislav Kaznacheev, 2014-04-10 14:38:11]

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/adb/android_usb_device.cc (right):

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_device.cc:174: AndroidUsbDevices*
claimed_devices) {
Unclear name (already claimed or newly claimed?) 
new_devices seems a better name.

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_device.cc:198: static void
OpenAndroidDevicesOnFileThread(
's' at the end of 'devices' seems out of place

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_device.cc:261: devices,
I suggest using base::Owned for this parameter.

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_device.cc:308: if (!(*it)->terminated_)
{
AndroidUsbDevice::terminated() exists (but not used yet)

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_device.cc:587: if
(usb_device_->device())
usb_device_ pointer can be set to NULL by a different thread. This frightens me.

How about we bind a copy as a parameter?

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_device.cc:602: terminated_ = true;
terminated_ is always in sync with !usb_device_. Do we need both?

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/adb/android_usb_socket.cc (right):

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_socket.cc:37: is_closed_(false) {
is_closed_ seems to be redundant now and can be replaced with !device_. In fact
it is only used at one point, see my comment at line 118.

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_socket.cc:118: return is_closed_ ? 0 :
net::ERR_SOCKET_NOT_CONNECTED;
Just out of curiosity, why not return net::ERR_SOCKET_NOT_CONNECTED in both
cases?

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_socket.cc:235: while
(!read_requests.empty() && (!read_buffer_.empty() || disconnect)) {
Theoretically you can replace |disconnect| with |is_closed_| or |!device_| and
remove the parameter altogether.

[pfeldman, 2014-04-10 15:37:46]

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/adb/android_usb_device.cc (right):

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_device.cc:174: AndroidUsbDevices*
claimed_devices) {
On 2014/04/10 14:38:11, Vladislav Kaznacheev wrote:
> Unclear name (already claimed or newly claimed?) 
> new_devices seems a better name.

Done.

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_device.cc:198: static void
OpenAndroidDevicesOnFileThread(
On 2014/04/10 14:38:11, Vladislav Kaznacheev wrote:
> 's' at the end of 'devices' seems out of place

Done.

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_device.cc:261: devices,
On 2014/04/10 14:38:11, Vladislav Kaznacheev wrote:
> I suggest using base::Owned for this parameter.

Done.

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_device.cc:308: if (!(*it)->terminated_)
{
On 2014/04/10 14:38:11, Vladislav Kaznacheev wrote:
> AndroidUsbDevice::terminated() exists (but not used yet)

Done.

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_device.cc:587: if
(usb_device_->device())
On 2014/04/10 14:38:11, Vladislav Kaznacheev wrote:
> usb_device_ pointer can be set to NULL by a different thread. This frightens
me.
> 
> How about we bind a copy as a parameter? 

Done.

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_device.cc:602: terminated_ = true;
On 2014/04/10 14:38:11, Vladislav Kaznacheev wrote:
> terminated_ is always in sync with !usb_device_. Do we need both?

Done.

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
File chrome/browser/devtools/adb/android_usb_socket.cc (right):

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_socket.cc:37: is_closed_(false) {
On 2014/04/10 14:38:11, Vladislav Kaznacheev wrote:
> is_closed_ seems to be redundant now and can be replaced with !device_. In
fact
> it is only used at one point, see my comment at line 118.

Done.

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_socket.cc:118: return is_closed_ ? 0 :
net::ERR_SOCKET_NOT_CONNECTED;
On 2014/04/10 14:38:11, Vladislav Kaznacheev wrote:
> Just out of curiosity, why not return net::ERR_SOCKET_NOT_CONNECTED in both
> cases?

Done.

https://codereview.chromium.org/230773003/diff/20001/chrome/browser/devtools/...
chrome/browser/devtools/adb/android_usb_socket.cc:235: while
(!read_requests.empty() && (!read_buffer_.empty() || disconnect)) {
On 2014/04/10 14:38:11, Vladislav Kaznacheev wrote:
> Theoretically you can replace |disconnect| with |is_closed_| or |!device_| and
> remove the parameter altogether.

Lets keep it.

[Vladislav Kaznacheev, 2014-04-10 16:03:36]

lgtm

[pfeldman, 2014-04-10 16:07:11]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2014-04-10 16:07:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pfeldman@chromium.org/230773003/40001

[commit-bot: I haz the power, 2014-04-10 18:48:49]

Change committed as 263037