Remove BrowserThread dependency from usb_service.

Remove BrowserThread dependency from usb_service.

Instead of explicitly depending on specific browser threads the USB
service can assume that it is instantiated on BrowserThread::FILE (or
equivalent) and save a TaskRunner reference from this instantiation for
later use when called from other threads.

To reach BrowserThread::UI (required for DBus on Chrome OS) a reference
to the appropriate TaskRunner must be provided when calling
UsbService::GetInstance().

BUG=

Committed: https://crrev.com/e471fab8c731cfc2eacceca8cc5be524c2d6f4b4
Cr-Commit-Position: refs/heads/master@{#292546}

[Reilly Grant (use Gerrit), 2014-08-25 23:10:56]

reillyg@chromium.org changed reviewers:
+ rockot@chromium.org, rpaquay@chromium.org, scheib@chromium.org

[Reilly Grant (use Gerrit), 2014-08-27 22:44:23]

Patchset #2 (id:20001) has been deleted

[Reilly Grant (use Gerrit), 2014-08-27 22:56:48]

At Ken's suggestion I've created a device::DeviceClient class to manage getting
the UsbService instance for the current container. Once
https://codereview.chromium.org/464753002/ lands I'll extend this to provide
HidService instances as well.

[scheib, 2014-08-28 18:18:14]

lgtm after comments added.

https://codereview.chromium.org/507503002/diff/40001/device/common/device_cli...
File device/common/device_client.h (right):

https://codereview.chromium.org/507503002/diff/40001/device/common/device_cli...
device/common/device_client.h:14: class DeviceClient {
Every class definition should have an accompanying comment that describes what
it is for and how it should be used.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Class_Comments
dito elsewhere.

[Reilly Grant (use Gerrit), 2014-08-28 18:35:13]

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

[Reilly Grant (use Gerrit), 2014-08-28 18:35:55]

Adding thestig for high-level //chrome/browser changes and dgozman for
//chrome/browser/devtools changes.

[Lei Zhang, 2014-08-28 18:40:02]

What platforms actually use usb_service?

[Lei Zhang, 2014-08-28 18:44:58]

https://codereview.chromium.org/507503002/diff/60001/chrome/browser/chrome_de...
File chrome/browser/chrome_device_client.h (right):

https://codereview.chromium.org/507503002/diff/60001/chrome/browser/chrome_de...
chrome/browser/chrome_device_client.h:19: virtual usb_service::UsbService*
GetUsbService() OVERRIDE;
nit: Add a comment to say // device::DEviceClient impl

https://codereview.chromium.org/507503002/diff/60001/chrome/browser/chrome_de...
chrome/browser/chrome_device_client.h:20: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/507503002/diff/60001/device/common/device_cli...
File device/common/device_client.cc (right):

https://codereview.chromium.org/507503002/diff/60001/device/common/device_cli...
device/common/device_client.cc:17: DeviceClient::DeviceClient() { g_instance =
this; }
DCHECK |g_instance| is NULL first?

https://codereview.chromium.org/507503002/diff/60001/device/common/device_cli...
device/common/device_client.cc:28: NOTIMPLEMENTED();
This should be NOTREACHED(). NOTIMPLEMENTED() implies you want to actually
implement something here eventually.

https://codereview.chromium.org/507503002/diff/60001/device/common/device_cli...
File device/common/device_client.h (right):

https://codereview.chromium.org/507503002/diff/60001/device/common/device_cli...
device/common/device_client.h:19: // Construction sets the singlse instance.
typo?

[dgozman, 2014-08-28 19:07:26]

chrome/browser/devtools rslgtm

[Reilly Grant (use Gerrit), 2014-08-28 19:25:14]

On 2014/08/28 18:40:02, Lei Zhang wrote:
> What platforms actually use usb_service?

Chrome OS, Linux, OS X and Windows.

[Reilly Grant (use Gerrit), 2014-08-28 19:42:00]

I've addressed thestig's comments and excluded ChromeDeviceClient from mobile
builds (thanks for pointing that out so I would take a second look at the trybot
failures).

[Lei Zhang, 2014-08-28 19:56:06]

On 2014/08/28 19:42:00, reillyg wrote:
> I've addressed thestig's comments and excluded ChromeDeviceClient from mobile
> builds (thanks for pointing that out so I would take a second look at the
trybot
> failures).

Thanks. Looking again. It would be nice in the future if you can hit the "done"
link for each comment you addressed and then send them out. At least I, as a
reviewer, use that as a checksum to make sure all my comments have been
addressed.

[Lei Zhang, 2014-08-28 20:05:32]

https://codereview.chromium.org/507503002/diff/80001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/507503002/diff/80001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.h:304: #if !defined(OS_ANDROID)
What about iOS?

https://codereview.chromium.org/507503002/diff/80001/chrome/browser/devtools/...
File chrome/browser/devtools/device/usb/DEPS (right):

https://codereview.chromium.org/507503002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/usb/DEPS:1: include_rules = [
Do you still need this entire file now that chrome/browser/DEPS have the same
rules?

https://codereview.chromium.org/507503002/diff/80001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/507503002/diff/80001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:2935: '../device/common/common.gyp:device_common',
shouldn't this be down around line 3255?

[Reilly Grant (use Gerrit), 2014-08-28 20:33:55]

https://codereview.chromium.org/507503002/diff/80001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/507503002/diff/80001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.h:304: #if !defined(OS_ANDROID)
On 2014/08/28 20:05:31, Lei Zhang wrote:
> What about iOS?

This file isn't built on iOS.

https://codereview.chromium.org/507503002/diff/80001/chrome/browser/devtools/...
File chrome/browser/devtools/device/usb/DEPS (right):

https://codereview.chromium.org/507503002/diff/80001/chrome/browser/devtools/...
chrome/browser/devtools/device/usb/DEPS:1: include_rules = [
On 2014/08/28 20:05:31, Lei Zhang wrote:
> Do you still need this entire file now that chrome/browser/DEPS have the same
> rules?

I suppose not.

https://codereview.chromium.org/507503002/diff/80001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/507503002/diff/80001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:2935: '../device/common/common.gyp:device_common',
On 2014/08/28 20:05:31, Lei Zhang wrote:
> shouldn't this be down around line 3255?

Agreed, I can move usb_service out of the !dependencies section below that as
well.

[Lei Zhang, 2014-08-28 20:40:20]

chrome/ lgtm if you update chrome/browser/BUILD.gn to match
chrome/chrome_browser.gypi.

[Reilly Grant (use Gerrit), 2014-08-28 20:56:13]

Did the same to chrome/browser/BUILD.gn.

[Ken Rockot(use gerrit already), 2014-08-28 21:47:03]

lgtm

https://codereview.chromium.org/507503002/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/507503002/diff/120001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:30: #include
"chrome/browser/chrome_device_client.h"
Please wrap this in !defined(OS_ANDROID)

https://codereview.chromium.org/507503002/diff/120001/chrome/browser/chrome_d...
File chrome/browser/chrome_device_client.h (right):

https://codereview.chromium.org/507503002/diff/120001/chrome/browser/chrome_d...
chrome/browser/chrome_device_client.h:13: // Implementation of
device::DeviceClient that returns //device API service
nix the "API" imo

https://codereview.chromium.org/507503002/diff/120001/components/usb_service/...
File components/usb_service/usb_device_handle_impl.cc (right):

https://codereview.chromium.org/507503002/diff/120001/components/usb_service/...
components/usb_service/usb_device_handle_impl.cc:215: /* static */ void
LIBUSB_CALL UsbDeviceHandleImpl::PlatformTransferCallback(
// static 

on its own line

https://codereview.chromium.org/507503002/diff/120001/components/usb_service/...
File components/usb_service/usb_service.h (right):

https://codereview.chromium.org/507503002/diff/120001/components/usb_service/...
components/usb_service/usb_service.h:29: // Must be called on a thread with a
MessageLoopIO (normally
MessageLoopForIO? I also might omit any mention of BrowserThread(/::FILE) even
in the comment.

https://codereview.chromium.org/507503002/diff/120001/device/common/BUILD.gn
File device/common/BUILD.gn (right):

https://codereview.chromium.org/507503002/diff/120001/device/common/BUILD.gn#...
device/common/BUILD.gn:5: source_set("common") {
I don't like "common" very much because it carries the connotation that it's OK
to be a dependency of basically any code, browser or renderer. In the context of
chrome this is strictly for browser process usage.

I would recommend //device/core which won't carry the same connotation.

I also don't feel TOO strongly about this, so feel free to argue around it if
you like.

https://codereview.chromium.org/507503002/diff/120001/device/common/device_cl...
File device/common/device_client.cc (right):

https://codereview.chromium.org/507503002/diff/120001/device/common/device_cl...
device/common/device_client.cc:22: DeviceClient::~DeviceClient() { g_instance =
NULL; }
I think it's preferable to use newlines when there's a non-empty body. Only
common exception is class-inlined code.

Technically not a strict style guide violation, but I've had complaints about
this in the past and tend to agree.

https://codereview.chromium.org/507503002/diff/120001/device/common/device_cl...
device/common/device_client.cc:24: /* static */ DeviceClient*
DeviceClient::Get() {
// static

https://codereview.chromium.org/507503002/diff/120001/device/common/device_cl...
File device/common/device_client.h (right):

https://codereview.chromium.org/507503002/diff/120001/device/common/device_cl...
device/common/device_client.h:30: };
Please add the standard private DISALLOW_COPY_AND_ASSIGN boilerplate here as
well.

[Reilly Grant (use Gerrit), 2014-08-28 22:16:13]

https://codereview.chromium.org/507503002/diff/120001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/507503002/diff/120001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:30: #include
"chrome/browser/chrome_device_client.h"
On 2014/08/28 21:47:03, Ken Rockot wrote:
> Please wrap this in !defined(OS_ANDROID)

Done.

https://codereview.chromium.org/507503002/diff/120001/chrome/browser/chrome_d...
File chrome/browser/chrome_device_client.h (right):

https://codereview.chromium.org/507503002/diff/120001/chrome/browser/chrome_d...
chrome/browser/chrome_device_client.h:13: // Implementation of
device::DeviceClient that returns //device API service
On 2014/08/28 21:47:03, Ken Rockot wrote:
> nix the "API" imo

Done.

https://codereview.chromium.org/507503002/diff/120001/components/usb_service/...
File components/usb_service/usb_device_handle_impl.cc (right):

https://codereview.chromium.org/507503002/diff/120001/components/usb_service/...
components/usb_service/usb_device_handle_impl.cc:215: /* static */ void
LIBUSB_CALL UsbDeviceHandleImpl::PlatformTransferCallback(
On 2014/08/28 21:47:03, Ken Rockot wrote:
> // static 
> 
> on its own line

Done.

https://codereview.chromium.org/507503002/diff/120001/components/usb_service/...
File components/usb_service/usb_service.h (right):

https://codereview.chromium.org/507503002/diff/120001/components/usb_service/...
components/usb_service/usb_service.h:29: // Must be called on a thread with a
MessageLoopIO (normally
On 2014/08/28 21:47:03, Ken Rockot wrote:
> MessageLoopForIO? I also might omit any mention of BrowserThread(/::FILE) even
> in the comment.

I'm using it as an example.

https://codereview.chromium.org/507503002/diff/120001/device/common/BUILD.gn
File device/common/BUILD.gn (right):

https://codereview.chromium.org/507503002/diff/120001/device/common/BUILD.gn#...
device/common/BUILD.gn:5: source_set("common") {
On 2014/08/28 21:47:03, Ken Rockot wrote:
> I don't like "common" very much because it carries the connotation that it's
OK
> to be a dependency of basically any code, browser or renderer. In the context
of
> chrome this is strictly for browser process usage.
> 
> I would recommend //device/core which won't carry the same connotation.
> 
> I also don't feel TOO strongly about this, so feel free to argue around it if
> you like.

Done.

https://codereview.chromium.org/507503002/diff/120001/device/common/device_cl...
File device/common/device_client.cc (right):

https://codereview.chromium.org/507503002/diff/120001/device/common/device_cl...
device/common/device_client.cc:22: DeviceClient::~DeviceClient() { g_instance =
NULL; }
On 2014/08/28 21:47:03, Ken Rockot wrote:
> I think it's preferable to use newlines when there's a non-empty body. Only
> common exception is class-inlined code.
> 
> Technically not a strict style guide violation, but I've had complaints about
> this in the past and tend to agree.

Done.

https://codereview.chromium.org/507503002/diff/120001/device/common/device_cl...
device/common/device_client.cc:24: /* static */ DeviceClient*
DeviceClient::Get() {
On 2014/08/28 21:47:03, Ken Rockot wrote:
> // static

Done.

https://codereview.chromium.org/507503002/diff/120001/device/common/device_cl...
File device/common/device_client.h (right):

https://codereview.chromium.org/507503002/diff/120001/device/common/device_cl...
device/common/device_client.h:30: };
On 2014/08/28 21:47:03, Ken Rockot wrote:
> Please add the standard private DISALLOW_COPY_AND_ASSIGN boilerplate here as
> well.

Done.

[Reilly Grant (use Gerrit), 2014-08-28 23:30:03]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2014-08-28 23:31:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reillyg@chromium.org/507503002/140001

[commit-bot: I haz the power, 2014-08-29 00:46:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-29 01:58:58]

Committed patchset #7 (id:140001) as 46a5475ab0ce5076bec3d05d30685537df6c04d3

[commit-bot: I haz the power, 2014-09-10 03:05:11]

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