Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2644233002:
cros: Added a fake fingerprint storage class. (Closed)
|
Created:
3 years, 11 months ago by sammiequon Modified:
3 years, 8 months ago CC:
chromium-reviews, hashimoto+watch_chromium.org, oshima+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Added a fake fingerprint storage class.
A class which is used by the fake biod interfaces. It emulates what the fingerprint scanner daemon should do, saving, deleting enrollments and sending signals.
Part 5 of a 5 patch series:
https://codereview.chromium.org/2567813002/ Adds biometrics manager interface
https://codereview.chromium.org/2581403002/ Adds other interfaces
https://codereview.chromium.org/2578323004/ Adds fake clients
https://codereview.chromium.org/2646793003/ Hook up to chromeos
https://codereview.chromium.org/2644233002/ Add a fake implemntation <<<
BUG=702675
TEST=chromeos_unittests --gtest_filter="FakeBiodBiometricClientTest.*"
Review-Url: https://codereview.chromium.org/2644233002
Cr-Commit-Position: refs/heads/master@{#463926}
Committed: https://chromium.googlesource.com/chromium/src/+/b6dc62f53d4c3b14c0f541897f8d2970be89b84b
Patch Set 1 #Patch Set 2 : Fixed some casts. #Patch Set 3 : Rebased to master. #Patch Set 4 : Nits. #Patch Set 5 : Rebased. #Patch Set 6 : Rebased. #
Total comments: 58
Patch Set 7 : Fixed patch set 6 errors. #Patch Set 8 : Changed fake fingerprint from string to string vec. #
Total comments: 58
Patch Set 9 : Fixed patch set 8 errors. #
Total comments: 1
Patch Set 10 : Rebased. #
Total comments: 34
Patch Set 11 : Fixed patch set 10 errors. #
Total comments: 6
Patch Set 12 : Fixed patch set 11 errors. #
Messages
Total messages: 43 (24 generated)
Description was changed from ========== cros: Added a fake fingerprint storage class. A class which is used by the fake biod interfaces. It emulates what the fingerprint scanner daemon should do, saving, deleting enrollments and sending signals. BUG= TEST=chromeos_unittests --gtest_filter="FakeBiodBiometricClientTest.*" ========== to ========== cros: Added a fake fingerprint storage class. A class which is used by the fake biod interfaces. It emulates what the fingerprint scanner daemon should do, saving, deleting enrollments and sending signals. BUG=702675 TEST=chromeos_unittests --gtest_filter="FakeBiodBiometricClientTest.*" ==========
sammiequon@chromium.org changed reviewers: + steel@chromium.org
steel@ - Please take a look. Thanks!
Description was changed from ========== cros: Added a fake fingerprint storage class. A class which is used by the fake biod interfaces. It emulates what the fingerprint scanner daemon should do, saving, deleting enrollments and sending signals. BUG=702675 TEST=chromeos_unittests --gtest_filter="FakeBiodBiometricClientTest.*" ========== to ========== cros: Added a fake fingerprint storage class. A class which is used by the fake biod interfaces. It emulates what the fingerprint scanner daemon should do, saving, deleting enrollments and sending signals. Part 5 of a 5 patch series: https://codereview.chromium.org/2567813002/ Adds biometrics manager interface https://codereview.chromium.org/2581403002/ Adds other interfaces https://codereview.chromium.org/2578323004/ Adds fake clients https://codereview.chromium.org/2646793003/ Hook up to chromeos https://codereview.chromium.org/2644233002/ Add a fake implemntation <<< BUG=702675 TEST=chromeos_unittests --gtest_filter="FakeBiodBiometricClientTest.*" ==========
sammiequon@chromium.org changed reviewers: + rkc@chromium.org
On 2017/03/17 19:49:12, sammiequon wrote: > steel@ - Please take a look. Thanks! rkc@ - Please take a look. Thanks!
Patchset #6 (id:100001) has been deleted
sammiequon@chromium.org changed reviewers: + derat@chromium.org
derat@ - Please take a look. Thanks!
didn't look at all of the test file https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.cc (right): https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:20: const char kFakeEnrollSessionObjectPath[] = "/fake/EnrollSession/"; i would drop "Fake" from all of these names to make them shorter https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:23: const char kFakeRecordObjectPathHeader[] = "/fake/Record/"; s/Header/Prefix/ https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:40: DCHECK(current_record_) https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:63: // Iterate throught all the records to check if fingerprint is a match and s/throught/through/ https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:65: // then each records' fake fingerprint, but neither of these should ever have s/records'/record's/ https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:72: matches[user_id] = std::vector<std::string>(); you shouldn't need to create this if it's missing; the next line will do so https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:106: DCHECK(current_session_ == FingerprintSession::NONE); DCHECK_EQ https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:139: DCHECK(current_session_ == FingerprintSession::NONE); DCHECK_EQ https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:155: DCHECK(current_session_ == FingerprintSession::ENROLL); DCHECK_EQ https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:156: if (enroll_session_path.value() != kFakeEnrollSessionObjectPath) what does it mean if this happens? isn't the calling code broken in that case? https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:166: DCHECK(current_session_ == FingerprintSession::AUTH); DCHECK_EQ https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:167: if (auth_session_path.value() != kFakeEnrollSessionObjectPath) same question about what this means? DCHECK? log an error? https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:175: if (!label.empty() && records_.find(record_path) != records_.end()) do you expect the caller to pass an empty label? https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:180: auto it = records_.find(record_path); can you just do records_.erase(record_path) if you don't care about bogus paths? https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:26: // providing the same API and storaging fingerprints locally. A fingerprint is nit: storing https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:29: // until and compeleted enroll scan is sent. An attempt scan is also sent with a nit: "until a completed enroll ..." https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:37: NONE = 0, you don't need the '= 0' here https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:49: // |fingerprint| is the fake data of the finger as a char. If |is_complete| is this char/string interface seems a bit odd. why not just add each scan as a string, stick them all in a vector, and iterate through them for auth? https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:82: // Clears and stored and current records from the fake storage. s/and/all/ https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:103: std::unique_ptr<dbus::ObjectPath> current_record_path_; you don't need unique_ptr; just use an empty path when it's unset https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client_unittest.cc (right): https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:5: #include "chromeos/dbus/biod/fake_biod_client.h" add a blank line after this one https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:20: void CopyObjectPath(dbus::ObjectPath* dest_path, instead of copy-and-pasting code from biod_client_unittest.cc move it into a new test_util file that both can use. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:36: *out_num = int{object_path_array.size()}; static_cast https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:45: class TestBiometricsObserver : public BiodClient::Observer { put this into the utils class as well; it looks like it's almost exactly the same as the code in biod_client_unittest.cc. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:111: int NumRecords() { return int{fake_biod_client_.records_.size()}; } again, static_cast https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:118: return int{enrollment_paths.size()}; static_cast https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:141: for (int i = 0; i < int{fingerprint_data.size()} - 1; ++i) { static_cast
Patchset #7 (id:140001) has been deleted
https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.cc (right): https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:20: const char kFakeEnrollSessionObjectPath[] = "/fake/EnrollSession/"; On 2017/04/07 18:27:55, Daniel Erat wrote: > i would drop "Fake" from all of these names to make them shorter Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:23: const char kFakeRecordObjectPathHeader[] = "/fake/Record/"; On 2017/04/07 18:27:56, Daniel Erat wrote: > s/Header/Prefix/ Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:40: On 2017/04/07 18:27:55, Daniel Erat wrote: > DCHECK(current_record_) Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:63: // Iterate throught all the records to check if fingerprint is a match and On 2017/04/07 18:27:56, Daniel Erat wrote: > s/throught/through/ Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:65: // then each records' fake fingerprint, but neither of these should ever have On 2017/04/07 18:27:55, Daniel Erat wrote: > s/records'/record's/ Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:72: matches[user_id] = std::vector<std::string>(); On 2017/04/07 18:27:56, Daniel Erat wrote: > you shouldn't need to create this if it's missing; the next line will do so Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:106: DCHECK(current_session_ == FingerprintSession::NONE); On 2017/04/07 18:27:56, Daniel Erat wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:139: DCHECK(current_session_ == FingerprintSession::NONE); On 2017/04/07 18:27:55, Daniel Erat wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:155: DCHECK(current_session_ == FingerprintSession::ENROLL); On 2017/04/07 18:27:56, Daniel Erat wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:156: if (enroll_session_path.value() != kFakeEnrollSessionObjectPath) On 2017/04/07 18:27:56, Daniel Erat wrote: > what does it mean if this happens? isn't the calling code broken in that case? It means user used the path that was handed out by the class. I guess the calling test would probably fail, yeah. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:166: DCHECK(current_session_ == FingerprintSession::AUTH); On 2017/04/07 18:27:56, Daniel Erat wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:167: if (auth_session_path.value() != kFakeEnrollSessionObjectPath) On 2017/04/07 18:27:56, Daniel Erat wrote: > same question about what this means? DCHECK? log an error? Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:175: if (!label.empty() && records_.find(record_path) != records_.end()) On 2017/04/07 18:27:56, Daniel Erat wrote: > do you expect the caller to pass an empty label? No, but I thought it wouldn't hurt to check. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:180: auto it = records_.find(record_path); On 2017/04/07 18:27:55, Daniel Erat wrote: > can you just do records_.erase(record_path) if you don't care about bogus paths? What does it mean bogus paths? If it does the same thing as before it's good for me. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:26: // providing the same API and storaging fingerprints locally. A fingerprint is On 2017/04/07 18:27:56, Daniel Erat wrote: > nit: storing Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:29: // until and compeleted enroll scan is sent. An attempt scan is also sent with a On 2017/04/07 18:27:56, Daniel Erat wrote: > nit: "until a completed enroll ..." Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:37: NONE = 0, On 2017/04/07 18:27:56, Daniel Erat wrote: > you don't need the '= 0' here Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:49: // |fingerprint| is the fake data of the finger as a char. If |is_complete| is On 2017/04/07 18:27:56, Daniel Erat wrote: > this char/string interface seems a bit odd. why not just add each scan as a > string, stick them all in a vector, and iterate through them for auth? I could do that, but I am wondering why the vector of strings is preferred. Is the fingerprint as a bunch of chars confusing? Or would you rather each scan be more specific (ie. "Sammie's first scan")? (Or neither?) https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:82: // Clears and stored and current records from the fake storage. On 2017/04/07 18:27:56, Daniel Erat wrote: > s/and/all/ Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:103: std::unique_ptr<dbus::ObjectPath> current_record_path_; On 2017/04/07 18:27:56, Daniel Erat wrote: > you don't need unique_ptr; just use an empty path when it's unset Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client_unittest.cc (right): https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:5: #include "chromeos/dbus/biod/fake_biod_client.h" On 2017/04/07 18:27:56, Daniel Erat wrote: > add a blank line after this one Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:20: void CopyObjectPath(dbus::ObjectPath* dest_path, On 2017/04/07 18:27:56, Daniel Erat wrote: > instead of copy-and-pasting code from biod_client_unittest.cc move it into a new > test_util file that both can use. Yeah, sorry that should've been obvious. Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:36: *out_num = int{object_path_array.size()}; On 2017/04/07 18:27:56, Daniel Erat wrote: > static_cast https://google.github.io/styleguide/cppguide.html#Casting I'm confused, I've multiple people telling to go either way. But it seems static_cast is more common..Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:45: class TestBiometricsObserver : public BiodClient::Observer { On 2017/04/07 18:27:56, Daniel Erat wrote: > put this into the utils class as well; it looks like it's almost exactly the > same as the code in biod_client_unittest.cc. Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:111: int NumRecords() { return int{fake_biod_client_.records_.size()}; } On 2017/04/07 18:27:56, Daniel Erat wrote: > again, static_cast Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:118: return int{enrollment_paths.size()}; On 2017/04/07 18:27:56, Daniel Erat wrote: > static_cast Done. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:141: for (int i = 0; i < int{fingerprint_data.size()} - 1; ++i) { On 2017/04/07 18:27:56, Daniel Erat wrote: > static_cast Done.
https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:49: // |fingerprint| is the fake data of the finger as a char. If |is_complete| is On 2017/04/07 20:45:50, sammiequon wrote: > On 2017/04/07 18:27:56, Daniel Erat wrote: > > this char/string interface seems a bit odd. why not just add each scan as a > > string, stick them all in a vector, and iterate through them for auth? > > I could do that, but I am wondering why the vector of strings is preferred. Is > the fingerprint as a bunch of chars confusing? Or would you rather each scan be > more specific (ie. "Sammie's first scan")? (Or neither?) yes, my main thought was that strings are much nicer when it comes to debugging. they're easier to print in c++. it's easier to come up with descriptive, unique strings in tests; if you're using 'a', 'b', etc., it's likely that people will mess up at some point and add a character that's already being used. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client_unittest.cc (right): https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:36: *out_num = int{object_path_array.size()}; On 2017/04/07 20:45:51, sammiequon wrote: > On 2017/04/07 18:27:56, Daniel Erat wrote: > > static_cast > > https://google.github.io/styleguide/cppguide.html#Casting I'm confused, I've > multiple people telling to go either way. But it seems static_cast is more > common..Done. i think that this is one place where chromium style is more specific than google. see the "types" section of https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md. there are only a few instances of 'int{' in *.cc in all of the chromium tree, and about half of them are in fingerprint and smart-unlock code. would you mind updating the fingerprint ones (in a separate change)?
Patchset #8 (id:180001) has been deleted
Dan, I've changed the fake fingerprint from a string to a vector to strings. Please take a look. Thanks! https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:49: // |fingerprint| is the fake data of the finger as a char. If |is_complete| is On 2017/04/07 20:57:25, Daniel Erat wrote: > On 2017/04/07 20:45:50, sammiequon wrote: > > On 2017/04/07 18:27:56, Daniel Erat wrote: > > > this char/string interface seems a bit odd. why not just add each scan as a > > > string, stick them all in a vector, and iterate through them for auth? > > > > I could do that, but I am wondering why the vector of strings is preferred. Is > > the fingerprint as a bunch of chars confusing? Or would you rather each scan > be > > more specific (ie. "Sammie's first scan")? (Or neither?) > > yes, my main thought was that strings are much nicer when it comes to debugging. > they're easier to print in c++. it's easier to come up with descriptive, unique > strings in tests; if you're using 'a', 'b', etc., it's likely that people will > mess up at some point and add a character that's already being used. Acknowledged. https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client_unittest.cc (right): https://codereview.chromium.org/2644233002/diff/120001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:36: *out_num = int{object_path_array.size()}; On 2017/04/07 20:57:25, Daniel Erat wrote: > On 2017/04/07 20:45:51, sammiequon wrote: > > On 2017/04/07 18:27:56, Daniel Erat wrote: > > > static_cast > > > > https://google.github.io/styleguide/cppguide.html#Casting I'm confused, I've > > multiple people telling to go either way. But it seems static_cast is more > > common..Done. > > i think that this is one place where chromium style is more specific than > google. see the "types" section of > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md. > > there are only a few instances of 'int{' in *.cc in all of the chromium tree, > and about half of them are in fingerprint and smart-unlock code. would you mind > updating the fingerprint ones (in a separate change)? Understood, I would say those half are mine :S (smart-unlock = quick-unlock? or maybe my search is different). Filed a bug for myself.
https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_test_utils.cc (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.cc:7: #include "dbus/object_path.h" i think you should include base/logging.h for CHECK() https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_test_utils.h (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:5: #ifndef CHROMEOS_DBUS_BIOD_BIOD_TEST_UTILS_H_ consider renaming this file to test_utils to match the namespace https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:21: void CopyObjectPath(dbus::ObjectPath* dest_path, add brief comments to all of these like: // Copies |src_path| to |dest_path|. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:30: void CopyLabel(std::string* expected_label, const std::string& src_label); CopyString? it's not label-specific. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:38: bool CheckExpectedLastAttemptMatches(const AuthScanMatches& expected_matches); nit: document what this dose; also move non-inlined methods below inlined methods https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:45: nit: remove blank lines between accessors https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:63: void BiodServiceRestarted() override {} move this empty impl to the .cc file: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:68: nit: delete extra blank line https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.cc (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:72: std::string user_id = record.second->user_id; const std::string& ? https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:36: enum class FingerprintSession { move this to the private section https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:84: void Reset(); nit: move this above overrides https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:87: friend FakeBiodClientTest; consider not listing the test class as a friend, since it often encourages tests to become overly reliant on the implementation. since this is a fake, you can probably just add accessors for any members that need to be accessed, right? https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client_unittest.cc (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:36: return static_cast<int>(fake_biod_client_.records_.size()); this feels like it's testing the class's internals more than necessary. why do you care how many records it has internally? it'd probably be better to check that GetRecordsForUser returns the expected records for each user; that's what you actually care about, right? https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:39: int NumRecords(const std::string& user_id) { it'd probably be cleaner to call this GetRecordsForUser() and return the vector of enrollments https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:48: void SendAuthScanDone(const std::string& scan_data) { this is a single call to a public method. can the tests just do this themselves instead? https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:58: if (fingerprint_data.size() < 1) you can just use fingerprint_data.empty() instead, but it's better to ASSERT_FALSE or CHECK here if you want to make sure that test code never calls this with an empty vector. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:65: EXPECT_NE(returned_path, dbus::ObjectPath()); EXPECT_ and ASSERT_ expect parameters to be (expected, actual) rather than (actual, expected); you'll get confusing failure methods otherwise. (it's confusing how this is backwards from how CHECK_EQ expressions are usually written, but blame junit :-P) https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:68: for (int i = 0; i < static_cast<int>(fingerprint_data.size()) - 1; ++i) { instead of casting, just make i be a size_t https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:71: false /* is_complete*/); /* is_complete */ (spaces on both sides) https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:77: true /*is_complete*/); just move this into the above for loop and pass (i == fingerprint_data.size() - 1) as is_complete https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:123: auto kTestFingerprint = GenerateTestFingerprint(2); i think you need to write the type here instead of 'auto', since it isn't obvious from the function call https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:124: // Verify that successful enrollments gets stored as expected. A fingerprint nit: s/gets/get/ https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:152: // Add two fingerprints a and b and start an auth session. is this comment outdated? i don't see 'a' or 'b' https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:162: // enrolled, the observer should received two matches and zero unmatches. s/received/receive/, s/unmatches/non-matches/ https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:168: // Verify that by sending two attempts signals of fingerprints that have not s/attempts/attempt/ https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:169: // been enrolled, the observer should received two unmatches and zero s/received/receive/, s/unmatches/non-matches/ https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:251: kTestUserId, base::Bind(&test_utils::CopyNumRecords, &num_fingerprints)); why copy the number of records here instead of just copying the actual records and comparing .size()? it'd be cleaner to delete CopyNumRecords if you don't actually need it. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:286: const std::string& kLabelOne = "Finger 1"; const std::string (no reference since this isn't actually a reference to some other std::string object)
https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_test_utils.cc (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.cc:7: #include "dbus/object_path.h" On 2017/04/07 23:40:55, Daniel Erat wrote: > i think you should include base/logging.h for CHECK() Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_test_utils.h (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:21: void CopyObjectPath(dbus::ObjectPath* dest_path, On 2017/04/07 23:40:55, Daniel Erat wrote: > add brief comments to all of these like: > > // Copies |src_path| to |dest_path|. Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:30: void CopyLabel(std::string* expected_label, const std::string& src_label); On 2017/04/07 23:40:55, Daniel Erat wrote: > CopyString? it's not label-specific. Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:38: bool CheckExpectedLastAttemptMatches(const AuthScanMatches& expected_matches); On 2017/04/07 23:40:55, Daniel Erat wrote: > nit: document what this dose; also move non-inlined methods below inlined > methods Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:45: On 2017/04/07 23:40:55, Daniel Erat wrote: > nit: remove blank lines between accessors Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:63: void BiodServiceRestarted() override {} On 2017/04/07 23:40:55, Daniel Erat wrote: > move this empty impl to the .cc file: > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:68: On 2017/04/07 23:40:55, Daniel Erat wrote: > nit: delete extra blank line Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.cc (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:72: std::string user_id = record.second->user_id; On 2017/04/07 23:40:56, Daniel Erat wrote: > const std::string& ? Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:36: enum class FingerprintSession { On 2017/04/07 23:40:56, Daniel Erat wrote: > move this to the private section Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:84: void Reset(); On 2017/04/07 23:40:56, Daniel Erat wrote: > nit: move this above overrides Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:87: friend FakeBiodClientTest; On 2017/04/07 23:40:56, Daniel Erat wrote: > consider not listing the test class as a friend, since it often encourages tests > to become overly reliant on the implementation. since this is a fake, you can > probably just add accessors for any members that need to be accessed, right? Acknowledged. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client_unittest.cc (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:36: return static_cast<int>(fake_biod_client_.records_.size()); On 2017/04/07 23:40:56, Daniel Erat wrote: > this feels like it's testing the class's internals more than necessary. why do > you care how many records it has internally? it'd probably be better to check > that GetRecordsForUser returns the expected records for each user; that's what > you actually care about, right? Yeah, think this was a temporary that i forgot to replace. Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:39: int NumRecords(const std::string& user_id) { On 2017/04/07 23:40:56, Daniel Erat wrote: > it'd probably be cleaner to call this GetRecordsForUser() and return the vector > of enrollments Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:48: void SendAuthScanDone(const std::string& scan_data) { On 2017/04/07 23:40:56, Daniel Erat wrote: > this is a single call to a public method. can the tests just do this themselves > instead? I thought it would be nice to make a helper since it was pretty long. Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:58: if (fingerprint_data.size() < 1) On 2017/04/07 23:40:56, Daniel Erat wrote: > you can just use fingerprint_data.empty() instead, but it's better to > ASSERT_FALSE or CHECK here if you want to make sure that test code never calls > this with an empty vector. Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:65: EXPECT_NE(returned_path, dbus::ObjectPath()); On 2017/04/07 23:40:56, Daniel Erat wrote: > EXPECT_ and ASSERT_ expect parameters to be (expected, actual) rather than > (actual, expected); you'll get confusing failure methods otherwise. > > (it's confusing how this is backwards from how CHECK_EQ expressions are usually > written, but blame junit :-P) Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:68: for (int i = 0; i < static_cast<int>(fingerprint_data.size()) - 1; ++i) { On 2017/04/07 23:40:56, Daniel Erat wrote: > instead of casting, just make i be a size_t Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:71: false /* is_complete*/); On 2017/04/07 23:40:56, Daniel Erat wrote: > /* is_complete */ (spaces on both sides) Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:77: true /*is_complete*/); On 2017/04/07 23:40:56, Daniel Erat wrote: > just move this into the above for loop and pass (i == fingerprint_data.size() - > 1) as is_complete Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:123: auto kTestFingerprint = GenerateTestFingerprint(2); On 2017/04/07 23:40:56, Daniel Erat wrote: > i think you need to write the type here instead of 'auto', since it isn't > obvious from the function call Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:124: // Verify that successful enrollments gets stored as expected. A fingerprint On 2017/04/07 23:40:56, Daniel Erat wrote: > nit: s/gets/get/ Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:152: // Add two fingerprints a and b and start an auth session. On 2017/04/07 23:40:56, Daniel Erat wrote: > is this comment outdated? i don't see 'a' or 'b' Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:162: // enrolled, the observer should received two matches and zero unmatches. On 2017/04/07 23:40:56, Daniel Erat wrote: > s/received/receive/, s/unmatches/non-matches/ Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:168: // Verify that by sending two attempts signals of fingerprints that have not On 2017/04/07 23:40:57, Daniel Erat wrote: > s/attempts/attempt/ Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:169: // been enrolled, the observer should received two unmatches and zero On 2017/04/07 23:40:56, Daniel Erat wrote: > s/received/receive/, s/unmatches/non-matches/ Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:251: kTestUserId, base::Bind(&test_utils::CopyNumRecords, &num_fingerprints)); On 2017/04/07 23:40:56, Daniel Erat wrote: > why copy the number of records here instead of just copying the actual records > and comparing .size()? it'd be cleaner to delete CopyNumRecords if you don't > actually need it. Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:286: const std::string& kLabelOne = "Finger 1"; On 2017/04/07 23:40:56, Daniel Erat wrote: > const std::string (no reference since this isn't actually a reference to some > other std::string object) Done. https://codereview.chromium.org/2644233002/diff/220001/chromeos/dbus/biod/tes... File chromeos/dbus/biod/test_utils.h (right): https://codereview.chromium.org/2644233002/diff/220001/chromeos/dbus/biod/tes... chromeos/dbus/biod/test_utils.h:29: void CopyString(std::string* dest_str, const std::string& src_str); Oops will do comment this next cycle.
https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client_unittest.cc (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:48: void SendAuthScanDone(const std::string& scan_data) { On 2017/04/08 00:43:32, sammiequon wrote: > On 2017/04/07 23:40:56, Daniel Erat wrote: > > this is a single call to a public method. can the tests just do this > themselves > > instead? > > I thought it would be nice to make a helper since it was pretty long. Done. i agree that it's long! :-) biod::ScanResult::SUCCESS would be a better name -- you don't need a redundant SCAN_RESULT_ prefix in the value name if you're using an enum class, since it's already namespaced. note also that you can do "using ScanResult = biod::ScanResult" near the top of this file to avoid needing to type the namespace everywhere.
The CQ bit was checked by sammiequon@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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sammiequon@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.
Patchset #10 (id:240001) has been deleted
https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_test_utils.h (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_test_utils.h:5: #ifndef CHROMEOS_DBUS_BIOD_BIOD_TEST_UTILS_H_ On 2017/04/07 23:40:55, Daniel Erat wrote: > consider renaming this file to test_utils to match the namespace Done. https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client_unittest.cc (right): https://codereview.chromium.org/2644233002/diff/200001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:48: void SendAuthScanDone(const std::string& scan_data) { On 2017/04/08 00:57:21, Daniel Erat wrote: > On 2017/04/08 00:43:32, sammiequon wrote: > > On 2017/04/07 23:40:56, Daniel Erat wrote: > > > this is a single call to a public method. can the tests just do this > > themselves > > > instead? > > > > I thought it would be nice to make a helper since it was pretty long. Done. > > i agree that it's long! :-) > > biod::ScanResult::SUCCESS would be a better name -- you don't need a redundant > SCAN_RESULT_ prefix in the value name if you're using an enum class, since it's > already namespaced. > > note also that you can do "using ScanResult = biod::ScanResult" near the top of > this file to avoid needing to type the namespace everywhere. Done.
https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.cc (right): https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:20: const char kEnrollSessionObjectPath[] = "/EnrollSession/"; nit: consider adding a blank line after this line and after kRecordObjectPathPrefix to make this easier to read https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:23: const char RecordObjectPathPrefix[] = "/Record/"; missing the 'k' on the beginning of this constant name https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:68: for (const auto& record : records_) { nit: maybe call this |it| or |pair| or |entry| or something similar instead of |record|; it's actually a pair. feel free to create a const ref named |record| inside of the loop body to make all of the .second calls unnecessary, too https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:26: // sent as strings. If they are successful they get push backed to the current nit: s/push backed/pushed/ or maybe just "added" https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:89: struct FakeRecord { just forward-declare this here and define it in the .cc file? https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:91: ~FakeRecord(); nit: i don't think you need this c'tor and d'tor https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client_unittest.cc (right): https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:53: CHECK(!fingerprint_data.empty()); nit: EXPECT_FALSE or ASSERT_FALSE? https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:66: i == fingerprint_data.size() - 1 /* is_complete*/); nit: add space after is_complete https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:82: DCHECK_GE(scans, 0); EXPECT_GE? (i think you can't use ASSERT_ since this has a return value) https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:115: // that was created with 2 scans should have 1 incomplete scans and 1 complete nit: 1 incomplete scan https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:116: // scans. nit: scan https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:123: // 3 more fingerprints (each with 1 incomplete and 1 complete scans). nit: scan https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:156: // Verify that by sending two attempts signals of fingerprints that have been nit: s/attempts/attempt/ https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:168: EXPECT_EQ(2, observer.num_matched_auth_scans_received()); i was confused by this not matching the comment at first. mind adding a Reset() method to the observer that you can call in the middle of tests so that you don't need to account for previously-received scans? https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:200: // User two has allowed user one to unlock his/her account with his/her nit: "his/her" -> "their" https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/tes... File chromeos/dbus/biod/test_utils.cc (right): https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/tes... chromeos/dbus/biod/test_utils.cc:49: return expected_matches == last_auth_scan_matches_; i don't understand why this method is needed. can you just add an accessor for last_auth_scan_matches_ and use EXPECT_EQ in the test? if that works, it'll give a better error message. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/tes... File chromeos/dbus/biod/test_utils.h (right): https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/tes... chromeos/dbus/biod/test_utils.h:60: // Check's if the last attempt received in BiodAuthScanDoneReceived is equal nit: s/Check's/Check/
https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.cc (right): https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:20: const char kEnrollSessionObjectPath[] = "/EnrollSession/"; On 2017/04/11 17:50:13, Daniel Erat wrote: > nit: consider adding a blank line after this line and after > kRecordObjectPathPrefix to make this easier to read Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:23: const char RecordObjectPathPrefix[] = "/Record/"; On 2017/04/11 17:50:13, Daniel Erat wrote: > missing the 'k' on the beginning of this constant name Oops. Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:68: for (const auto& record : records_) { On 2017/04/11 17:50:13, Daniel Erat wrote: > nit: maybe call this |it| or |pair| or |entry| or something similar instead of > |record|; it's actually a pair. feel free to create a const ref named |record| > inside of the loop body to make all of the .second calls unnecessary, too Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:26: // sent as strings. If they are successful they get push backed to the current On 2017/04/11 17:50:13, Daniel Erat wrote: > nit: s/push backed/pushed/ or maybe just "added" Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:89: struct FakeRecord { On 2017/04/11 17:50:13, Daniel Erat wrote: > just forward-declare this here and define it in the .cc file? Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:91: ~FakeRecord(); On 2017/04/11 17:50:13, Daniel Erat wrote: > nit: i don't think you need this c'tor and d'tor Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client_unittest.cc (right): https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:53: CHECK(!fingerprint_data.empty()); On 2017/04/11 17:50:13, Daniel Erat wrote: > nit: EXPECT_FALSE or ASSERT_FALSE? Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:66: i == fingerprint_data.size() - 1 /* is_complete*/); On 2017/04/11 17:50:14, Daniel Erat wrote: > nit: add space after is_complete Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:82: DCHECK_GE(scans, 0); On 2017/04/11 17:50:13, Daniel Erat wrote: > EXPECT_GE? (i think you can't use ASSERT_ since this has a return value) Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:115: // that was created with 2 scans should have 1 incomplete scans and 1 complete On 2017/04/11 17:50:14, Daniel Erat wrote: > nit: 1 incomplete scan Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:116: // scans. On 2017/04/11 17:50:13, Daniel Erat wrote: > nit: scan Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:123: // 3 more fingerprints (each with 1 incomplete and 1 complete scans). On 2017/04/11 17:50:14, Daniel Erat wrote: > nit: scan Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:156: // Verify that by sending two attempts signals of fingerprints that have been On 2017/04/11 17:50:14, Daniel Erat wrote: > nit: s/attempts/attempt/ Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:168: EXPECT_EQ(2, observer.num_matched_auth_scans_received()); On 2017/04/11 17:50:13, Daniel Erat wrote: > i was confused by this not matching the comment at first. mind adding a Reset() > method to the observer that you can call in the middle of tests so that you > don't need to account for previously-received scans? Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client_unittest.cc:200: // User two has allowed user one to unlock his/her account with his/her On 2017/04/11 17:50:14, Daniel Erat wrote: > nit: "his/her" -> "their" Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/tes... File chromeos/dbus/biod/test_utils.cc (right): https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/tes... chromeos/dbus/biod/test_utils.cc:49: return expected_matches == last_auth_scan_matches_; On 2017/04/11 17:50:14, Daniel Erat wrote: > i don't understand why this method is needed. can you just add an accessor for > last_auth_scan_matches_ and use EXPECT_EQ in the test? if that works, it'll give > a better error message. Yeah, your way is makes much more sense. Done. https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/tes... File chromeos/dbus/biod/test_utils.h (right): https://codereview.chromium.org/2644233002/diff/260001/chromeos/dbus/biod/tes... chromeos/dbus/biod/test_utils.h:60: // Check's if the last attempt received in BiodAuthScanDoneReceived is equal On 2017/04/11 17:50:14, Daniel Erat wrote: > nit: s/Check's/Check/ Done.
https://codereview.chromium.org/2644233002/diff/280001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.cc (right): https://codereview.chromium.org/2644233002/diff/280001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:80: const auto& record = entry.second; nit: instead of 'auto', explicitly give the type here since it's not self-documenting from the context https://codereview.chromium.org/2644233002/diff/280001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2644233002/diff/280001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:22: struct FakeRecord; i'd keep this as a nested class at the top of the 'private' section. it doesn't need to be exposed outside of the implementation, right? https://codereview.chromium.org/2644233002/diff/280001/chromeos/dbus/biod/tes... File chromeos/dbus/biod/test_utils.h (right): https://codereview.chromium.org/2644233002/diff/280001/chromeos/dbus/biod/tes... chromeos/dbus/biod/test_utils.h:59: AuthScanMatches last_auth_scan_matches() const { can this return a const ref instead of a copy?
https://codereview.chromium.org/2644233002/diff/280001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.cc (right): https://codereview.chromium.org/2644233002/diff/280001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:80: const auto& record = entry.second; On 2017/04/11 18:45:21, Daniel Erat wrote: > nit: instead of 'auto', explicitly give the type here since it's not > self-documenting from the context Done. https://codereview.chromium.org/2644233002/diff/280001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2644233002/diff/280001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:22: struct FakeRecord; On 2017/04/11 18:45:21, Daniel Erat wrote: > i'd keep this as a nested class at the top of the 'private' section. it doesn't > need to be exposed outside of the implementation, right? Right, done. https://codereview.chromium.org/2644233002/diff/280001/chromeos/dbus/biod/tes... File chromeos/dbus/biod/test_utils.h (right): https://codereview.chromium.org/2644233002/diff/280001/chromeos/dbus/biod/tes... chromeos/dbus/biod/test_utils.h:59: AuthScanMatches last_auth_scan_matches() const { On 2017/04/11 18:45:21, Daniel Erat wrote: > can this return a const ref instead of a copy? Done.
lgtm
On 2017/04/11 21:24:47, Daniel Erat wrote: > lgtm Thanks!
The CQ bit was checked by sammiequon@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.
The CQ bit was checked by sammiequon@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": 300001, "attempt_start_ts": 1491970013353810,
"parent_rev": "0cc4c62a163318c06636916b7e2b2c6a56f16b74", "commit_rev":
"b6dc62f53d4c3b14c0f541897f8d2970be89b84b"}
Message was sent while issue was closed.
Description was changed from ========== cros: Added a fake fingerprint storage class. A class which is used by the fake biod interfaces. It emulates what the fingerprint scanner daemon should do, saving, deleting enrollments and sending signals. Part 5 of a 5 patch series: https://codereview.chromium.org/2567813002/ Adds biometrics manager interface https://codereview.chromium.org/2581403002/ Adds other interfaces https://codereview.chromium.org/2578323004/ Adds fake clients https://codereview.chromium.org/2646793003/ Hook up to chromeos https://codereview.chromium.org/2644233002/ Add a fake implemntation <<< BUG=702675 TEST=chromeos_unittests --gtest_filter="FakeBiodBiometricClientTest.*" ========== to ========== cros: Added a fake fingerprint storage class. A class which is used by the fake biod interfaces. It emulates what the fingerprint scanner daemon should do, saving, deleting enrollments and sending signals. Part 5 of a 5 patch series: https://codereview.chromium.org/2567813002/ Adds biometrics manager interface https://codereview.chromium.org/2581403002/ Adds other interfaces https://codereview.chromium.org/2578323004/ Adds fake clients https://codereview.chromium.org/2646793003/ Hook up to chromeos https://codereview.chromium.org/2644233002/ Add a fake implemntation <<< BUG=702675 TEST=chromeos_unittests --gtest_filter="FakeBiodBiometricClientTest.*" Review-Url: https://codereview.chromium.org/2644233002 Cr-Commit-Position: refs/heads/master@{#463926} Committed: https://chromium.googlesource.com/chromium/src/+/b6dc62f53d4c3b14c0f541897f8d... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:300001) as https://chromium.googlesource.com/chromium/src/+/b6dc62f53d4c3b14c0f541897f8d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
