Provide the DeviceManager service to the renderer directly, no app.

Provide the DeviceManager service to the renderer directly, no app.

The Mojo app model forces us to create a new thread on which to run the
device manager. Inside Chrome this then means that we have to post tasks
to the UI thread in order to use the UsbService instance it is
implemented with. This patch removes the devices app from Chrome,
instead registering a factory function for DeviceManagers directly with
the RenderFrameHost. DeviceManagerImpl can then live on the UI thread
which greatly simplfies its implementation. Otherwise we would have to
implement similar thread hopping in DeviceImpl and that would be a lot
of unnecessary code.

Committed: https://crrev.com/869af1c5170c48b8987b7ce4e2c70c9b37f9beb7
Cr-Commit-Position: refs/heads/master@{#352222}

[Reilly Grant (use Gerrit), 2015-09-26 00:05:41]

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

[Reilly Grant (use Gerrit), 2015-09-26 00:05:42]

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

[Reilly Grant (use Gerrit), 2015-09-26 00:06:04]

rockot@ for a general review before I add other OWNERS.

[Ken Rockot(use gerrit already), 2015-09-28 22:23:14]

https://codereview.chromium.org/1371793004/diff/20001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1371793004/diff/20001/chrome/browser/DEPS#new...
chrome/browser/DEPS:122: "+device/devices_app/public",
no longer used, right?

https://codereview.chromium.org/1371793004/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1371793004/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:624: #if !defined(OS_ANDROID) &&
!defined(OS_IOS)
nit: you could simplify slightly by removing the platform guard in
RegisterRenderFrameMojoServices and moving this one into the function
implementation (i.e. so CreateUsbDeviceManager is a no-op on Android and iOS).

https://codereview.chromium.org/1371793004/diff/20001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1371793004/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:620: // Allows to register
browser Mojo services exposes through the
nit: exposed

https://codereview.chromium.org/1371793004/diff/20001/device/devices_app/devi...
File device/devices_app/devices_app.h (right):

https://codereview.chromium.org/1371793004/diff/20001/device/devices_app/devi...
device/devices_app/devices_app.h:28: explicit DevicesApp();
nit: -explicit

https://codereview.chromium.org/1371793004/diff/20001/device/devices_app/usb/...
File device/devices_app/usb/device_manager_impl.cc (right):

https://codereview.chromium.org/1371793004/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:68: observer_.Add(usb_service);
It seems to me like it's not correct to observe the service here but then use
DeviceClient to get the UsbService every other time it's needed. What if the
service goes away so we stop observing it? Is it still correct to use
DeviceClient::GetUsbService() when operating in e.g. GetDevices etc?

So I guess what I'm saying is, maybe we should cache usb_service as a class
member on construction, set it to nullptr on WillDestroyUsbService, and use the
cached pointer everywhere.

[Reilly Grant (use Gerrit), 2015-09-28 23:32:00]

https://codereview.chromium.org/1371793004/diff/20001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1371793004/diff/20001/chrome/browser/DEPS#new...
chrome/browser/DEPS:122: "+device/devices_app/public",
On 2015/09/28 22:23:14, Ken Rockot wrote:
> no longer used, right?

Done.

https://codereview.chromium.org/1371793004/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1371793004/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:624: #if !defined(OS_ANDROID) &&
!defined(OS_IOS)
On 2015/09/28 22:23:14, Ken Rockot wrote:
> nit: you could simplify slightly by removing the platform guard in
> RegisterRenderFrameMojoServices and moving this one into the function
> implementation (i.e. so CreateUsbDeviceManager is a no-op on Android and iOS).

Done.

https://codereview.chromium.org/1371793004/diff/20001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1371793004/diff/20001/content/public/browser/...
content/public/browser/content_browser_client.h:620: // Allows to register
browser Mojo services exposes through the
On 2015/09/28 22:23:14, Ken Rockot wrote:
> nit: exposed

Done.

https://codereview.chromium.org/1371793004/diff/20001/device/devices_app/devi...
File device/devices_app/devices_app.h (right):

https://codereview.chromium.org/1371793004/diff/20001/device/devices_app/devi...
device/devices_app/devices_app.h:28: explicit DevicesApp();
On 2015/09/28 22:23:14, Ken Rockot wrote:
> nit: -explicit

Done.

https://codereview.chromium.org/1371793004/diff/20001/device/devices_app/usb/...
File device/devices_app/usb/device_manager_impl.cc (right):

https://codereview.chromium.org/1371793004/diff/20001/device/devices_app/usb/...
device/devices_app/usb/device_manager_impl.cc:68: observer_.Add(usb_service);
On 2015/09/28 22:23:14, Ken Rockot wrote:
> It seems to me like it's not correct to observe the service here but then use
> DeviceClient to get the UsbService every other time it's needed. What if the
> service goes away so we stop observing it? Is it still correct to use
> DeviceClient::GetUsbService() when operating in e.g. GetDevices etc?
> 
> So I guess what I'm saying is, maybe we should cache usb_service as a class
> member on construction, set it to nullptr on WillDestroyUsbService, and use
the
> cached pointer everywhere.

Done.

[Ken Rockot(use gerrit already), 2015-09-28 23:35:30]

lgtm

[Reilly Grant (use Gerrit), 2015-09-28 23:45:02]

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

[Reilly Grant (use Gerrit), 2015-09-28 23:45:03]

thestig@, please review //chrome/browser/DEPS.

kinuko@, please review //content.

[Lei Zhang, 2015-09-29 00:20:33]

On 2015/09/28 23:45:03, Reilly Grant wrote:
> thestig@, please review //chrome/browser/DEPS.

lgtm, but android bots are red.

[Ken Rockot(use gerrit already), 2015-09-29 00:22:53]

https://codereview.chromium.org/1371793004/diff/40001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1371793004/diff/40001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:627:
mojo::InterfaceRequest<device::usb::DeviceManager> request) {
you should include "device/usb/public/interfaces/device_manager.mojom"
regardless of platform

[Reilly Grant (use Gerrit), 2015-09-29 20:08:19]

reillyg@chromium.org changed reviewers:
- kinuko@chromium.org

[Reilly Grant (use Gerrit), 2015-09-29 20:09:51]

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

[Reilly Grant (use Gerrit), 2015-09-29 20:12:11]

reillyg@chromium.org changed reviewers:
+ jam@chromium.org
- kinuko@chromium.org

[Reilly Grant (use Gerrit), 2015-09-29 20:12:43]

jam@, please take a look at //content instead because you're more familiar with
Mojo.

[Reilly Grant (use Gerrit), 2015-10-01 18:18:43]

On 2015/09/29 20:12:43, Reilly Grant wrote:
> jam@, please take a look at //content instead because you're more familiar
with
> Mojo.

*ping*

[jam, 2015-10-02 17:32:50]

lgtm

[Reilly Grant (use Gerrit), 2015-10-02 17:36:24]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2015-10-02 17:36:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1371793004/#ps80001
(title: "Fix chrome_browser.gypi.")

[commit-bot: I haz the power, 2015-10-02 17:37:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1371793004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1371793004/80001

[commit-bot: I haz the power, 2015-10-02 21:13:47]

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

[commit-bot: I haz the power, 2015-10-02 21:13:48]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Reilly Grant (use Gerrit), 2015-10-02 23:18:39]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2015-10-02 23:20:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1371793004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1371793004/80001

[commit-bot: I haz the power, 2015-10-03 01:54:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-03 01:54:59]

Patchset 5 (id:??) landed as
https://crrev.com/869af1c5170c48b8987b7ce4e2c70c9b37f9beb7
Cr-Commit-Position: refs/heads/master@{#352222}

[Finnur, 2015-10-05 10:18:03]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1382383002/ by finnur@chromium.org.

The reason for reverting is: It is causing a number of layout tests to fail with
error:

01:16:18.824 21620  
[4:4:1005/011618:1968830910:ERROR:web_usb_client_impl.cc(72)] Device manager
connection failed.

See for example:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6/builds/...
.