Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2721223002: Add support for U2fHidDevice interaction (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 2 weeks ago by Casey Piper
Modified:
5 months ago
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add support for U2fHidDevice interaction Add U2fHidDevice class. The class has an internal state machine to abstract to lazily connect and initialize the device upon instruction to transact with the device. BUG=697282 Review-Url: https://codereview.chromium.org/2721223002 Cr-Commit-Position: refs/heads/master@{#458196} Committed: https://chromium.googlesource.com/chromium/src/+/3a4044706b783686a071046c96ba3a09ca781837

Patch Set 1 #

Patch Set 2 : Modify unittest BUILD file #

Total comments: 32

Patch Set 3 : Add queueing of requests if device is busy & change Callback to OnceCallback #

Patch Set 4 : Add missing else statement #

Total comments: 12

Patch Set 5 : Fix nits and segfault #

Total comments: 2

Patch Set 6 : Add comment and change logic for executing pending transactions #

Total comments: 2

Patch Set 7 : Ensure queued callbacks are called in error scenarios #

Total comments: 8

Patch Set 8 : Modify failure tests to use mock connection #

Patch Set 9 : Add //device/hid:mocks to unittest build file #

Total comments: 8

Patch Set 10 : Use MockHidConnection to fail writes in unittest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+810 lines, -10 lines) Patch
M device/BUILD.gn View 1 2 3 4 5 6 7 8 3 chunks +4 lines, -2 lines 0 comments Download
M device/u2f/BUILD.gn View 1 2 2 chunks +5 lines, -0 lines 0 comments Download
M device/u2f/DEPS View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M device/u2f/u2f_device.h View 1 2 3 4 5 chunks +19 lines, -3 lines 0 comments Download
M device/u2f/u2f_device.cc View 1 2 4 chunks +50 lines, -5 lines 0 comments Download
A device/u2f/u2f_hid_device.h View 1 2 3 4 5 6 1 chunk +101 lines, -0 lines 0 comments Download
A device/u2f/u2f_hid_device.cc View 1 2 3 4 5 6 7 1 chunk +314 lines, -0 lines 0 comments Download
A device/u2f/u2f_hid_device_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +316 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Dependent Patchsets:

Messages

Total messages: 72 (52 generated)
Casey Piper
5 months, 2 weeks ago (2017-03-01 21:43:13 UTC) #17
Reilly Grant (use Gerrit)
Now that I see more of how the message and command classes are going to ...
5 months, 2 weeks ago (2017-03-01 22:48:33 UTC) #18
Casey Piper
I agree that I should be able to use std::unique_ptr rather than shared pointers. I ...
5 months, 2 weeks ago (2017-03-04 02:06:28 UTC) #19
Reilly Grant (use Gerrit)
lgtm with nits https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_device.cc File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/40001/device/u2f/u2f_hid_device.cc#newcode277 device/u2f/u2f_hid_device.cc:277: return id.str(); On 2017/03/04 02:06:28, Casey ...
5 months, 2 weeks ago (2017-03-06 20:49:23 UTC) #20
Casey Piper
https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_device.h File device/u2f/u2f_device.h (right): https://codereview.chromium.org/2721223002/diff/80001/device/u2f/u2f_device.h#newcode55 device/u2f/u2f_device.h:55: virtual std::string Id() = 0; On 2017/03/06 20:49:23, Reilly ...
5 months, 2 weeks ago (2017-03-07 00:53:28 UTC) #25
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2721223002/diff/120001/device/u2f/u2f_hid_device.cc File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/120001/device/u2f/u2f_hid_device.cc#newcode265 device/u2f/u2f_hid_device.cc:265: } We should probably document that the reason for ...
5 months, 2 weeks ago (2017-03-07 01:13:25 UTC) #31
davidben
crypto DEPS lgtm. I didn't review the protocol or the rest of the code and ...
5 months, 1 week ago (2017-03-07 19:32:37 UTC) #34
Casey Piper
https://codereview.chromium.org/2721223002/diff/120001/device/u2f/u2f_hid_device.cc File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/120001/device/u2f/u2f_hid_device.cc#newcode265 device/u2f/u2f_hid_device.cc:265: } On 2017/03/07 01:13:25, Reilly Grant wrote: > We ...
5 months, 1 week ago (2017-03-07 22:46:05 UTC) #37
Casey Piper
5 months, 1 week ago (2017-03-07 22:46:10 UTC) #38
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2721223002/diff/140001/device/u2f/u2f_hid_device.cc File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/140001/device/u2f/u2f_hid_device.cc#newcode259 device/u2f/u2f_hid_device.cc:259: if (self && !pending_transactions_.empty()) { I just noticed that ...
5 months, 1 week ago (2017-03-07 23:08:38 UTC) #41
Casey Piper
https://codereview.chromium.org/2721223002/diff/140001/device/u2f/u2f_hid_device.cc File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/140001/device/u2f/u2f_hid_device.cc#newcode259 device/u2f/u2f_hid_device.cc:259: if (self && !pending_transactions_.empty()) { On 2017/03/07 23:08:38, Reilly ...
5 months, 1 week ago (2017-03-08 23:07:20 UTC) #43
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_device.cc File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_device.cc#newcode69 device/u2f/u2f_hid_device.cc:69: // Executing |callback| may have freed |this|. Check |self| ...
5 months, 1 week ago (2017-03-09 00:37:03 UTC) #45
Casey Piper
https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_device.cc File device/u2f/u2f_hid_device.cc (right): https://codereview.chromium.org/2721223002/diff/190001/device/u2f/u2f_hid_device.cc#newcode69 device/u2f/u2f_hid_device.cc:69: // Executing |callback| may have freed |this|. Check |self| ...
5 months, 1 week ago (2017-03-10 00:32:10 UTC) #47
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_device_unittest.cc File device/u2f/u2f_hid_device_unittest.cc (right): https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_device_unittest.cc#newcode285 device/u2f/u2f_hid_device_unittest.cc:285: std::unique_ptr<MockDeviceClient> client(new MockDeviceClient()); auto client = base::MakeUnique<MockDeviceClient>(); https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_device_unittest.cc#newcode299 device/u2f/u2f_hid_device_unittest.cc:299: ...
5 months, 1 week ago (2017-03-14 01:07:41 UTC) #59
Casey Piper
https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_device_unittest.cc File device/u2f/u2f_hid_device_unittest.cc (right): https://codereview.chromium.org/2721223002/diff/310001/device/u2f/u2f_hid_device_unittest.cc#newcode285 device/u2f/u2f_hid_device_unittest.cc:285: std::unique_ptr<MockDeviceClient> client(new MockDeviceClient()); On 2017/03/14 01:07:41, Reilly Grant wrote: ...
5 months ago (2017-03-20 18:22:45 UTC) #61
Reilly Grant (use Gerrit)
lgtm
5 months ago (2017-03-20 18:27:47 UTC) #62
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2721223002/350001
5 months ago (2017-03-20 18:28:56 UTC) #65
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/253633)
5 months ago (2017-03-20 19:21:11 UTC) #67
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2721223002/350001
5 months ago (2017-03-20 20:21:51 UTC) #69
commit-bot: I haz the power
5 months ago (2017-03-20 21:36:56 UTC) #72
Message was sent while issue was closed.
Committed patchset #10 (id:350001) as
https://chromium.googlesource.com/chromium/src/+/3a4044706b783686a071046c96ba...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b