Move device/usb classes from the FILE thread to UI thread.

Move device/usb classes from the FILE thread to UI thread.

Code that interacts with device/usb often lives on the UI thread. As
with the recent migration of device/hid (issue 422540) moving ownership
of these classes to the UI thread makes calling code substancially
simplier. Blocking operations are handled internally by posting tasks
to the FILE thread and returning a result to the UI thread
asynchronously.

This change paves the way for replacing libusb with platform-specific
implementations of these classes that may have different thread usage
requirements (as is the case in the device/hid code).

BUG=427985
Committed: https://crrev.com/b87cb277526092e101ce29f2ad0393f6247cc39e
Cr-Commit-Position: refs/heads/master@{#325491}

[Reilly Grant (use Gerrit), 2015-04-06 20:54:12]

Patchset #2 (id:20001) has been deleted

[Reilly Grant (use Gerrit), 2015-04-06 20:54:22]

Patchset #1 (id:1) has been deleted

[Reilly Grant (use Gerrit), 2015-04-06 22:09:42]

Patchset #1 (id:40001) has been deleted

[Reilly Grant (use Gerrit), 2015-04-06 22:19:12]

Patchset #1 (id:60001) has been deleted

[Reilly Grant (use Gerrit), 2015-04-06 23:45:00]

reillyg@chromium.org changed reviewers:
+ rockot@chromium.org

[Reilly Grant (use Gerrit), 2015-04-06 23:45:01]

Please take a look at the //device/usb parts of this Ken. I'm so sorry for the
size of this patch. I'll try to get others to review the extensions bits.

[Ken Rockot(use gerrit already), 2015-04-07 22:09:44]

Partial review - still need to look at UsbService; I don't mind doing the
extensions review as well

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device.h
File device/usb/usb_device.h (right):

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device.h#...
device/usb/usb_device.h:25: typedef
base::Callback<void(scoped_refptr<UsbDeviceHandle>)> OpenCallback;
nit: Please use aliases instead of typedefs

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_fi...
File device/usb/usb_device_filter_unittest.cc (right):

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_fi...
device/usb/usb_device_filter_unittest.cc:24: : UsbDevice(vendor_id,
nit: (Here and other places) I'm surprised by some of the formatting conventions
going on in this patch. Is this really what clang_format is doing now?

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_ha...
File device/usb/usb_device_handle.h (right):

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_ha...
device/usb/usb_device_handle.h:33: typedef base::Callback<void(bool)>
UsbSuccessCallback;
name nit: Maybe UsbResultCallback? SuccessCallback might imply that it's only
called upon success, despite the signature.

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_ha...
File device/usb/usb_device_handle_impl.cc (right):

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_ha...
device/usb/usb_device_handle_impl.cc:544: task_runner_->PostTask(FROM_HERE,
base::Bind(callback, true));
nit: Maybe it doesn't matter, but is there realistically any concern over
ClaimInterface (and some of these other methods) supporting reentrancy? It seems
unlikely that calling the callback directly here would be a problem.

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_ha...
File device/usb/usb_device_handle_unittest.cc (right):

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_ha...
device/usb/usb_device_handle_unittest.cc:212: handle->Close();
Is there some reason why this has to be called now but didn't before, or was it
a bug that it was missing before? Though it doesn't appear to be new logic, the
logic around closing/destruction of handles is a bit confusing (i.e., why
doesn't the destructor just call Close, and why doesn't Close also close the
platform handle?)

[Ken Rockot(use gerrit already), 2015-04-08 07:45:57]

https://codereview.chromium.org/980023002/diff/100001/extensions/browser/api/...
File extensions/browser/api/device_permissions_manager.h (right):

https://codereview.chromium.org/980023002/diff/100001/extensions/browser/api/...
extensions/browser/api/device_permissions_manager.h:164: DevicePermissions*
Get(const std::string& extension_id) const;
nit: I find Get and GetForExtension to be confusing names. One is passive while
the other has magic side effects, but they both "get" permissions and they both
do it "for [an] extension." Not immediately sure how to convey side effects
without being annoying.

https://codereview.chromium.org/980023002/diff/100001/extensions/browser/api/...
File extensions/browser/api/device_permissions_prompt.cc (right):

https://codereview.chromium.org/980023002/diff/100001/extensions/browser/api/...
extensions/browser/api/device_permissions_prompt.cc:87:
usb_service_observer_.Add(service);
Why is DevicePermissionsPrompt's observation of UsbService only started when
someone else starts observing DevicePermissionPrompt? i.e. why can't it just
start observing the service in the constructor?

https://codereview.chromium.org/980023002/diff/100001/extensions/browser/api/...
File extensions/browser/api/usb/usb_api.h (right):

https://codereview.chromium.org/980023002/diff/100001/extensions/browser/api/...
extensions/browser/api/usb/usb_api.h:29: class UsbPermissionCheckingFunction :
public UIThreadExtensionFunction {
Inheritance seems like overkill for this. What about making a
UsbPermissionChecker that can be composed into the few function impls which need
it?

[Reilly Grant (use Gerrit), 2015-04-08 21:39:04]

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device.h
File device/usb/usb_device.h (right):

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device.h#...
device/usb/usb_device.h:25: typedef
base::Callback<void(scoped_refptr<UsbDeviceHandle>)> OpenCallback;
On 2015/04/07 22:09:43, Ken Rockot wrote:
> nit: Please use aliases instead of typedefs

Done.

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_fi...
File device/usb/usb_device_filter_unittest.cc (right):

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_fi...
device/usb/usb_device_filter_unittest.cc:24: : UsbDevice(vendor_id,
On 2015/04/07 22:09:43, Ken Rockot wrote:
> nit: (Here and other places) I'm surprised by some of the formatting
conventions
> going on in this patch. Is this really what clang_format is doing now?

Yeah. It's odd because here it prefers one item per line while in function calls
it tries to use as few lines as possible.

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_ha...
File device/usb/usb_device_handle.h (right):

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_ha...
device/usb/usb_device_handle.h:33: typedef base::Callback<void(bool)>
UsbSuccessCallback;
On 2015/04/07 22:09:43, Ken Rockot wrote:
> name nit: Maybe UsbResultCallback? SuccessCallback might imply that it's only
> called upon success, despite the signature.

Done.

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_ha...
File device/usb/usb_device_handle_impl.cc (right):

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_ha...
device/usb/usb_device_handle_impl.cc:544: task_runner_->PostTask(FROM_HERE,
base::Bind(callback, true));
On 2015/04/07 22:09:43, Ken Rockot wrote:
> nit: Maybe it doesn't matter, but is there realistically any concern over
> ClaimInterface (and some of these other methods) supporting reentrancy? It
seems
> unlikely that calling the callback directly here would be a problem.

I'll change it because I've recently decided that no, I don't care about calling
the callback directly.

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_ha...
File device/usb/usb_device_handle_unittest.cc (right):

https://codereview.chromium.org/980023002/diff/80001/device/usb/usb_device_ha...
device/usb/usb_device_handle_unittest.cc:212: handle->Close();
On 2015/04/07 22:09:44, Ken Rockot wrote:
> Is there some reason why this has to be called now but didn't before, or was
it
> a bug that it was missing before? Though it doesn't appear to be new logic,
the
> logic around closing/destruction of handles is a bit confusing (i.e., why
> doesn't the destructor just call Close, and why doesn't Close also close the
> platform handle?)

This was a bug. Since these objects are reference counted without an explicit
Close function the device may remain open for an unknown period of time.

https://codereview.chromium.org/980023002/diff/100001/extensions/browser/api/...
File extensions/browser/api/device_permissions_manager.h (right):

https://codereview.chromium.org/980023002/diff/100001/extensions/browser/api/...
extensions/browser/api/device_permissions_manager.h:164: DevicePermissions*
Get(const std::string& extension_id) const;
On 2015/04/08 07:45:57, Ken Rockot wrote:
> nit: I find Get and GetForExtension to be confusing names. One is passive
while
> the other has magic side effects, but they both "get" permissions and they
both
> do it "for [an] extension." Not immediately sure how to convey side effects
> without being annoying.

An important difference is also that Get() is const while GetForExtension() is
not. I'm going to rename Get() to GetInternal().

https://codereview.chromium.org/980023002/diff/100001/extensions/browser/api/...
File extensions/browser/api/device_permissions_prompt.cc (right):

https://codereview.chromium.org/980023002/diff/100001/extensions/browser/api/...
extensions/browser/api/device_permissions_prompt.cc:87:
usb_service_observer_.Add(service);
On 2015/04/08 07:45:57, Ken Rockot wrote:
> Why is DevicePermissionsPrompt's observation of UsbService only started when
> someone else starts observing DevicePermissionPrompt? i.e. why can't it just
> start observing the service in the constructor?

I want to start observing and call GetDevices at the same time and doing so from
the constructor is dangerous because it result in a lot of calls to other
members before the constructor is complete.

https://codereview.chromium.org/980023002/diff/100001/extensions/browser/api/...
File extensions/browser/api/usb/usb_api.h (right):

https://codereview.chromium.org/980023002/diff/100001/extensions/browser/api/...
extensions/browser/api/usb/usb_api.h:29: class UsbPermissionCheckingFunction :
public UIThreadExtensionFunction {
On 2015/04/08 07:45:57, Ken Rockot wrote:
> Inheritance seems like overkill for this. What about making a
> UsbPermissionChecker that can be composed into the few function impls which
need
> it?

It's helpful to have access to ExtensionFunction members like browser_context()
and extension().

[Ken Rockot(use gerrit already), 2015-04-08 21:52:26]

cool, lgtm

On 2015/04/08 21:39:04, reillyg wrote:
> It's helpful to have access to ExtensionFunction members like
browser_context()
> and extension().

That's fair, and I know lots of other extension functions do the same thing, but
I still think it's an abuse of inheritance to only add utility methods which are
only used by subclass implementation code. You could construct the helper over
the subset of things it needs (like a BrowserContext* and Extension*).

[Reilly Grant (use Gerrit), 2015-04-08 22:12:07]

reillyg@chromium.org changed reviewers:
+ dgozman@chromium.org

[Reilly Grant (use Gerrit), 2015-04-08 22:12:07]

Adding dgozman@ to review the DevTools parts.

[dgozman, 2015-04-09 13:21:10]

I'am wondering how does this make calling code easier? Judging by
chrome/browser/devtools/device/usb/android_usb_device.{h,cc}, code is
approximately similar, just a little more verbose (look at the delta). I hope,
this makes things easier for other callers.

As I understand, we now jump from our special "device" thread (which was
introduced to reduce jank on UI) to UI thread for usb stuff, and then back to
"device" thread. Seems strange. I'm also worried this may affect UI thread
responsiveness.

Looking at usb_device.h, there is new comment:
> Methods other than simple property accessors must be called from the thread on
which this object was created.
This makes me think that we can work with usb on the "device" thread directly
and avoid jumping to UI. What do you think?

[Reilly Grant (use Gerrit), 2015-04-09 16:57:37]

On 2015/04/09 13:21:10, dgozman wrote:
> I'am wondering how does this make calling code easier? Judging by
> chrome/browser/devtools/device/usb/android_usb_device.{h,cc}, code is
> approximately similar, just a little more verbose (look at the delta). I hope,
> this makes things easier for other callers.

The extensions code sees most of the benefit in reduced complexity because most
of the extensions code is concerned about device enumeration, not device I/O.
Enumeration ends up being tied to either Extension objects (which live on the UI
thread) or directly to UI code (like the USB device picker). I/O is limited to a
very few functions that are pretty simple wrappers and again, have to do things
like permissions checks which require state that exists on the UI thread so it's
a net win there.

> As I understand, we now jump from our special "device" thread (which was
> introduced to reduce jank on UI) to UI thread for usb stuff, and then back to
> "device" thread. Seems strange. I'm also worried this may affect UI thread
> responsiveness.

That's a fair concern. I'm curious about the reasons for the special "device"
thread. I didn't actually know about it until I started writing this patch and
realized that while some enumeration was happening on the UI thread there was
also another thread. Where was jank found? Was it in the device APIs or was
there too much processing of data from the device happening on the UI thread to
maintain responsiveness?

> Looking at usb_device.h, there is new comment:
> > Methods other than simple property accessors must be called from the thread
on
> which this object was created.
> This makes me think that we can work with usb on the "device" thread directly
> and avoid jumping to UI. What do you think?

I've considered keeping UsbService and UsbDevice on the UI thread while handing
UsbDeviceHandle to the IO thread. This would be nice because it would provide a
faster path from the renderer process since IPCs arrive on the IO thread but I
first want to finish the planned transition away from libusb. I'd also like to
better understand the reasons for the creation of the "device" thread.

[dgozman, 2015-04-10 11:34:47]

> I've considered keeping UsbService and UsbDevice on the UI thread while
handing
> UsbDeviceHandle to the IO thread.

I thought usb blocking calls are made on FILE thread. I think, we can use FILE
thread for our purposes as well.
Can we have usb interfaces which can work on FILE _and_ UI?

> This would be nice because it would provide a
> faster path from the renderer process since IPCs arrive on the IO thread but I
> first want to finish the planned transition away from libusb. I'd also like to
> better understand the reasons for the creation of the "device" thread.

We had blocking IO calls there, but those are gone. I guess, we have the same
reason as usb: to not block IPCs while doing the work.
We should be able to move to FILE thread completely, make calls to usb and
perform IO (mostly net::Socket) directly there.
That's why I'm interested in direct FILE thread usb interface.

[Reilly Grant (use Gerrit), 2015-04-10 18:25:17]

On 2015/04/10 11:34:47, dgozman wrote:
> > I've considered keeping UsbService and UsbDevice on the UI thread while
> handing
> > UsbDeviceHandle to the IO thread.
> 
> I thought usb blocking calls are made on FILE thread. I think, we can use FILE
> thread for our purposes as well.
> Can we have usb interfaces which can work on FILE _and_ UI?

USB uses the FILE thread under the hood to call platform APIs that provide no
asynchronous alternative. I'm trying to avoid those as much as possible so I'd
rather not expose this to callers.

This change removed the ability to issue USB transfers from any thread and have
the callback run back on that thread. I removed this because it made the
external interface more complicated and because at the time I thought that
DevTools calls originated from the UI thread. I can try adding that back and it
should reduce the size of the diff to android_usb_device.cc.

> > This would be nice because it would provide a
> > faster path from the renderer process since IPCs arrive on the IO thread but
I
> > first want to finish the planned transition away from libusb. I'd also like
to
> > better understand the reasons for the creation of the "device" thread.
> 
> We had blocking IO calls there, but those are gone. I guess, we have the same
> reason as usb: to not block IPCs while doing the work.
> We should be able to move to FILE thread completely, make calls to usb and
> perform IO (mostly net::Socket) directly there.
> That's why I'm interested in direct FILE thread usb interface.

Part of this patch removed APIs that were marked as "blocking" and make them
take callbacks instead. This frees up the implementation to do whatever is
necessary on the given platform. On OS X for example USB operations are most
natural on the main thread because the asynchronous interface uses a CFRunLoop
event source to deliver callbacks.

[dgozman, 2015-04-13 15:52:19]

> USB uses the FILE thread under the hood to call platform APIs that provide no
> asynchronous alternative. I'm trying to avoid those as much as possible so I'd
> rather not expose this to callers.
> 
> This change removed the ability to issue USB transfers from any thread and
have
> the callback run back on that thread. I removed this because it made the
> external interface more complicated and because at the time I thought that
> DevTools calls originated from the UI thread. I can try adding that back and
it
> should reduce the size of the diff to android_usb_device.cc.
This would be nice. Then we can safely migrate to FILE, and everything will
continue to work.

[Reilly Grant (use Gerrit), 2015-04-13 18:01:07]

On 2015/04/13 15:52:19, dgozman wrote:
> > USB uses the FILE thread under the hood to call platform APIs that provide
no
> > asynchronous alternative. I'm trying to avoid those as much as possible so
I'd
> > rather not expose this to callers.
> > 
> > This change removed the ability to issue USB transfers from any thread and
> have
> > the callback run back on that thread. I removed this because it made the
> > external interface more complicated and because at the time I thought that
> > DevTools calls originated from the UI thread. I can try adding that back and
> it
> > should reduce the size of the diff to android_usb_device.cc.
> This would be nice. Then we can safely migrate to FILE, and everything will
> continue to work.

I did a survey of device providers and after this patch it appears that there is
no reason to prefer the FILE thread over the UI or IO threads. The USB backend
will be safe to use on the UI thread (with these changes as they remove all APIs
marked "this may block"). The WebRTC backend already posts tasks to the UI
thread. It is only the ADB backend that uses net::Socket and so should remain on
its own thread or could use the FILE or IO thread.

[Reilly Grant (use Gerrit), 2015-04-13 23:30:24]

I've added back the code to allow USB transfer calls from any thread. Let's
revisit the DevTools code soon because there's a lot of thread hopping at this
point and I think we could defragment it a bit.

[vkuzkokov, 2015-04-15 14:15:45]

vkuzkokov@chromium.org changed reviewers:
+ vkuzkokov@chromium.org

[vkuzkokov, 2015-04-15 14:15:46]

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/android_usb_browsertest.cc (right):

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_browsertest.cc:427: bool
Close(scoped_refptr<UsbDeviceHandle> handle) override { return true; }
nit: keep it on three separate lines.

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_browsertest.cc:628: void
Shutdown() { base::MessageLoop::current()->Quit(); }
Thread-hopping serves a purpose in test. We check that device counting will not
be rescheduled after we remove last listener. As far as I understand we can
remove hopping to FILE though.

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/android_usb_device.cc (right):

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:458: ProcessOutgoing();
This and other |PostTask|'s are ok to remove as long as we guarantee that
|BulkTransfer| doesn't return successful (USB_TRANSFER_COMPLETED) result
synchronously. Otherwise we could have stack overflow. This guarantee should be
reflected in comment to BulkTransfer.

[Reilly Grant (use Gerrit), 2015-04-15 20:03:47]

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/android_usb_browsertest.cc (right):

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_browsertest.cc:427: bool
Close(scoped_refptr<UsbDeviceHandle> handle) override { return true; }
On 2015/04/15 14:15:46, vkuzkokov wrote:
> nit: keep it on three separate lines.

I'll try but clang-format likes to put such definitions on one line.

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_browsertest.cc:628: void
Shutdown() { base::MessageLoop::current()->Quit(); }
On 2015/04/15 14:15:46, vkuzkokov wrote:
> Thread-hopping serves a purpose in test. We check that device counting will
not
> be rescheduled after we remove last listener. As far as I understand we can
> remove hopping to FILE though.

I think this is tested by the AndroidUsbCountTests that check
scheduler_invoked_. The code here just bounces back and forth 10 times before
quitting. There are no checks performed.

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/android_usb_device.cc (right):

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:458: ProcessOutgoing();
On 2015/04/15 14:15:46, vkuzkokov wrote:
> This and other |PostTask|'s are ok to remove as long as we guarantee that
> |BulkTransfer| doesn't return successful (USB_TRANSFER_COMPLETED) result
> synchronously. Otherwise we could have stack overflow. This guarantee should
be
> reflected in comment to BulkTransfer.

I'll replace the PostTask calls in BulkTransfer callbacks.

[vkuzkokov, 2015-04-15 21:58:06]

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/android_usb_browsertest.cc (right):

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_browsertest.cc:628: void
Shutdown() { base::MessageLoop::current()->Quit(); }
On 2015/04/15 20:03:47, reillyg wrote:
> On 2015/04/15 14:15:46, vkuzkokov wrote:
> > Thread-hopping serves a purpose in test. We check that device counting will
> not
> > be rescheduled after we remove last listener. As far as I understand we can
> > remove hopping to FILE though.
> 
> I think this is tested by the AndroidUsbCountTests that check
> scheduler_invoked_. The code here just bounces back and forth 10 times before
> quitting. There are no checks performed.

That last check is enough. Bouncing is needed in case there is a task posted
that would cause extra scheduling.

[Reilly Grant (use Gerrit), 2015-04-15 22:26:45]

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/android_usb_browsertest.cc (right):

https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_browsertest.cc:628: void
Shutdown() { base::MessageLoop::current()->Quit(); }
On 2015/04/15 21:58:06, vkuzkokov wrote:
> On 2015/04/15 20:03:47, reillyg wrote:
> > On 2015/04/15 14:15:46, vkuzkokov wrote:
> > > Thread-hopping serves a purpose in test. We check that device counting
will
> > not
> > > be rescheduled after we remove last listener. As far as I understand we
can
> > > remove hopping to FILE though.
> > 
> > I think this is tested by the AndroidUsbCountTests that check
> > scheduler_invoked_. The code here just bounces back and forth 10 times
before
> > quitting. There are no checks performed.
> 
> That last check is enough. Bouncing is needed in case there is a task posted
> that would cause extra scheduling.

The FILE thread isn't used anymore so I can just use IsIdleForTesting to check
that nothing has been scheduled on the UI thread.

[vkuzkokov, 2015-04-16 12:06:03]

On 2015/04/15 22:26:45, reillyg wrote:
>
https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
> File chrome/browser/devtools/device/usb/android_usb_browsertest.cc (right):
> 
>
https://codereview.chromium.org/980023002/diff/140001/chrome/browser/devtools...
> chrome/browser/devtools/device/usb/android_usb_browsertest.cc:628: void
> Shutdown() { base::MessageLoop::current()->Quit(); }
> On 2015/04/15 21:58:06, vkuzkokov wrote:
> > On 2015/04/15 20:03:47, reillyg wrote:
> > > On 2015/04/15 14:15:46, vkuzkokov wrote:
> > > > Thread-hopping serves a purpose in test. We check that device counting
> will
> > > not
> > > > be rescheduled after we remove last listener. As far as I understand we
> can
> > > > remove hopping to FILE though.
> > > 
> > > I think this is tested by the AndroidUsbCountTests that check
> > > scheduler_invoked_. The code here just bounces back and forth 10 times
> before
> > > quitting. There are no checks performed.
> > 
> > That last check is enough. Bouncing is needed in case there is a task posted
> > that would cause extra scheduling.
> 
> The FILE thread isn't used anymore so I can just use IsIdleForTesting to check
> that nothing has been scheduled on the UI thread.

lgtm

[dgozman, 2015-04-16 12:47:18]

lgtm modulo comments

Vladislav will follow up with devtools-side cleanup to remove unnecessary
thread-hopping here.

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/android_usb_device.cc (right):

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:158: void
CreateDeviceOnInterfaceClaimed(AndroidUsbDevices* devices,
Let's add DCHECK_CURRENTLY_ON(BrowserThread::UI).

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:178: void
OnDeviceOpened(AndroidUsbDevices* devices,
Let's add DCHECK_CURRENTLY_ON(BrowserThread::UI).

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:197: void
OpenAndroidDevice(AndroidUsbDevices* devices,
Let's add DCHECK_CURRENTLY_ON(BrowserThread::UI).

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:244: void
OpenAndroidDevices(
Let's add DCHECK_CURRENTLY_ON(BrowserThread::UI).

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:309: UsbService*
service = device::DeviceClient::Get()->GetUsbService();
Is it safe to get usb service on this thread?

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:313:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
Shouldn't this be current thread instead of UI?

[Reilly Grant (use Gerrit), 2015-04-16 16:24:12]

reillyg@chromium.org changed reviewers:
+ jhawkins@chromium.org

[Reilly Grant (use Gerrit), 2015-04-16 16:24:14]

James, please take a look at the change to chrome_device_client.cc.

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
File chrome/browser/devtools/device/usb/android_usb_device.cc (right):

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:158: void
CreateDeviceOnInterfaceClaimed(AndroidUsbDevices* devices,
On 2015/04/16 12:47:18, dgozman wrote:
> Let's add DCHECK_CURRENTLY_ON(BrowserThread::UI).

Done.

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:178: void
OnDeviceOpened(AndroidUsbDevices* devices,
On 2015/04/16 12:47:18, dgozman wrote:
> Let's add DCHECK_CURRENTLY_ON(BrowserThread::UI).

Done.

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:197: void
OpenAndroidDevice(AndroidUsbDevices* devices,
On 2015/04/16 12:47:18, dgozman wrote:
> Let's add DCHECK_CURRENTLY_ON(BrowserThread::UI).

Done.

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:244: void
OpenAndroidDevices(
On 2015/04/16 12:47:18, dgozman wrote:
> Let's add DCHECK_CURRENTLY_ON(BrowserThread::UI).

Done.

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:309: UsbService*
service = device::DeviceClient::Get()->GetUsbService();
On 2015/04/16 12:47:18, dgozman wrote:
> Is it safe to get usb service on this thread?

Yes.

https://codereview.chromium.org/980023002/diff/180001/chrome/browser/devtools...
chrome/browser/devtools/device/usb/android_usb_device.cc:313:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2015/04/16 12:47:18, dgozman wrote:
> Shouldn't this be current thread instead of UI?

This function runs on the UI thread. I'll change the PostTask use the current
task runner.

[James Hawkins, 2015-04-16 16:44:17]

lgtm

[Reilly Grant (use Gerrit), 2015-04-16 18:27:46]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2015-04-16 18:27:47]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
vkuzkokov@chromium.org, dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/980023002/#ps200001
(title: "Add more thread assertions.")

[commit-bot: I haz the power, 2015-04-16 18:28:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980023002/200001

[commit-bot: I haz the power, 2015-04-16 19:11:13]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2015-04-16 19:12:47]

Patchset 7 (id:??) landed as
https://crrev.com/b87cb277526092e101ce29f2ad0393f6247cc39e
Cr-Commit-Position: refs/heads/master@{#325491}

[angiegvalg, 2015-05-03 03:20:45]

angiegvalg@gmail.com changed reviewers:
+ angiegvalg@gmail.com

[angiegvalg, 2015-05-03 03:20:47]

lgtm

Check my phone