Add device path enumeration to the new Windows USB backend.

Add device path enumeration to the new Windows USB backend.

This patch adds enumeration of USB device paths and detection of USB
device add and removal events. Since no descriptors are read yet the
resulting UsbDevice objects have no useful properties.

BUG=422562
Review-Url: https://codereview.chromium.org/2690383002
Cr-Commit-Position: refs/heads/master@{#451320}
Committed: https://chromium.googlesource.com/chromium/src/+/0843f8c3625094b1b3b9b65935bcd1abfd2c9679

[Reilly Grant (use Gerrit), 2017-02-14 02:06:26]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 02:07:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Reilly Grant (use Gerrit), 2017-02-14 02:07:23]

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

[Reilly Grant (use Gerrit), 2017-02-14 02:07:24]

PTAL

[commit-bot: I haz the power, 2017-02-14 03:33:39]

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

[commit-bot: I haz the power, 2017-02-14 03:33:40]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[scheib, 2017-02-14 19:40:05]

Below, questions about test plan and task runners:

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_device.cc
File device/usb/usb_device.cc (right):

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_device.cc#ne...
device/usb/usb_device.cc:18: : usb_version_(0),
It's fine, but did you consider using default member initializers in the
declaration? Seems more robust to future edits (hard to forget to add in member
list and constructors)

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_service_impl.h
File device/usb/usb_service_impl.h (right):

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_service_impl...
device/usb/usb_service_impl.h:37: class UsbDeviceImpl;
You don't use this, at least in this patch.

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_service_win.cc
File device/usb/usb_service_win.cc (right):

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_service_win....
device/usb/usb_service_win.cc:47: if (SetupDiGetDeviceProperty(dev_info,
dev_info_data, &property,
When bluetooth windows work was underway doing similar things to this, I pushed
back to have a testing wrapper at the OS level and to have test coverage for
code such as this. Super tedious, but does get test coverage for all these
permutations of errors. There are many logic paths here and
GetDeviceInterfaceDetails, GetHubDevicePath, FileThreadHelper.

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_service_win....
device/usb/usb_service_win.cc:242: scoped_refptr<base::SingleThreadTaskRunner>
task_runner_;
I'm still not quickly understanding the hop via task_runner and then
blocking_task_runner. Perhaps that could be explained some here. I'm assuming
task_runner is back on UI thread? Or is that wrong? --- ohh, late breaking, I
see that this class is calling DeviceAdded, not OnDeviceAdded. I need to go see
why/when OnDeviceAdded is called. Ah-ha ha it does bounce through from here to
there via DeviceMonitorWin. I didn't take the time to fully grok that.. I'll
trust you if it's needed. But.. yeah.. demonstrating complexity here - makes me
wonder if we can demonstrate simplicity? :)

[Reilly Grant (use Gerrit), 2017-02-15 22:00:49]

Addressed scheib@'s feedback.

[Reilly Grant (use Gerrit), 2017-02-15 22:03:08]

The CQ bit was checked by reillyg@chromium.org to run a CQ dry run

[Reilly Grant (use Gerrit), 2017-02-15 22:03:13]

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_device.cc
File device/usb/usb_device.cc (right):

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_device.cc#ne...
device/usb/usb_device.cc:18: : usb_version_(0),
On 2017/02/14 19:40:05, scheib wrote:
> It's fine, but did you consider using default member initializers in the
> declaration? Seems more robust to future edits (hard to forget to add in
member
> list and constructors)

Fixed. I was just waiting for another patch to land that removed all but the
guid_ field from this list.

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_service_impl.h
File device/usb/usb_service_impl.h (right):

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_service_impl...
device/usb/usb_service_impl.h:37: class UsbDeviceImpl;
On 2017/02/14 19:40:05, scheib wrote:
> You don't use this, at least in this patch.

Changing headers broke compilation of this file which does require UsbDeviceImpl
to be defined.

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_service_win.cc
File device/usb/usb_service_win.cc (right):

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_service_win....
device/usb/usb_service_win.cc:47: if (SetupDiGetDeviceProperty(dev_info,
dev_info_data, &property,
On 2017/02/14 19:40:05, scheib wrote:
> When bluetooth windows work was underway doing similar things to this, I
pushed
> back to have a testing wrapper at the OS level and to have test coverage for
> code such as this. Super tedious, but does get test coverage for all these
> permutations of errors. There are many logic paths here and
> GetDeviceInterfaceDetails, GetHubDevicePath, FileThreadHelper.

We discussed the test plan offline.

https://codereview.chromium.org/2690383002/diff/1/device/usb/usb_service_win....
device/usb/usb_service_win.cc:242: scoped_refptr<base::SingleThreadTaskRunner>
task_runner_;
On 2017/02/14 19:40:05, scheib wrote:
> I'm still not quickly understanding the hop via task_runner and then
> blocking_task_runner. Perhaps that could be explained some here. I'm assuming
> task_runner is back on UI thread? Or is that wrong? --- ohh, late breaking, I
> see that this class is calling DeviceAdded, not OnDeviceAdded. I need to go
see
> why/when OnDeviceAdded is called. Ah-ha ha it does bounce through from here to
> there via DeviceMonitorWin. I didn't take the time to fully grok that.. I'll
> trust you if it's needed. But.. yeah.. demonstrating complexity here - makes
me
> wonder if we can demonstrate simplicity? :)

We discussed naming offline and I've made a number of changes that should make
the flow clearer.

[commit-bot: I haz the power, 2017-02-15 22:05:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 22:37:20]

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

[commit-bot: I haz the power, 2017-02-15 22:37:21]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[scheib, 2017-02-15 22:41:54]

LGTM with: and pinky promise for awesome test fixture later.

https://codereview.chromium.org/2690383002/diff/20001/device/usb/usb_service_...
File device/usb/usb_service_win.cc (right):

https://codereview.chromium.org/2690383002/diff/20001/device/usb/usb_service_...
device/usb/usb_service_win.cc:254: // Calls back to |service_| must be posted to
|task_runner_|, where that
to |service_task_runner_|,

[Reilly Grant (use Gerrit), 2017-02-16 02:06:48]

Fix a comment.

[Reilly Grant (use Gerrit), 2017-02-16 02:12:49]

The CQ bit was checked by reillyg@chromium.org

[Reilly Grant (use Gerrit), 2017-02-16 02:12:49]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/2690383002/#ps40001
(title: "Fix a comment.")

[commit-bot: I haz the power, 2017-02-16 02:14:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 03:05:40]

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

[commit-bot: I haz the power, 2017-02-16 03:05:41]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Reilly Grant (use Gerrit), 2017-02-16 21:29:54]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2017-02-16 21:30:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 23:05:46]

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

