Add U2F request state machines

Add U2F request state machines

U2fRequest base class is extended by each type of U2F
request. Common functions of enumeration and device iteration
are defined in the base class. U2fSign performs an entire
sign request on any attached U2F devices, and U2FRegister
performs the registration request on the devices.

BUG=686310
Review-Url: https://codereview.chromium.org/2821263005
Cr-Commit-Position: refs/heads/master@{#466529}
Committed: https://chromium.googlesource.com/chromium/src/+/892eb34117f811aa62c821c9129dc53110266e9a

[Casey Piper, 2017-04-19 21:59:01]

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

[commit-bot: I haz the power, 2017-04-19 21:59:33]

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-04-19 22:11:05]

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

[commit-bot: I haz the power, 2017-04-19 22:11:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Casey Piper, 2017-04-20 23:13:37]

Description was changed from

==========
Add U2fRequest base class

U2fRequest base class will be extended by each type of U2F
request. Common functions of enumeration and iteration are defined
in the base class.

BUG=686310
==========

to

==========
Add U2F request state machines

U2fRequest base class is extended by each type of U2F
request. Common functions of enumeration and device iteration
are defined in the base class. U2fSign performs an entire
sign request on any attached U2F devices, and U2FRegister
performs the registration request on the devices.

BUG=686310
==========

[Casey Piper, 2017-04-20 23:13:37]

piperc@chromium.org changed reviewers:
+ juanlang@chromium.org, kpaulhamus@chromium.org, reillyg@chromium.org

[Casey Piper, 2017-04-20 23:13:38]

piperc@chromium.org changed required reviewers:
+ reillyg@chromium.org

[Casey Piper, 2017-04-20 23:13:51]

Patchset #3 (id:40001) has been deleted

[Casey Piper, 2017-04-20 23:13:57]

Patchset #2 (id:20001) has been deleted

[Casey Piper, 2017-04-20 23:14:03]

Patchset #1 (id:1) has been deleted

[Casey Piper, 2017-04-20 23:14:09]

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

[commit-bot: I haz the power, 2017-04-20 23:14:29]

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-04-20 23:17:46]

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

[commit-bot: I haz the power, 2017-04-20 23:17:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Casey Piper, 2017-04-20 23:22:15]

Patchset #1 (id:60001) has been deleted

[Casey Piper, 2017-04-20 23:22:20]

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

[commit-bot: I haz the power, 2017-04-20 23:22:38]

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

[Casey Piper, 2017-04-20 23:31:08]

Hey Reilly, can you take a look?
I can split this into smaller patches if that is preferred.

[Casey Piper, 2017-04-20 23:36:32]

Patchset #1 (id:80001) has been deleted

[Casey Piper, 2017-04-20 23:36:39]

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

[commit-bot: I haz the power, 2017-04-20 23:36:53]

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-04-21 00:12:52]

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

[commit-bot: I haz the power, 2017-04-21 00:12:52]

Dry run: 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_androi...)

[piperc, 2017-04-21 00:31:10]

