Chromium Code Reviews
Help | Chromium Project | Sign in
(41)

Issue 2721223002: Add support for U2fHidDevice interaction (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 4 days ago by Casey Piper
Modified:
5 days, 21 hours 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
3 weeks, 3 days ago (2017-03-01 21:43:13 UTC) #17
Reilly Grant
Now that I see more of how the message and command classes are going to ...
3 weeks, 3 days 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 ...
3 weeks, 1 day ago (2017-03-04 02:06:28 UTC) #19
Reilly Grant
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 ...
2 weeks, 5 days 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 ...
2 weeks, 5 days ago (2017-03-07 00:53:28 UTC) #25
Reilly Grant
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 ...
2 weeks, 5 days 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 ...
2 weeks, 4 days 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 ...
2 weeks, 4 days ago (2017-03-07 22:46:05 UTC) #37
Casey Piper
2 weeks, 4 days ago (2017-03-07 22:46:10 UTC) #38
Reilly Grant
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 ...
2 weeks, 4 days 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 ...
2 weeks, 3 days ago (2017-03-08 23:07:20 UTC) #43
Reilly Grant
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| ...
2 weeks, 3 days 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| ...
2 weeks, 2 days ago (2017-03-10 00:32:10 UTC) #47
Reilly Grant
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: ...
1 week, 5 days 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: ...
6 days ago (2017-03-20 18:22:45 UTC) #61
Reilly Grant
lgtm
6 days 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
6 days 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 days, 23 hours 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 days, 22 hours ago (2017-03-20 20:21:51 UTC) #69
commit-bot: I haz the power
5 days, 21 hours 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 d1a128a62