[commit-bot: I haz the power, 2017-02-16 23:05:47]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Reilly Grant (use Gerrit), 2017-02-17 01:48:51]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2017-02-17 01:50:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 03:54:02]

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

[commit-bot: I haz the power, 2017-02-17 03:54:03]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Reilly Grant (use Gerrit), 2017-02-17 15:30:43]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2017-02-17 15:30:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 16:41:13]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487345443312920,
"parent_rev": "32c0c38623575d0441969a8a11b23ee540bf28cb", "commit_rev":
"0843f8c3625094b1b3b9b65935bcd1abfd2c9679"}

[commit-bot: I haz the power, 2017-02-17 16:41:49]

Description was changed from

==========
Add device path enumeration to the new Windows USB backend.

This patch adds enumeration of USB device paths and detection of USB
device add and removal events. Since no descriptors are read yet the
resulting UsbDevice objects have no useful properties.

BUG=422562
==========

to

==========
Add device path enumeration to the new Windows USB backend.

This patch adds enumeration of USB device paths and detection of USB
device add and removal events. Since no descriptors are read yet the
resulting UsbDevice objects have no useful properties.

BUG=422562

Review-Url: https://codereview.chromium.org/2690383002
Cr-Commit-Position: refs/heads/master@{#451320}
Committed:
https://chromium.googlesource.com/chromium/src/+/0843f8c3625094b1b3b9b65935bc...
==========

[commit-bot: I haz the power, 2017-02-17 16:41:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0843f8c3625094b1b3b9b65935bc...