The CQ bit was checked by piperc@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 00:31:41]

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-04-21 02:15:01]

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

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

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2017-04-21 17:19:17]

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_register.h
File device/u2f/u2f_register.h (right):

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_registe...
device/u2f/u2f_register.h:17: std::vector<uint8_t> app_param,
std::vector is usually passed as a const&. The reason we don't pass std::vector
by value is that it is too easy to forget to std::move it to avoid the copy, as
you've done here.

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_request.cc
File device/u2f/u2f_request.cc (right):

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_request...
device/u2f/u2f_request.cc:109: }
devices_.remove_if([](const std::unique_ptr<U2fDevice>& this_device) {
  return this_device->GetId() == device->GetId();
});

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_request...
device/u2f/u2f_request.cc:119: }
attempted_devices_.remove_if(
    [](const std::unique_ptr<U2fDevice>& this_device) {
      return this_device->GetId() == device->GetId();
    });

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_request.h
File device/u2f/u2f_request.h (right):

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_request...
device/u2f/u2f_request.h:40: const ResponseCallback& cb_;
This should not be a reference as there is no guarantee that it will be valid
after it is set.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_registe...
File device/u2f/u2f_register.cc (right):

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_registe...
device/u2f/u2f_register.cc:28: if (request)
base::MakeUnique cannot fail.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_registe...
device/u2f/u2f_register.cc:35: return;
DCHECK?

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_registe...
File device/u2f/u2f_register_unittest.cc (right):

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_registe...
device/u2f/u2f_register_unittest.cc:72:
request.U2fRequest::devices_.push_back(std::move(device));
Instead of accessing a private member here I would use an AddDeviceForTesting()
method to inject the mock device. You could also then try to share some log the
logic from OnDeviceAdded so that you call Start() and then AddDeviceForTesting()
and it runs through more of the state machine.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_request.h
File device/u2f/u2f_request.h (right):

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_request...
device/u2f/u2f_request.h:18: base::Callback<void(uint8_t status_code,
std::vector<uint8_t> response)>;
Any reason this takes a uint8_t instead of a U2fDevice::ReturnCode? It seems to
result in a lot of casting.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_sign.cc
File device/u2f/u2f_sign.cc (right):

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_sign.cc...
device/u2f/u2f_sign.cc:31: if (request)
base::MakeUnique does not fail.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_sign.cc...
device/u2f/u2f_sign.cc:38: return;
DCHECK?

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_sign.h
File device/u2f/u2f_sign.h (right):

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_sign.h#...
device/u2f/u2f_sign.h:18: std::vector<uint8_t> app_param,
Same comments here about passing vectors by value.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_sign.h#...
device/u2f/u2f_sign.h:34: std::vector<std::vector<uint8_t>> registered_keys_;
Make this vector const so that it is clear that it is safe to pass around
iterators pointing into it.

[Casey Piper, 2017-04-21 18:42:27]

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_registe...
File device/u2f/u2f_register.cc (right):

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_registe...
device/u2f/u2f_register.cc:28: if (request)
On 2017/04/21 17:19:16, Reilly Grant wrote:
> base::MakeUnique cannot fail.

Acknowledged.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_registe...
device/u2f/u2f_register.cc:35: return;
On 2017/04/21 17:19:16, Reilly Grant wrote:
> DCHECK?

Done.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_registe...
File device/u2f/u2f_register_unittest.cc (right):

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_registe...
device/u2f/u2f_register_unittest.cc:72:
request.U2fRequest::devices_.push_back(std::move(device));
On 2017/04/21 17:19:16, Reilly Grant wrote:
> Instead of accessing a private member here I would use an
AddDeviceForTesting()
> method to inject the mock device. You could also then try to share some log
the
> logic from OnDeviceAdded so that you call Start() and then
AddDeviceForTesting()
> and it runs through more of the state machine.

Switched everywhere.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_request.h
File device/u2f/u2f_request.h (right):

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_request...
device/u2f/u2f_request.h:18: base::Callback<void(uint8_t status_code,
std::vector<uint8_t> response)>;
On 2017/04/21 17:19:16, Reilly Grant wrote:
> Any reason this takes a uint8_t instead of a U2fDevice::ReturnCode? It seems
to
> result in a lot of casting.

The goal was to avoid the caller from depending on U2fDevice by returning a
uint.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_sign.cc
File device/u2f/u2f_sign.cc (right):

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_sign.cc...
device/u2f/u2f_sign.cc:31: if (request)
On 2017/04/21 17:19:17, Reilly Grant wrote:
> base::MakeUnique does not fail.

Acknowledged.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_sign.cc...
device/u2f/u2f_sign.cc:38: return;
On 2017/04/21 17:19:16, Reilly Grant wrote:
> DCHECK?

Done.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_sign.h
File device/u2f/u2f_sign.h (right):

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_sign.h#...
device/u2f/u2f_sign.h:18: std::vector<uint8_t> app_param,
On 2017/04/21 17:19:17, Reilly Grant wrote:
> Same comments here about passing vectors by value.

Done.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_sign.h#...
device/u2f/u2f_sign.h:34: std::vector<std::vector<uint8_t>> registered_keys_;
On 2017/04/21 17:19:17, Reilly Grant wrote:
> Make this vector const so that it is clear that it is safe to pass around
> iterators pointing into it.

Done.

[Reilly Grant (use Gerrit), 2017-04-21 20:10:02]

I think you missed my comments on patch 1.

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_request.h
File device/u2f/u2f_request.h (right):

