Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2665493002:
Add builders to APDU command class (Closed)
|
Created:
3 years, 10 months ago by Casey Piper Modified:
3 years, 10 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd builders to APDU command class
Sign, Register and GetVersion APDU messages
are defined as part of the FIDO U2f specification.
Adding static builder methods for the creation
of these messages.
R=reillyg@chromium.org
BUG=686306
Review-Url: https://codereview.chromium.org/2665493002
Cr-Commit-Position: refs/heads/master@{#447122}
Committed: https://chromium.googlesource.com/chromium/src/+/4889a81bbe4d67d614fad10e334a5fc64cd409b7
Patch Set 1 #Patch Set 2 : Remove unused constant #
Total comments: 16
Patch Set 3 : Add suffix for legacy version case and add additional unittests #
Messages
Total messages: 20 (12 generated)
Description was changed from ========== Add builders to APDU command class Sign, Register and GetVersion APDU messages are defined as part of the FIDO U2f specification. Adding static builder methods for the creation of these messages. R=reillyg@chromium.org BUG=NONE ========== to ========== Add builders to APDU command class Sign, Register and GetVersion APDU messages are defined as part of the FIDO U2f specification. Adding static builder methods for the creation of these messages. R=reillyg@chromium.org BUG=NONE ==========
piperc@chromium.org changed required reviewers: + reillyg@chromium.org
Description was changed from ========== Add builders to APDU command class Sign, Register and GetVersion APDU messages are defined as part of the FIDO U2f specification. Adding static builder methods for the creation of these messages. R=reillyg@chromium.org BUG=NONE ========== to ========== Add builders to APDU command class Sign, Register and GetVersion APDU messages are defined as part of the FIDO U2f specification. Adding static builder methods for the creation of these messages. R=reillyg@chromium.org BUG=686306 ==========
piperc@chromium.org changed reviewers: + juanlang@chromium.org, kpaulhamus@chromium.org
Split from https://codereview.chromium.org/2502103002/
Hi Casey, some nits. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... File device/u2f/u2f_apdu_command.cc (right): https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... device/u2f/u2f_apdu_command.cc:151: command->set_data(std::vector<uint8_t>(2, 0)); Comment why two 0 bytes, please. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... File device/u2f/u2f_apdu_command.h (right): https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... device/u2f/u2f_apdu_command.h:40: static scoped_refptr<U2fApduCommand> BuildRegisterCommand( Maybe remove Command? The caller will be using U2fApduCommand::BuildRegisterCommand, and I don't find repeating the word Command all that valuable. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... device/u2f/u2f_apdu_command.h:43: static scoped_refptr<U2fApduCommand> BuildGetVersionCommand(); Remove Get, it's cleaner that way. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... device/u2f/u2f_apdu_command.h:45: static scoped_refptr<U2fApduCommand> BuildGetLegacyVersionCommand(); Ditto. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_uni... File device/u2f/u2f_apdu_unittest.cc (right): https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_uni... device/u2f/u2f_apdu_unittest.cc:185: TEST_F(U2fApduTest, TestBuildApdu) { BuildGetVersionCommand and BuildGetLegacyVersionCommand need tests too, please.
https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... File device/u2f/u2f_apdu_command.cc (right): https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... device/u2f/u2f_apdu_command.cc:131: scoped_refptr<U2fApduCommand> command = U2fApduCommand::Create(); No "U2fApduCommand::" required here and below. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... File device/u2f/u2f_apdu_command.h (right): https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... device/u2f/u2f_apdu_command.h:40: static scoped_refptr<U2fApduCommand> BuildRegisterCommand( On 2017/01/28 00:08:39, juanlang (chromium.org) wrote: > Maybe remove Command? The caller will be using > U2fApduCommand::BuildRegisterCommand, and I don't find repeating the word > Command all that valuable. I suggest U2fApduCommand::CreateRegister to match the methods above. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_uni... File device/u2f/u2f_apdu_unittest.cc (right): https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_uni... device/u2f/u2f_apdu_unittest.cc:185: TEST_F(U2fApduTest, TestBuildApdu) { On 2017/01/28 00:08:39, juanlang (chromium.org) wrote: > BuildGetVersionCommand and BuildGetLegacyVersionCommand need tests too, please. Put each one in its own test method.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... File device/u2f/u2f_apdu_command.cc (right): https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... device/u2f/u2f_apdu_command.cc:131: scoped_refptr<U2fApduCommand> command = U2fApduCommand::Create(); On 2017/01/28 00:29:29, Reilly Grant wrote: > No "U2fApduCommand::" required here and below. Done. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... device/u2f/u2f_apdu_command.cc:151: command->set_data(std::vector<uint8_t>(2, 0)); On 2017/01/28 00:08:39, juanlang (chromium.org) wrote: > Comment why two 0 bytes, please. Done. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... File device/u2f/u2f_apdu_command.h (right): https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... device/u2f/u2f_apdu_command.h:40: static scoped_refptr<U2fApduCommand> BuildRegisterCommand( On 2017/01/28 00:29:29, Reilly Grant wrote: > On 2017/01/28 00:08:39, juanlang (http://chromium.org) wrote: > > Maybe remove Command? The caller will be using > > U2fApduCommand::BuildRegisterCommand, and I don't find repeating the word > > Command all that valuable. > > I suggest U2fApduCommand::CreateRegister to match the methods above. Done. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... device/u2f/u2f_apdu_command.h:40: static scoped_refptr<U2fApduCommand> BuildRegisterCommand( On 2017/01/28 00:08:39, juanlang (chromium.org) wrote: > Maybe remove Command? The caller will be using > U2fApduCommand::BuildRegisterCommand, and I don't find repeating the word > Command all that valuable. Acknowledged. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... device/u2f/u2f_apdu_command.h:43: static scoped_refptr<U2fApduCommand> BuildGetVersionCommand(); On 2017/01/28 00:08:39, juanlang (chromium.org) wrote: > Remove Get, it's cleaner that way. Done. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_com... device/u2f/u2f_apdu_command.h:45: static scoped_refptr<U2fApduCommand> BuildGetLegacyVersionCommand(); On 2017/01/28 00:08:39, juanlang (chromium.org) wrote: > Ditto. Done. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_uni... File device/u2f/u2f_apdu_unittest.cc (right): https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_uni... device/u2f/u2f_apdu_unittest.cc:185: TEST_F(U2fApduTest, TestBuildApdu) { On 2017/01/28 00:08:39, juanlang (chromium.org) wrote: > BuildGetVersionCommand and BuildGetLegacyVersionCommand need tests too, please. Done. https://codereview.chromium.org/2665493002/diff/20001/device/u2f/u2f_apdu_uni... device/u2f/u2f_apdu_unittest.cc:185: TEST_F(U2fApduTest, TestBuildApdu) { On 2017/01/28 00:29:29, Reilly Grant wrote: > On 2017/01/28 00:08:39, juanlang (http://chromium.org) wrote: > > BuildGetVersionCommand and BuildGetLegacyVersionCommand need tests too, > please. > > Put each one in its own test method. Done.
lgtm
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by piperc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485821154819450,
"parent_rev": "581521281404da94a7c8a6281d34c7ac65fa1d9d", "commit_rev":
"4889a81bbe4d67d614fad10e334a5fc64cd409b7"}
Message was sent while issue was closed.
Description was changed from ========== Add builders to APDU command class Sign, Register and GetVersion APDU messages are defined as part of the FIDO U2f specification. Adding static builder methods for the creation of these messages. R=reillyg@chromium.org BUG=686306 ========== to ========== Add builders to APDU command class Sign, Register and GetVersion APDU messages are defined as part of the FIDO U2f specification. Adding static builder methods for the creation of these messages. R=reillyg@chromium.org BUG=686306 Review-Url: https://codereview.chromium.org/2665493002 Cr-Commit-Position: refs/heads/master@{#447122} Committed: https://chromium.googlesource.com/chromium/src/+/4889a81bbe4d67d614fad10e334a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/4889a81bbe4d67d614fad10e334a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