https://codereview.chromium.org/2821263005/diff/120001/device/u2f/u2f_request...
device/u2f/u2f_request.h:18: base::Callback<void(uint8_t status_code,
std::vector<uint8_t> response)>;
On 2017/04/21 18:42:26, Casey Piper wrote:
> On 2017/04/21 17:19:16, Reilly Grant wrote:
> > Any reason this takes a uint8_t instead of a U2fDevice::ReturnCode? It seems
> to
> > result in a lot of casting.
> 
> The goal was to avoid the caller from depending on U2fDevice by returning a
> uint.

I think readability is improved by avoid the casts. If you want to status codes
to a separate header that is fine.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_registe...
File device/u2f/u2f_register_unittest.cc (right):

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_registe...
device/u2f/u2f_register_unittest.cc:18: class U2fRegisterTest : public
testing::Test {
Same comments here as for U2fRequestTest and TestRegisterCallback in
u2f_request_unittests.cc.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_registe...
device/u2f/u2f_register_unittest.cc:72:
request->AddDeviceForTesting(std::move(device));
Even though it's not necessary to make these tests pass we should probably still
call Start() here.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request.cc
File device/u2f/u2f_request.cc (right):

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
device/u2f/u2f_request.cc:157: }
I recommend pulling this out into an AddDevice() function that can be called
here and from OnDeviceAdded() so that this logic doesn't get out sync.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request.h
File device/u2f/u2f_request.h (right):

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
device/u2f/u2f_request.h:40: const ResponseCallback& cb_;
This is still unsafe.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
device/u2f/u2f_request.h:62: void AddDeviceForTesting(std::unique_ptr<U2fDevice>
device);
We actually have a presubmit check that bugs you if you call *ForTesting()
functions in non-testing code so you can make this public which should reduce
the number of FRIEND_TEST_ALL_PREFIXES declarations above.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
File device/u2f/u2f_request_unittest.cc (right):

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
device/u2f/u2f_request_unittest.cc:45: run_loop_() {}
closure_ and run_loop_ don't need to be explicitly listed here.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
device/u2f/u2f_request_unittest.cc:77: };
Since there are no weird setup/teardown ordering issues here this can be
simplified to:

class U2fRequestTest : public testing::Test {
 public:
  U2fRequestTest() : io_thread_(base::TestIOThread::kAutoStart) {}

 protected:
  base::MessageLoopForUI message_loop_;
  base::TestIOThread io_thread_;
  device::MockDeviceClient device_client_;
};

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
device/u2f/u2f_request_unittest.cc:129: request.U2fRequest::IterateDevice();
You don't need list the class explicitly here and below.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_sign_un...
File device/u2f/u2f_sign_unittest.cc (right):

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_sign_un...
device/u2f/u2f_sign_unittest.cc:17: class U2fSignTest : public testing::Test {
Same comments here as for U2fRequestTest and TestRequestCallback as in
u2f_request_unittest.cc.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_sign_un...
device/u2f/u2f_sign_unittest.cc:74:
request->AddDeviceForTesting(std::move(device));
Same comment about Start() here.

[Casey Piper, 2017-04-21 22:55:16]

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_register.h
File device/u2f/u2f_register.h (right):

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_registe...
device/u2f/u2f_register.h:17: std::vector<uint8_t> app_param,
On 2017/04/21 17:19:16, Reilly Grant wrote:
> std::vector is usually passed as a const&. The reason we don't pass
std::vector
> by value is that it is too easy to forget to std::move it to avoid the copy,
as
> you've done here.

Done.

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_request.cc
File device/u2f/u2f_request.cc (right):

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_request...
device/u2f/u2f_request.cc:109: }
On 2017/04/21 17:19:16, Reilly Grant wrote:
> devices_.remove_if([](const std::unique_ptr<U2fDevice>& this_device) {
>   return this_device->GetId() == device->GetId();
> });

Done.

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_request...
device/u2f/u2f_request.cc:119: }
On 2017/04/21 17:19:16, Reilly Grant wrote:
> attempted_devices_.remove_if(
>     [](const std::unique_ptr<U2fDevice>& this_device) {
>       return this_device->GetId() == device->GetId();
>     });

Done.

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_request.h
File device/u2f/u2f_request.h (right):

https://codereview.chromium.org/2821263005/diff/100001/device/u2f/u2f_request...
device/u2f/u2f_request.h:40: const ResponseCallback& cb_;
On 2017/04/21 17:19:16, Reilly Grant wrote:
> This should not be a reference as there is no guarantee that it will be valid
> after it is set.

Done.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_registe...
File device/u2f/u2f_register_unittest.cc (right):

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_registe...
device/u2f/u2f_register_unittest.cc:18: class U2fRegisterTest : public
testing::Test {
On 2017/04/21 20:10:01, Reilly Grant wrote:
> Same comments here as for U2fRequestTest and TestRegisterCallback in
> u2f_request_unittests.cc.

Done.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_registe...
device/u2f/u2f_register_unittest.cc:72:
request->AddDeviceForTesting(std::move(device));
On 2017/04/21 20:10:01, Reilly Grant wrote:
> Even though it's not necessary to make these tests pass we should probably
still
> call Start() here.

Done.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request.cc
File device/u2f/u2f_request.cc (right):

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
device/u2f/u2f_request.cc:157: }
On 2017/04/21 20:10:01, Reilly Grant wrote:
> I recommend pulling this out into an AddDevice() function that can be called
> here and from OnDeviceAdded() so that this logic doesn't get out sync.

Done.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request.h
File device/u2f/u2f_request.h (right):

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
device/u2f/u2f_request.h:40: const ResponseCallback& cb_;
On 2017/04/21 20:10:01, Reilly Grant wrote:
> This is still unsafe.

Done.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
device/u2f/u2f_request.h:62: void AddDeviceForTesting(std::unique_ptr<U2fDevice>
device);
On 2017/04/21 20:10:01, Reilly Grant wrote:
> We actually have a presubmit check that bugs you if you call *ForTesting()
> functions in non-testing code so you can make this public which should reduce
> the number of FRIEND_TEST_ALL_PREFIXES declarations above.

Done.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
File device/u2f/u2f_request_unittest.cc (right):

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
device/u2f/u2f_request_unittest.cc:45: run_loop_() {}
On 2017/04/21 20:10:01, Reilly Grant wrote:
> closure_ and run_loop_ don't need to be explicitly listed here.

Done.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
device/u2f/u2f_request_unittest.cc:77: };
On 2017/04/21 20:10:02, Reilly Grant wrote:
> Since there are no weird setup/teardown ordering issues here this can be
> simplified to:
> 
> class U2fRequestTest : public testing::Test {
>  public:
>   U2fRequestTest() : io_thread_(base::TestIOThread::kAutoStart) {}
> 
>  protected:
>   base::MessageLoopForUI message_loop_;
>   base::TestIOThread io_thread_;
>   device::MockDeviceClient device_client_;
> };

Done.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_request...
device/u2f/u2f_request_unittest.cc:129: request.U2fRequest::IterateDevice();
On 2017/04/21 20:10:02, Reilly Grant wrote:
> You don't need list the class explicitly here and below.

Done.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_sign_un...
File device/u2f/u2f_sign_unittest.cc (right):

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_sign_un...
device/u2f/u2f_sign_unittest.cc:17: class U2fSignTest : public testing::Test {
On 2017/04/21 20:10:02, Reilly Grant wrote:
> Same comments here as for U2fRequestTest and TestRequestCallback as in
> u2f_request_unittest.cc.

Done.

https://codereview.chromium.org/2821263005/diff/140001/device/u2f/u2f_sign_un...
device/u2f/u2f_sign_unittest.cc:74:
request->AddDeviceForTesting(std::move(device));
On 2017/04/21 20:10:02, Reilly Grant wrote:
> Same comment about Start() here.

Done.

[Reilly Grant (use Gerrit), 2017-04-22 00:16:44]

lgtm with nits

https://codereview.chromium.org/2821263005/diff/160001/device/u2f/BUILD.gn
File device/u2f/BUILD.gn (right):

https://codereview.chromium.org/2821263005/diff/160001/device/u2f/BUILD.gn#ne...
device/u2f/BUILD.gn:28: ]
Add "u2f_return_code.h" here.

https://codereview.chromium.org/2821263005/diff/160001/device/u2f/u2f_registe...
File device/u2f/u2f_register_unittest.cc (right):

https://codereview.chromium.org/2821263005/diff/160001/device/u2f/u2f_registe...
device/u2f/u2f_register_unittest.cc:71: request->Start();
To more closely mimic the normal state machine transitions Start() should be
called first.

https://codereview.chromium.org/2821263005/diff/160001/device/u2f/u2f_sign.h
File device/u2f/u2f_sign.h (right):

https://codereview.chromium.org/2821263005/diff/160001/device/u2f/u2f_sign.h#...
device/u2f/u2f_sign.h:32: std::vector<uint8_t>);
nit (for a later patch): MessageCallback should take a const
std::vector<uint8_t>&.

[Casey Piper, 2017-04-22 00:50:08]

https://codereview.chromium.org/2821263005/diff/160001/device/u2f/BUILD.gn
File device/u2f/BUILD.gn (right):

https://codereview.chromium.org/2821263005/diff/160001/device/u2f/BUILD.gn#ne...
device/u2f/BUILD.gn:28: ]
On 2017/04/22 00:16:43, Reilly Grant wrote:
> Add "u2f_return_code.h" here.

Done.

https://codereview.chromium.org/2821263005/diff/160001/device/u2f/u2f_registe...
File device/u2f/u2f_register_unittest.cc (right):

https://codereview.chromium.org/2821263005/diff/160001/device/u2f/u2f_registe...
device/u2f/u2f_register_unittest.cc:71: request->Start();
On 2017/04/22 00:16:43, Reilly Grant wrote:
> To more closely mimic the normal state machine transitions Start() should be
> called first.

Done.

https://codereview.chromium.org/2821263005/diff/160001/device/u2f/u2f_sign.h
File device/u2f/u2f_sign.h (right):

https://codereview.chromium.org/2821263005/diff/160001/device/u2f/u2f_sign.h#...
device/u2f/u2f_sign.h:32: std::vector<uint8_t>);
On 2017/04/22 00:16:43, Reilly Grant wrote:
> nit (for a later patch): MessageCallback should take a const
> std::vector<uint8_t>&.

Acknowledged.

[Casey Piper, 2017-04-22 00:50:30]

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

[commit-bot: I haz the power, 2017-04-22 00:50:59]

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-04-22 01:57:35]

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

[commit-bot: I haz the power, 2017-04-22 01:57:35]

Dry run: This issue passed the CQ dry run.

[Casey Piper, 2017-04-22 02:48:38]

The CQ bit was checked by piperc@chromium.org

[Casey Piper, 2017-04-22 02:48:38]

The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2821263005/#ps180001
(title: "Call Start before adding devices")

[commit-bot: I haz the power, 2017-04-22 02:48:50]

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-04-22 02:53:44]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492829318192350,
"parent_rev": "cda0932631d6ed3643067a98631f3ba91a5712b5", "commit_rev":
"892eb34117f811aa62c821c9129dc53110266e9a"}

[commit-bot: I haz the power, 2017-04-22 02:53:55]

Description was changed from

==========
Add U2F request state machines

U2fRequest base class is extended by each type of U2F
request. Common functions of enumeration and device iteration
are defined in the base class. U2fSign performs an entire
sign request on any attached U2F devices, and U2FRegister
performs the registration request on the devices.

BUG=686310
==========

to

==========
Add U2F request state machines

U2fRequest base class is extended by each type of U2F
request. Common functions of enumeration and device iteration
are defined in the base class. U2fSign performs an entire
sign request on any attached U2F devices, and U2FRegister
performs the registration request on the devices.

BUG=686310

Review-Url: https://codereview.chromium.org/2821263005
Cr-Commit-Position: refs/heads/master@{#466529}
Committed:
https://chromium.googlesource.com/chromium/src/+/892eb34117f811aa62c821c9129d...
==========

[commit-bot: I haz the power, 2017-04-22 02:53:56]

Committed patchset #5 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/892eb34117f811aa62c821c9129d...

[findit-for-me, 2017-04-22 06:18:26]

Findit(https://goo.gl/kROfz5) identified this CL at revision 466529 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[pkalinnikov, 2017-04-24 11:49:34]

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

The reason for reverting is: Reverting because U2fRequestTest.TestBasicMachine
fails consistently on MSan bots (Linux MSan, Linux ChromiumOS MSan)..