|
|
Created:
4 years 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: DBUS client to interact with fingerprint DBUS API.
Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after.
Part 1 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
TEST=chromeos_unittest --gtest_filter="BiodClientTest.*"
BUG=702675
Review-Url: https://codereview.chromium.org/2567813002
Cr-Commit-Position: refs/heads/master@{#461500}
Committed: https://chromium.googlesource.com/chromium/src/+/0958b7f56e08e90312288a938205dc33fd69d56b
Patch Set 1 #Patch Set 2 : Updated API and names. #Patch Set 3 : Some nits. #Patch Set 4 : Rebased to master. #
Total comments: 4
Patch Set 5 : Fixed patch set 4 errors. #
Total comments: 24
Patch Set 6 : Fixed patch set 5 errors. #Patch Set 7 : ScanFailed -> SessionFailed. #
Total comments: 24
Patch Set 8 : Fixed patch set 7 errors. #
Total comments: 34
Patch Set 9 : Fixed patch set 8 errors. #Patch Set 10 : Addressed comments. #Patch Set 11 : s/BiodBiometricsManagerClient/BiodClient #
Total comments: 53
Patch Set 12 : Fixed patch set 11 errors. #
Total comments: 16
Patch Set 13 : Fixed patch set 12 errors. #
Total comments: 4
Patch Set 14 : Fixed patch set 13 errors. #
Total comments: 2
Patch Set 15 : Fixed patch set 14 errors. #
Dependent Patchsets: Messages
Total messages: 86 (48 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== cros: DBUS client to interact with fingerprint DBUS API. Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after. TEST=chromeos_unittest --gtest_filter="BiometricsClientTest.*" BUG=672269 ========== to ========== cros: DBUS client to interact with fingerprint DBUS API. Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after. TEST=chromeos_unittest --gtest_filter="BiometricsClientTest.*" BUG=672269 ==========
sammiequon@chromium.org changed reviewers: + steel@chromium.org
On 2016/12/13 01:40:01, sammiequon wrote: > mailto:sammiequon@chromium.org changed reviewers: > + mailto:steel@chromium.org steel@ - Please take a look. Thanks!
Patchset #1 (id:20001) has been deleted
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...
Patchset #2 (id:60001) has been deleted
On 2016/12/13 01:40:17, sammiequon wrote: > On 2016/12/13 01:40:01, sammiequon wrote: > > mailto:sammiequon@chromium.org changed reviewers: > > + mailto:steel@chromium.org > > steel@ - Please take a look. Thanks! steel@ - Please take another look. Thanks! Updated to match new API & new names.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Description was changed from ========== cros: DBUS client to interact with fingerprint DBUS API. Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after. TEST=chromeos_unittest --gtest_filter="BiometricsClientTest.*" BUG=672269 ========== to ========== cros: DBUS client to interact with fingerprint DBUS API. Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after. TEST=chromeos_unittest --gtest_filter="BiometricsClientTest.*" BUG=702675 ==========
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.
rkc@chromium.org changed reviewers: + rkc@chromium.org
lgtm % comments https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:274: DBusClientImplementationType type) { Unused? https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client_unittest.cc:239: fake_object_path}; Nit: In lists, it's usually a good idea to have varied data so we can also ensure testing of completeness of data and ordering.
Description was changed from ========== cros: DBUS client to interact with fingerprint DBUS API. Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after. TEST=chromeos_unittest --gtest_filter="BiometricsClientTest.*" BUG=702675 ========== to ========== cros: DBUS client to interact with fingerprint DBUS API. Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after. Part 1 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 TEST=chromeos_unittest --gtest_filter="BiometricsClientTest.*" BUG=702675 ==========
Thanks! https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:274: DBusClientImplementationType type) { On 2017/03/20 23:24:34, rkc wrote: > Unused? Will be used shortly. Commented out for now. https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client_unittest.cc:239: fake_object_path}; On 2017/03/20 23:24:34, rkc wrote: > Nit: In lists, it's usually a good idea to have varied data so we can also > ensure testing of completeness of data and ordering. Done.
sammiequon@chromium.org changed reviewers: + stevenjb@chromium.org
stevenjb@ - Please take a look. Thanks!
https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:23: : biometrics_manager_proxy_(NULL), weak_ptr_factory_(this) {} s/NULL/nullptr throughout https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:195: void SignalConnected(const std::string& interface_name, nit: OnSignalConnected https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:246: matches[user_id] = labels; I think you can use std::move(labels) here to avoid a copy. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client.h (right): https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:42: virtual void EnrollScanDoneReceived(uint32_t scan_result, EnrollScanCompleteReceived ? https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:48: virtual void AuthScanDoneReceived(uint32_t scan_result, Complete? https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:63: virtual void RemoveObserver(Observer* observer) = 0; nit: blank line here https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:69: // paths for a given user. We should either name this type more specifically (e.g. UserRecordsCallback), or move the bulk of this comment to where it is used. Currently it's unclear looking at GetRecordsForUser what the callback contains since it looks like a generic callbakc type. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:77: // Starts the biometric enroll session. Describe what is passed to |callback|. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:82: // Gets all the records registered with this biometric. See above https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:89: // Starts the biometric authentication session. What gets passed to |calback|? https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:92: // Gets the type of biometric. nit: s/gets/requests/ https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:96: virtual biod::BiometricType GetTypeBlocking() = 0; We should avoid using blocking dbus calls like this pretty much always.
Description was changed from ========== cros: DBUS client to interact with fingerprint DBUS API. Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after. Part 1 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 TEST=chromeos_unittest --gtest_filter="BiometricsClientTest.*" BUG=702675 ========== to ========== cros: DBUS client to interact with fingerprint DBUS API. Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after. Part 1 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 TEST=chromeos_unittest --gtest_filter="BiodBiometricsManagerClientTest.*" BUG=702675 ==========
https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:23: : biometrics_manager_proxy_(NULL), weak_ptr_factory_(this) {} On 2017/03/21 18:46:16, stevenjb wrote: > s/NULL/nullptr throughout Done. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:195: void SignalConnected(const std::string& interface_name, On 2017/03/21 18:46:16, stevenjb wrote: > nit: OnSignalConnected Done. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:246: matches[user_id] = labels; On 2017/03/21 18:46:16, stevenjb wrote: > I think you can use std::move(labels) here to avoid a copy. Done. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client.h (right): https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:42: virtual void EnrollScanDoneReceived(uint32_t scan_result, On 2017/03/21 18:46:16, stevenjb wrote: > EnrollScanCompleteReceived ? These signals are based of the dbus signal names found here https://cs.corp.google.com/clankium/src/third_party/cros_system_api/dbus/serv.... There was a long discussion and the folks writing the daemon settled on these names. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:48: virtual void AuthScanDoneReceived(uint32_t scan_result, On 2017/03/21 18:46:16, stevenjb wrote: > Complete? See above. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:63: virtual void RemoveObserver(Observer* observer) = 0; On 2017/03/21 18:46:16, stevenjb wrote: > nit: blank line here Done. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:69: // paths for a given user. On 2017/03/21 18:46:17, stevenjb wrote: > We should either name this type more specifically (e.g. UserRecordsCallback), or > move the bulk of this comment to where it is used. Currently it's unclear > looking at GetRecordsForUser what the callback contains since it looks like a > generic callbakc type. Done. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:77: // Starts the biometric enroll session. On 2017/03/21 18:46:17, stevenjb wrote: > Describe what is passed to |callback|. Done. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:82: // Gets all the records registered with this biometric. On 2017/03/21 18:46:16, stevenjb wrote: > See above Done. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:89: // Starts the biometric authentication session. On 2017/03/21 18:46:16, stevenjb wrote: > What gets passed to |calback|? Done. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:92: // Gets the type of biometric. On 2017/03/21 18:46:16, stevenjb wrote: > nit: s/gets/requests/ Done. https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:96: virtual biod::BiometricType GetTypeBlocking() = 0; On 2017/03/21 18:46:16, stevenjb wrote: > We should avoid using blocking dbus calls like this pretty much always. Done.
lgtm
On 2017/03/21 21:12:01, stevenjb wrote: > lgtm Thanks! Changed some naming to match the changes here https://chromium-review.googlesource.com/c/455465/.
mostly lg, just a couple of comments. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:183: void NameOwnerChangedReceived(const std::string& old_owner, nit: /* old_owner */ /* new_owner */ https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client.h (right): https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:37: virtual void ClientRestarted() {} This isn't the "client" that is restarting though, right? We are the client. biod is the service. Should this be ServiceRestarted instead?
Can we move this to a //chromeos/dbus/biod directory? It is probably time that we start partitioning the //chromeos/dbus directory into more manageable chunks. Steven, any reason we may not want to?
stevenjb@chromium.org changed reviewers: + derat@chromium.org
No reason not to create sub directories under chromeos/dbus that I can think of. derat@, any concerns about that?
i didn't look at the test code but have comments. do you plan to add more biod-related files under //chromeos/dbus? if not, i don't think that a subdirectory makes sense (unless you plan to move everything else to subdirectories). if so, it sounds fine to me. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:109: // Monitor the NameOwnerChanged signal. these comments don't add much; i'd remove them https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:145: dbus::MessageReader reader(response); i think that all of these methods will segfault if there's an error due to e.g. biod not running. you need to check that |response| is non-null. did you test this case? https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:148: callback.Run(result); please make all of these methods log errors if incorrect arguments are supplied. otherwise, all of these methods will silently fail if there's a bug in biod. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:186: observer.ClientRestarted(); this seems incorrect. i think you'll see this method get called twice when biod gets restarted, once with the old client and "", and once with "" and the new client. if biod dies and isn't restarted, you'll see it called as well. calling the "restarted" method when new_owner is empty is misleading. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:205: dbus::MessageReader array_reader(NULL); use nullptr instead of NULL in c++ code (also below) https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client.h (right): https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:29: // BiodBiometricsManagerClient is used to communicate with the biod dbus nit: D-Bus https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:37: virtual void ClientRestarted() {} On 2017/03/25 00:28:20, rkc wrote: > This isn't the "client" that is restarting though, right? We are the client. > biod is the service. > Should this be ServiceRestarted instead? well, biod and chrome are both d-bus clients... but i agree that this name is confusing and that the method should be renamed to ServiceRestarted(). please also consider prefixing this method (and maybe all of the observer methods) with "Biod". when a class implements multiple interfaces, it's difficult to know which one an overridden method named "ServiceRestarted" is coming from. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:42: virtual void EnrollScanDoneReceived(uint32_t scan_result, at least from this comment, i have no idea what |scan_result| contains. is it an enum that needs to be typecast? https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:45: // Called to indicate a bad scan of any kind, or a succesful scan. If scan nit: successful https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:48: virtual void AuthScanDoneReceived(uint32_t scan_result, as above, please document how to use |scan_result| (or better, use a type that makes it obvious)
I specifically want to move everything that I makes sense into sub-directories. I can take that up as a separate cleanup task.
I moved the files to a biod directory since there will a couple more joining in the next few patches. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:109: // Monitor the NameOwnerChanged signal. On 2017/03/27 18:18:25, Daniel Erat wrote: > these comments don't add much; i'd remove them Done. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:145: dbus::MessageReader reader(response); On 2017/03/27 18:18:25, Daniel Erat wrote: > i think that all of these methods will segfault if there's an error due to e.g. > biod not running. you need to check that |response| is non-null. did you test > this case? Done. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:148: callback.Run(result); On 2017/03/27 18:18:25, Daniel Erat wrote: > please make all of these methods log errors if incorrect arguments are supplied. > otherwise, all of these methods will silently fail if there's a bug in biod. Done. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:183: void NameOwnerChangedReceived(const std::string& old_owner, On 2017/03/25 00:28:20, rkc wrote: > nit: /* old_owner */ > /* new_owner */ Done. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:186: observer.ClientRestarted(); On 2017/03/27 18:18:26, Daniel Erat wrote: > this seems incorrect. i think you'll see this method get called twice when biod > gets restarted, once with the old client and "", and once with "" and the new > client. if biod dies and isn't restarted, you'll see it called as well. calling > the "restarted" method when new_owner is empty is misleading. Done. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.cc:205: dbus::MessageReader array_reader(NULL); On 2017/03/27 18:18:25, Daniel Erat wrote: > use nullptr instead of NULL in c++ code (also below) Done. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... File chromeos/dbus/biod_biometrics_manager_client.h (right): https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:29: // BiodBiometricsManagerClient is used to communicate with the biod dbus On 2017/03/27 18:18:26, Daniel Erat wrote: > nit: D-Bus Done. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:37: virtual void ClientRestarted() {} On 2017/03/25 00:28:20, rkc wrote: > This isn't the "client" that is restarting though, right? We are the client. > biod is the service. > Should this be ServiceRestarted instead? Done. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:37: virtual void ClientRestarted() {} On 2017/03/27 18:18:26, Daniel Erat wrote: > On 2017/03/25 00:28:20, rkc wrote: > > This isn't the "client" that is restarting though, right? We are the client. > > biod is the service. > > Should this be ServiceRestarted instead? > > well, biod and chrome are both d-bus clients... but i agree that this name is > confusing and that the method should be renamed to ServiceRestarted(). > > please also consider prefixing this method (and maybe all of the observer > methods) with "Biod". when a class implements multiple interfaces, it's > difficult to know which one an overridden method named "ServiceRestarted" is > coming from. Done. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:42: virtual void EnrollScanDoneReceived(uint32_t scan_result, On 2017/03/27 18:18:26, Daniel Erat wrote: > at least from this comment, i have no idea what |scan_result| contains. is it an > enum that needs to be typecast? Done. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:45: // Called to indicate a bad scan of any kind, or a succesful scan. If scan On 2017/03/27 18:18:26, Daniel Erat wrote: > nit: successful Done. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_bio... chromeos/dbus/biod_biometrics_manager_client.h:48: virtual void AuthScanDoneReceived(uint32_t scan_result, On 2017/03/27 18:18:26, Daniel Erat wrote: > as above, please document how to use |scan_result| (or better, use a type that > makes it obvious) Done.
lgtm, but please wait for Dan's lgtm also.
On 2017/03/27 23:30:42, rkc wrote: > lgtm, but please wait for Dan's lgtm also. Thanks!
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.cc:143: LOG(ERROR) << "Failed to get response for " you can remove the LOG(ERROR)s in the response-is-null cases. ObjectProxy::CallMethod will already log this for you. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client.h (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.h:30: // interface. biod exposes (or will expose) multiple biometrics managers, each is which has its own d-bus object, right? this class appears to correspond to a single manager, based on its RequestType() method. is there going to be another class that communicates with a central biod d-bus object to get individual managers? https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.h:33: // Interface for observing changes from the biometrics. nit: "from the biometrics manager" https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.h:43: bool is_complete) {} consider renaming |is_complete| to |enroll_session_complete| to make it clearer what it's referring to (assuming that my understanding is correct) https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:37: EXPECT_EQ(expected_object_paths.size(), object_paths.size()); you need ASSERT_EQ or CHECK_EQ here. otherwise you'll segfault if the sizes are different. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:120: dbus::Response* response) { document memory ownership/lifetime when passing raw pointers (here and in EmitSignal). for example, i think that ownership of |response| isn't passed and that the pointed-to response must remain valid until OnCallMethod() is called. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:154: dbus::MessageWriter array_writer(NULL); s/NULL/nullptr/ throughout this file https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:176: dbus::Response* expected_response_; this memory looks like it's currently left uninitialized until the first call to SetExpectedMethodArguments(). please initialize it to nullptr here. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:186: // Maps from powerd signal name to the corresponding callback provided by i see where this code came from. :-P please fix the comment. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:209: EXPECT_EQ(expected_name_, method_call->GetMember()); this is fragile and will create a lot of future work for someone if a method is added later that needs to call multiple d-bus methods in the future. please add expectations for the proxy calls that you expect using EXPECT_CALL instead. you can see examples of doing this in e.g. power_manager_client_unittest.cc. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:219: const std::string& fake_id("fakeId"); uh, const reference? string constants should be: const char kFakeId = "fakeId"; please update the rest of the file. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:268: // Create a fake response with an array fake object paths. The get enrollments nit: "array of"
Patchset #9 (id:220001) has been deleted
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.cc:143: LOG(ERROR) << "Failed to get response for " On 2017/03/28 00:14:34, Daniel Erat wrote: > you can remove the LOG(ERROR)s in the response-is-null cases. > ObjectProxy::CallMethod will already log this for you. Done. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client.h (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.h:30: // interface. On 2017/03/28 00:14:34, Daniel Erat wrote: > biod exposes (or will expose) multiple biometrics managers, each is which has > its own d-bus object, right? this class appears to correspond to a single > manager, based on its RequestType() method. is there going to be another class > that communicates with a central biod d-bus object to get individual managers? Yeah, I realized that after completing this CL and the following ones. I'll have to add that later once I figure out how it is going to work. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.h:33: // Interface for observing changes from the biometrics. On 2017/03/28 00:14:35, Daniel Erat wrote: > nit: "from the biometrics manager" Done. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.h:43: bool is_complete) {} On 2017/03/28 00:14:35, Daniel Erat wrote: > consider renaming |is_complete| to |enroll_session_complete| to make it clearer > what it's referring to (assuming that my understanding is correct) Done. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:37: EXPECT_EQ(expected_object_paths.size(), object_paths.size()); On 2017/03/28 00:14:35, Daniel Erat wrote: > you need ASSERT_EQ or CHECK_EQ here. otherwise you'll segfault if the sizes are > different. Done. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:120: dbus::Response* response) { On 2017/03/28 00:14:35, Daniel Erat wrote: > document memory ownership/lifetime when passing raw pointers (here and in > EmitSignal). for example, i think that ownership of |response| isn't passed and > that the pointed-to response must remain valid until OnCallMethod() is called. I changed this to use smart pointer is that ok? If so, done. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:154: dbus::MessageWriter array_writer(NULL); On 2017/03/28 00:14:35, Daniel Erat wrote: > s/NULL/nullptr/ throughout this file Done. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:176: dbus::Response* expected_response_; On 2017/03/28 00:14:35, Daniel Erat wrote: > this memory looks like it's currently left uninitialized until the first call to > SetExpectedMethodArguments(). please initialize it to nullptr here. Done. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:186: // Maps from powerd signal name to the corresponding callback provided by On 2017/03/28 00:14:35, Daniel Erat wrote: > i see where this code came from. :-P please fix the comment. Oops. Done. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:209: EXPECT_EQ(expected_name_, method_call->GetMember()); On 2017/03/28 00:14:35, Daniel Erat wrote: > this is fragile and will create a lot of future work for someone if a method is > added later that needs to call multiple d-bus methods in the future. > > please add expectations for the proxy calls that you expect using EXPECT_CALL > instead. you can see examples of doing this in e.g. > power_manager_client_unittest.cc. You mean consolidating all of this the d-bus calls testing into this one OnCallMethod (with the SetExpectedStuff). Do you wish for me to add seperate methods for each call i.e OnStartEnrollSessionMethod? https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:219: const std::string& fake_id("fakeId"); On 2017/03/28 00:14:35, Daniel Erat wrote: > uh, const reference? string constants should be: > > const char kFakeId = "fakeId"; > > please update the rest of the file. I've believed I've seen this notation used in many tests (minus the reference). Just skimming the dbus unittests it seems to be half/half. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:268: // Create a fake response with an array fake object paths. The get enrollments On 2017/03/28 00:14:35, Daniel Erat wrote: > nit: "array of" Done.
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client.h (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.h:30: // interface. On 2017/03/28 01:31:05, sammiequon wrote: > On 2017/03/28 00:14:34, Daniel Erat wrote: > > biod exposes (or will expose) multiple biometrics managers, each is which has > > its own d-bus object, right? this class appears to correspond to a single > > manager, based on its RequestType() method. is there going to be another class > > that communicates with a central biod d-bus object to get individual managers? > > Yeah, I realized that after completing this CL and the following ones. I'll have > to add that later once I figure out how it is going to work. i'm a bit hesitant for this to go in before you know how you'll be able to instantiate it. :-) can you figure out that part of the design before proceeding with this? https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:209: EXPECT_EQ(expected_name_, method_call->GetMember()); On 2017/03/28 01:31:05, sammiequon wrote: > On 2017/03/28 00:14:35, Daniel Erat wrote: > > this is fragile and will create a lot of future work for someone if a method > is > > added later that needs to call multiple d-bus methods in the future. > > > > please add expectations for the proxy calls that you expect using EXPECT_CALL > > instead. you can see examples of doing this in e.g. > > power_manager_client_unittest.cc. > > You mean consolidating all of this the d-bus calls testing into this one > OnCallMethod (with the SetExpectedStuff). Do you wish for me to add seperate > methods for each call i.e OnStartEnrollSessionMethod? i mean setting expectations similar to this (from the PowerManagerClient tests): EXPECT_CALL( *proxy_.get(), CallMethod(HasMember(power_manager::kRegisterSuspendDelayMethod), _, _)) .WillRepeatedly( Invoke(this, &PowerManagerClientTest::RegisterSuspendDelay)); that is, let googlemock do the matching. this will scale to calling multiple methods, verifying that a particular method is called exactly once, etc. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:219: const std::string& fake_id("fakeId"); On 2017/03/28 01:31:05, sammiequon wrote: > On 2017/03/28 00:14:35, Daniel Erat wrote: > > uh, const reference? string constants should be: > > > > const char kFakeId = "fakeId"; > > > > please update the rest of the file. > > I've believed I've seen this notation used in many tests (minus the reference). > Just skimming the dbus unittests it seems to be half/half. yes, a const std::string is okay; it's the reference that's strange (since there's no existing variable that the reference points towards, although i believe that the c++ spec says that a temporary is created and persists), along with not using kFakeId naming.
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client.h (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.h:30: // interface. On 2017/03/28 01:44:30, Daniel Erat wrote: > On 2017/03/28 01:31:05, sammiequon wrote: > > On 2017/03/28 00:14:34, Daniel Erat wrote: > > > biod exposes (or will expose) multiple biometrics managers, each is which > has > > > its own d-bus object, right? this class appears to correspond to a single > > > manager, based on its RequestType() method. is there going to be another > class > > > that communicates with a central biod d-bus object to get individual > managers? > > > > Yeah, I realized that after completing this CL and the following ones. I'll > have > > to add that later once I figure out how it is going to work. > > i'm a bit hesitant for this to go in before you know how you'll be able to > instantiate it. :-) can you figure out that part of the design before proceeding > with this? I looked at the biod code and if I read it correctly there is not a way to choose a biometrics manager. It currently runs two managers, "fakefinger", "fpcfinger". So I guess it would be either connect to the "fpcfinger" all the time (ie. either side will change the interface") or there will be a biod manager manager where I can query a list of biometrics manager in which I would have to add a biod manager manager client. I don't think either will be non-trivial. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:209: EXPECT_EQ(expected_name_, method_call->GetMember()); On 2017/03/28 01:44:30, Daniel Erat wrote: > On 2017/03/28 01:31:05, sammiequon wrote: > > On 2017/03/28 00:14:35, Daniel Erat wrote: > > > this is fragile and will create a lot of future work for someone if a method > > is > > > added later that needs to call multiple d-bus methods in the future. > > > > > > please add expectations for the proxy calls that you expect using > EXPECT_CALL > > > instead. you can see examples of doing this in e.g. > > > power_manager_client_unittest.cc. > > > > You mean consolidating all of this the d-bus calls testing into this one > > OnCallMethod (with the SetExpectedStuff). Do you wish for me to add seperate > > methods for each call i.e OnStartEnrollSessionMethod? > > i mean setting expectations similar to this (from the PowerManagerClient tests): > > EXPECT_CALL( > *proxy_.get(), > CallMethod(HasMember(power_manager::kRegisterSuspendDelayMethod), _, _)) > .WillRepeatedly( > Invoke(this, &PowerManagerClientTest::RegisterSuspendDelay)); > > that is, let googlemock do the matching. this will scale to calling multiple > methods, verifying that a particular method is called exactly once, etc. I gave it a shot. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc:219: const std::string& fake_id("fakeId"); On 2017/03/28 01:44:30, Daniel Erat wrote: > On 2017/03/28 01:31:05, sammiequon wrote: > > On 2017/03/28 00:14:35, Daniel Erat wrote: > > > uh, const reference? string constants should be: > > > > > > const char kFakeId = "fakeId"; > > > > > > please update the rest of the file. > > > > I've believed I've seen this notation used in many tests (minus the > reference). > > Just skimming the dbus unittests it seems to be half/half. > > yes, a const std::string is okay; it's the reference that's strange (since > there's no existing variable that the reference points towards, although i > believe that the c++ spec says that a temporary is created and persists), along > with not using kFakeId naming. So this means i should use the kFoo naming instead of the underscore seperated naming here? Because I saw both instances in other tests as well, but I'm not strongly opposed to either.
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.cc:107: biod::kBiodServiceName, dbus::ObjectPath(biod::kBiodServicePath)); i think that this part is wrong. this class is supposed to communicate with an individual BiometricsManager object exported by biod, not with the central biod object. https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client.h (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.h:30: // interface. On 2017/03/28 03:17:54, sammiequon wrote: > On 2017/03/28 01:44:30, Daniel Erat wrote: > > On 2017/03/28 01:31:05, sammiequon wrote: > > > On 2017/03/28 00:14:34, Daniel Erat wrote: > > > > biod exposes (or will expose) multiple biometrics managers, each is which > > has > > > > its own d-bus object, right? this class appears to correspond to a single > > > > manager, based on its RequestType() method. is there going to be another > > class > > > > that communicates with a central biod d-bus object to get individual > > managers? > > > > > > Yeah, I realized that after completing this CL and the following ones. I'll > > have > > > to add that later once I figure out how it is going to work. > > > > i'm a bit hesitant for this to go in before you know how you'll be able to > > instantiate it. :-) can you figure out that part of the design before > proceeding > > with this? > > I looked at the biod code and if I read it correctly there is not a way to > choose a biometrics manager. It currently runs two managers, "fakefinger", > "fpcfinger". So I guess it would be either connect to the "fpcfinger" all the > time (ie. either side will change the interface") or there will be a biod > manager manager where I can query a list of biometrics manager in which I would > have to add a biod manager manager client. I don't think either will be > non-trivial. this is why i suggested not having biod create multiple d-bus objects. :-( i think that the right way to model the current interface would be by introducing a BiodClient class that interfaces with biod::kBiodServiceName/kBiodServicePath to enumerate BiometricsManager objects, and then have it create and return BiodBiometricsManagerClient objects. class BiodClient : public DBusClient { ... // you probably need something like WeakPtr here since these managers can // presumably disappear at any time. the calling code is going to need to // constantly check whether the pointer is still valid, too... std::vector<WeakPtr<BiodBiometricsManagerClient>> GetManagers(); ... private: std::vector<std::unique_ptr<BiodBiometricsManagerClient>> managers_; }; class BiodBiometricsManagerClient { ... biod::BiometricType type() const { return type_; } ... private: // if this can't change for a given manager, just initialize it when // you first see the manager instead of making the caller call RequestType biod::BiometricType type_; ... }; BiodClient's Observer interface should contain BiodServiceRestarted, while BiodBiometricsManagerClient's interface should contain BiodEnrollScanDoneReceived and the rest of the methods.
On 2017/03/29 22:11:58, Daniel Erat wrote: > this is why i suggested not having biod create multiple d-bus objects. :-( > > i think that the right way to model the current interface would be by > introducing a BiodClient class that interfaces with > biod::kBiodServiceName/kBiodServicePath to enumerate BiometricsManager objects, > and then have it create and return BiodBiometricsManagerClient objects. > > class BiodClient : public DBusClient { > ... > // you probably need something like WeakPtr here since these managers can > // presumably disappear at any time. the calling code is going to need to > // constantly check whether the pointer is still valid, too... > std::vector<WeakPtr<BiodBiometricsManagerClient>> GetManagers(); > ... > private: > std::vector<std::unique_ptr<BiodBiometricsManagerClient>> managers_; > }; > > class BiodBiometricsManagerClient { > ... > biod::BiometricType type() const { return type_; } > ... > private: > // if this can't change for a given manager, just initialize it when > // you first see the manager instead of making the caller call RequestType > biod::BiometricType type_; > ... > }; > > BiodClient's Observer interface should contain BiodServiceRestarted, while > BiodBiometricsManagerClient's interface should contain > BiodEnrollScanDoneReceived and the rest of the methods. No, I agree with you on the multiple d-bus objects. I had a similar suggestion in the previous comment expect I called it BiodManagerManagerClient (just as a temp). My only concern is that the biod code does not support querying a list of BiodManagers at the moment, so this BiodClient would not be usable anyways. I feel like it's just as possible the concept of multiple BiodManagers gets dropped and just use the single one as a FingerprintManager.
On 2017/03/29 22:30:45, sammiequon wrote: > On 2017/03/29 22:11:58, Daniel Erat wrote: > > this is why i suggested not having biod create multiple d-bus objects. :-( > > > > i think that the right way to model the current interface would be by > > introducing a BiodClient class that interfaces with > > biod::kBiodServiceName/kBiodServicePath to enumerate BiometricsManager > objects, > > and then have it create and return BiodBiometricsManagerClient objects. > > > > class BiodClient : public DBusClient { > > ... > > // you probably need something like WeakPtr here since these managers can > > // presumably disappear at any time. the calling code is going to need to > > // constantly check whether the pointer is still valid, too... > > std::vector<WeakPtr<BiodBiometricsManagerClient>> GetManagers(); > > ... > > private: > > std::vector<std::unique_ptr<BiodBiometricsManagerClient>> managers_; > > }; > > > > class BiodBiometricsManagerClient { > > ... > > biod::BiometricType type() const { return type_; } > > ... > > private: > > // if this can't change for a given manager, just initialize it when > > // you first see the manager instead of making the caller call RequestType > > biod::BiometricType type_; > > ... > > }; > > > > BiodClient's Observer interface should contain BiodServiceRestarted, while > > BiodBiometricsManagerClient's interface should contain > > BiodEnrollScanDoneReceived and the rest of the methods. > > No, I agree with you on the multiple d-bus objects. I had a similar suggestion > in the previous comment expect I called it BiodManagerManagerClient (just as a > temp). My only concern is that the biod code does not support querying a list of > BiodManagers at the moment, so this BiodClient would not be usable anyways. I > feel like > it's just as possible the concept of multiple BiodManagers gets dropped and just > use the single one as a FingerprintManager. ah. yeah, if there are no plans to add more managers beyond FingerprintManager in the near future, then that would definitely be preferable.
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.cc:107: biod::kBiodServiceName, dbus::ObjectPath(biod::kBiodServicePath)); On 2017/03/29 22:11:57, Daniel Erat wrote: > i think that this part is wrong. this class is supposed to communicate with an > individual BiometricsManager object exported by biod, not with the central biod > object. If we are going with the possible single BiometricsManager instead of the BiodManager is this still incorrect?
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_biometrics_manager_client.cc:107: biod::kBiodServiceName, dbus::ObjectPath(biod::kBiodServicePath)); On 2017/03/30 00:22:27, sammiequon wrote: > On 2017/03/29 22:11:57, Daniel Erat wrote: > > i think that this part is wrong. this class is supposed to communicate with an > > individual BiometricsManager object exported by biod, not with the central > biod > > object. > > If we are going with the possible single BiometricsManager instead of the > BiodManager is this still incorrect? nope, it's correct then. this class should probably also just be renamed to BiodClient in that case -- i don't see any reason to retain the extra "manager" wordiness if there's just one service. i'd recommend figuring out the biod side of the interface before making any more changes here, though -- any simplification you can do on the service side will probably make this chrome code simpler too.
On 2017/03/30 00:25:38, Daniel Erat wrote: > https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... > File chromeos/dbus/biod/biod_biometrics_manager_client.cc (right): > > https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... > chromeos/dbus/biod/biod_biometrics_manager_client.cc:107: > biod::kBiodServiceName, dbus::ObjectPath(biod::kBiodServicePath)); > On 2017/03/30 00:22:27, sammiequon wrote: > > On 2017/03/29 22:11:57, Daniel Erat wrote: > > > i think that this part is wrong. this class is supposed to communicate with > an > > > individual BiometricsManager object exported by biod, not with the central > > biod > > > object. > > > > If we are going with the possible single BiometricsManager instead of the > > BiodManager is this still incorrect? > > nope, it's correct then. > > this class should probably also just be renamed to BiodClient in that case -- i > don't see any reason to retain the extra "manager" wordiness if there's just one > service. > > i'd recommend figuring out the biod side of the interface before making any more > changes here, though -- any simplification you can do on the service side will > probably make this chrome code simpler too. sure, I will as around and then get back to this shortly :-).
On 2017/03/30 18:14:42, sammiequon wrote: > On 2017/03/30 00:25:38, Daniel Erat wrote: > > > https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... > > File chromeos/dbus/biod/biod_biometrics_manager_client.cc (right): > > > > > https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/bio... > > chromeos/dbus/biod/biod_biometrics_manager_client.cc:107: > > biod::kBiodServiceName, dbus::ObjectPath(biod::kBiodServicePath)); > > On 2017/03/30 00:22:27, sammiequon wrote: > > > On 2017/03/29 22:11:57, Daniel Erat wrote: > > > > i think that this part is wrong. this class is supposed to communicate > with > > an > > > > individual BiometricsManager object exported by biod, not with the central > > > biod > > > > object. > > > > > > If we are going with the possible single BiometricsManager instead of the > > > BiodManager is this still incorrect? > > > > nope, it's correct then. > > > > this class should probably also just be renamed to BiodClient in that case -- > i > > don't see any reason to retain the extra "manager" wordiness if there's just > one > > service. > > > > i'd recommend figuring out the biod side of the interface before making any > more > > changes here, though -- any simplification you can do on the service side will > > probably make this chrome code simpler too. > > sure, I will as around and then get back to this shortly :-). derat@ - Can you take another look? Contrary to our chat earlier, we are deciding to go with a single formerly "BiodBiometricsManager", and renamed it to "BiodClient" after discussing with rkc@. Thanks!
https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client.cc (right): https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.cc:139: dbus::ObjectPath result; you could make all of these methods shorter: dbus::ObjectPath result; if (response) { dbus::MessageReader ... if (!reader.PopObjectPath... LOG(ERROR) << ... } } callback.Run(result); https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.cc:218: << "Failed to connect to biometrics signal:" << signal_name; nit: add space after ':' (also in other strings) https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client.h (right): https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:8: #include <stdint.h> are you actually using this? https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:9: #include <string> https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:25: // with the labels of all the matched stored fingerprints is returned. please go into more details about the actual values that are stored. are "the users" account ids (in which case you should probably use the AccountId class) or something else? what's the format for the fingerprints? (it's fine to point at another class if they're documented there.) https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:36: // Called when biometrics manager powers up or is restarted. does this actually get called if biod is already running before chrome starts, or only if it crashes and is restarted later? https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:39: // Called whenever a user attempts a scan. |scan_result| tells whether the Called whenever a user attempts a scan during enrollment. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:45: // Called to indicate a bad scan of any kind, or a successful scan. If scan Called when an authentication scan is performed. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:51: // Called during either session to indicate a failure. Any enrollment that s/either session/an enrollment or authentication session/ https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:69: // one argument which contains a list of the stored records object paths for s/records/records'/ https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:80: virtual void StartEnrollSession(const std::string& user_id, same comment re |user_id|, also document what |label| is https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:84: // Gets all the records registered with this biometric. |callback| is called do you mean "... with this user"? https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:86: virtual void GetRecordsForUser(const std::string& user_id, as above, document what the id actually is and ideally use a non-std::string type as well. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:89: // Irreversibly destroys all records registered with this biometric. should you drop text like "with this biometric" if there's only one manager? https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:37: CHECK_EQ(expected_object_paths.size(), object_paths.size()); ASSERT_EQ? https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:64: int num_enroll_scan_received() const { return num_enroll_scan_received_; } s/scan/scans/ https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:65: int num_auth_scan_received() const { return num_auth_scan_received_; } s/scan/scans/ https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:66: int num_failure_received() const { return num_failure_received_; } s/failure/failures/ https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:138: .WillRepeatedly(Invoke(this, &BiodClientTest::OnCallMethod)); adding a bunch of expectations like this doesn't improve anything. they all invoke the same method, so this code will have the same problem that i described earlier of not being able to e.g. handle multiple method calls. it would be better to add an expectation within each test listing only the method that you expect to be called, and to verify that it is called exactly once. googlemock doesn't integrate at all with base::Bind(), which makes using it in chromium extremely painful. i think your best bet due to the limitations of googlemock may be something like this: std::map<std::string, std::unique_ptr<dbus::Response>> pending_method_calls_; // private member void AddMethodExpectation( const std::string& method_name, std::unique_ptr<dbus::Response> response) { ASSERT_FALSE(pending_method_calls_.count(method_name)); pending_method_calls_[method_name] = std::move(response); EXPECT_CALL( *proxy_.get(), CallMethod(HasMember(method_name), _, _)), .WillOnce(Invoke(this, &BiodClientTest::OnCallMethod)); } void OnCallMethod(...) { // assert that the called method is in the map // remove the response from the map // post the response to the callback } then make tests call AddMethodExpectation as needed. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:196: void EmitScanFailedSignal() { i don't see this getting called https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:243: const std::string fake_id("fakeId"); kFakeId, kFakeLabel, etc. https://google.github.io/styleguide/cppguide.html#Constant_Names https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:256: base::Bind(&CheckObjectPathCallback, fake_object_path)); how about making the callback instead save the path to a passed-in pointer? then you can put the expectation within the test, and also catch the case where the callback doesn't get called at all (which i don't think the current code covers). https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:309: base::RunLoop().RunUntilIdle(); this isn't verifying anything right now, i don't think. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:379: client_->AddObserver(&observer2); this second observer doesn't seem useful. how would the class being tested even be able to call an observer that isn't registered with it? i'd combine all of this into a much-shorter single test that just adds a single observer and verifies that emitting the enroll scan and auth scan done signals once increment its received count as expected.
Patchset #12 (id:300001) has been deleted
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Description was changed from ========== cros: DBUS client to interact with fingerprint DBUS API. Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after. Part 1 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 TEST=chromeos_unittest --gtest_filter="BiodBiometricsManagerClientTest.*" BUG=702675 ========== to ========== cros: DBUS client to interact with fingerprint DBUS API. Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after. Part 1 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 TEST=chromeos_unittest --gtest_filter="BiodClientTest.*" BUG=702675 ==========
https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client.cc (right): https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.cc:139: dbus::ObjectPath result; On 2017/03/31 04:30:09, Daniel Erat wrote: > you could make all of these methods shorter: > > dbus::ObjectPath result; > if (response) { > dbus::MessageReader ... > if (!reader.PopObjectPath... > LOG(ERROR) << ... > } > } > callback.Run(result); Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.cc:218: << "Failed to connect to biometrics signal:" << signal_name; On 2017/03/31 04:30:09, Daniel Erat wrote: > nit: add space after ':' (also in other strings) Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client.h (right): https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:8: #include <stdint.h> On 2017/03/31 04:30:09, Daniel Erat wrote: > are you actually using this? Nope. That was for when the scan results were uint32_t. Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:9: On 2017/03/31 04:30:10, Daniel Erat wrote: > #include <string> Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:25: // with the labels of all the matched stored fingerprints is returned. On 2017/03/31 04:30:10, Daniel Erat wrote: > please go into more details about the actual values that are stored. are "the > users" account ids (in which case you should probably use the AccountId class) > or something else? what's the format for the fingerprints? (it's fine to point > at another class if they're documented there.) Theres another ongoing discussion (not involving me) about what |user_id| should be. But I was under impression the string -> AccountId would be translated at a higher level, or do you think it belongs down here? https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:36: // Called when biometrics manager powers up or is restarted. On 2017/03/31 04:30:10, Daniel Erat wrote: > does this actually get called if biod is already running before chrome starts, > or only if it crashes and is restarted later? Let me quickly test this out and I'll get back to this soon. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:39: // Called whenever a user attempts a scan. |scan_result| tells whether the On 2017/03/31 04:30:10, Daniel Erat wrote: > Called whenever a user attempts a scan during enrollment. Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:45: // Called to indicate a bad scan of any kind, or a successful scan. If scan On 2017/03/31 04:30:09, Daniel Erat wrote: > Called when an authentication scan is performed. Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:51: // Called during either session to indicate a failure. Any enrollment that On 2017/03/31 04:30:10, Daniel Erat wrote: > s/either session/an enrollment or authentication session/ Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:69: // one argument which contains a list of the stored records object paths for On 2017/03/31 04:30:10, Daniel Erat wrote: > s/records/records'/ Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:80: virtual void StartEnrollSession(const std::string& user_id, On 2017/03/31 04:30:10, Daniel Erat wrote: > same comment re |user_id|, also document what |label| is Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:84: // Gets all the records registered with this biometric. |callback| is called On 2017/03/31 04:30:09, Daniel Erat wrote: > do you mean "... with this user"? Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:86: virtual void GetRecordsForUser(const std::string& user_id, On 2017/03/31 04:30:10, Daniel Erat wrote: > as above, document what the id actually is and ideally use a non-std::string > type as well. Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:89: // Irreversibly destroys all records registered with this biometric. On 2017/03/31 04:30:09, Daniel Erat wrote: > should you drop text like "with this biometric" if there's only one manager? Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/03/31 04:30:10, Daniel Erat wrote: > 2017 Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:37: CHECK_EQ(expected_object_paths.size(), object_paths.size()); On 2017/03/31 04:30:10, Daniel Erat wrote: > ASSERT_EQ? Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:64: int num_enroll_scan_received() const { return num_enroll_scan_received_; } On 2017/03/31 04:30:10, Daniel Erat wrote: > s/scan/scans/ Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:65: int num_auth_scan_received() const { return num_auth_scan_received_; } On 2017/03/31 04:30:10, Daniel Erat wrote: > s/scan/scans/ Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:66: int num_failure_received() const { return num_failure_received_; } On 2017/03/31 04:30:11, Daniel Erat wrote: > s/failure/failures/ Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:138: .WillRepeatedly(Invoke(this, &BiodClientTest::OnCallMethod)); On 2017/03/31 04:30:10, Daniel Erat wrote: > adding a bunch of expectations like this doesn't improve anything. they all > invoke the same method, so this code will have the same problem that i described > earlier of not being able to e.g. handle multiple method calls. > > it would be better to add an expectation within each test listing only the > method that you expect to be called, and to verify that it is called exactly > once. > > googlemock doesn't integrate at all with base::Bind(), which makes using it in > chromium extremely painful. > > i think your best bet due to the limitations of googlemock may be something like > this: > > std::map<std::string, std::unique_ptr<dbus::Response>> pending_method_calls_; > // private member > > void AddMethodExpectation( > const std::string& method_name, > std::unique_ptr<dbus::Response> response) { > ASSERT_FALSE(pending_method_calls_.count(method_name)); > pending_method_calls_[method_name] = std::move(response); > EXPECT_CALL( > *proxy_.get(), > CallMethod(HasMember(method_name), _, _)), > .WillOnce(Invoke(this, &BiodClientTest::OnCallMethod)); > } > > void OnCallMethod(...) { > // assert that the called method is in the map > // remove the response from the map > // post the response to the callback > } > > then make tests call AddMethodExpectation as needed. Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:196: void EmitScanFailedSignal() { On 2017/03/31 04:30:11, Daniel Erat wrote: > i don't see this getting called Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:243: const std::string fake_id("fakeId"); On 2017/03/31 04:30:11, Daniel Erat wrote: > kFakeId, kFakeLabel, etc. > > https://google.github.io/styleguide/cppguide.html#Constant_Names Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:256: base::Bind(&CheckObjectPathCallback, fake_object_path)); On 2017/03/31 04:30:11, Daniel Erat wrote: > how about making the callback instead save the path to a passed-in pointer? then > you can put the expectation within the test, and also catch the case where the > callback doesn't get called at all (which i don't think the current code > covers). I tried using a unique ptr but I couldn't bind as reference or bind like &(callbackresult.get()) so I went with the normal pointer. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:309: base::RunLoop().RunUntilIdle(); On 2017/03/31 04:30:10, Daniel Erat wrote: > this isn't verifying anything right now, i don't think. Done. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:379: client_->AddObserver(&observer2); On 2017/03/31 04:30:11, Daniel Erat wrote: > this second observer doesn't seem useful. how would the class being tested even > be able to call an observer that isn't registered with it? > > i'd combine all of this into a much-shorter single test that just adds a single > observer and verifies that emitting the enroll scan and auth scan done signals > once increment its received count as expected. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client.h (right): https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:25: // with the labels of all the matched stored fingerprints is returned. On 2017/03/31 22:52:12, sammiequon wrote: > On 2017/03/31 04:30:10, Daniel Erat wrote: > > please go into more details about the actual values that are stored. are "the > > users" account ids (in which case you should probably use the AccountId class) > > or something else? what's the format for the fingerprints? (it's fine to point > > at another class if they're documented there.) > > Theres another ongoing discussion (not involving me) about what |user_id| should > be. But I was under impression the string -> AccountId would be translated at a > higher level, or do you think it belongs down here? i think it's okay to document it as being assigned by chrome here. if it ends up being a specific type that //chromeos/dbus is allowed to depend on (note that chromeos/DEPS permits components/signin/core/account_id/account_id.h), it'd probably be clearer to other developers to use the actual type. https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client.cc (right): https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.cc:144: << " had incorrect response."; i was going to say that i think it's more common in chrome to leave trailing periods off the end of log messages, but my (possibly flawed) queries suggest that almost a third of single-line messages actually have periods at the end: % git grep '^\s*LOG(.*\.";$' | wc -l 1992 % git grep '^\s*LOG(.*;$' | wc -l 6268 so, it's fine to leave the periods at the ends of these. :-) https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client.h (right): https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:25: // are unique identifiers which created and parsed by Chromium. The labels nit: "... which are assigned by Chrome. ..." see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... for guidance on using Chrome vs. Chromium. https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:26: // represent the name the user gave their biometric. nit: s/name/names/ https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:30: const dbus::ObjectPath& object_path) { you're leaking memory in tests now, i think. just do this: void CopyObjectPath(dbus::ObjectPath* dest_path, const dbus::ObjectPath& src_path) { CHECK(dest_path); *dest_path = src_path; } and then in tests: dbus::ObjectPath returned_path("invalid"); // pass base::Bind(&CopyObjectPath, &returned_path) EXPECT_EQ(kFakeObjectPath, returned_path); then you don't have any new-allocated memory that can be leaked. i think i'd recommend initializing it to "invalid" or something similar just so you can ensure that CopyObjectPath was actually called -- otherwise, you don't have any way to distinguish between not being called or being called with an empty path. https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:36: std::vector<dbus::ObjectPath>** expected_object_paths_out, same comment applies here. pass a std::vector<dbus::ObjectPath>* to this method and then change the body to just be: CHECK(dest_object_paths); *dest_object_paths = src_object_paths; https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:257: callback_result = nullptr; here's one of the places where memory is being leaked, for example. you'd need to call delete in each of these places instead of just at the end of the test, but see the earlier comment about how to avoid new/delete entirely. https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:262: ASSERT_TRUE(callback_result); after getting rid of the new calls, you won't need these null checks either https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:362: TEST_F(BiodClientTest, TestEnrollScanDoneObserver) { nit: TestNotifyObserverAboutScan?
The CQ bit was unchecked by commit-bot@chromium.org
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...)
https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client.h (right): https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:25: // with the labels of all the matched stored fingerprints is returned. On 2017/04/01 00:46:26, Daniel Erat wrote: > On 2017/03/31 22:52:12, sammiequon wrote: > > On 2017/03/31 04:30:10, Daniel Erat wrote: > > > please go into more details about the actual values that are stored. are > "the > > > users" account ids (in which case you should probably use the AccountId > class) > > > or something else? what's the format for the fingerprints? (it's fine to > point > > > at another class if they're documented there.) > > > > Theres another ongoing discussion (not involving me) about what |user_id| > should > > be. But I was under impression the string -> AccountId would be translated at > a > > higher level, or do you think it belongs down here? > > i think it's okay to document it as being assigned by chrome here. if it ends up > being a specific type that //chromeos/dbus is allowed to depend on (note that > chromeos/DEPS permits components/signin/core/account_id/account_id.h), it'd > probably be clearer to other developers to use the actual type. Acknowledged. https://codereview.chromium.org/2567813002/diff/280001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:36: // Called when biometrics manager powers up or is restarted. On 2017/03/31 22:52:13, sammiequon wrote: > On 2017/03/31 04:30:10, Daniel Erat wrote: > > does this actually get called if biod is already running before chrome starts, > > or only if it crashes and is restarted later? > > Let me quickly test this out and I'll get back to this soon. Yeah it gets called when biod is already running and chrome starts. https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client.cc (right): https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.cc:144: << " had incorrect response."; On 2017/04/01 00:46:26, Daniel Erat wrote: > i was going to say that i think it's more common in chrome to leave trailing > periods off the end of log messages, but my (possibly flawed) queries suggest > that almost a third of single-line messages actually have periods at the end: > > % git grep '^\s*LOG(.*\.";$' | wc -l > 1992 > % git grep '^\s*LOG(.*;$' | wc -l > 6268 > > so, it's fine to leave the periods at the ends of these. :-) :-) https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client.h (right): https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:25: // are unique identifiers which created and parsed by Chromium. The labels On 2017/04/01 00:46:26, Daniel Erat wrote: > nit: "... which are assigned by Chrome. ..." > > see > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > for guidance on using Chrome vs. Chromium. Done. https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client.h:26: // represent the name the user gave their biometric. On 2017/04/01 00:46:26, Daniel Erat wrote: > nit: s/name/names/ Done. https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:30: const dbus::ObjectPath& object_path) { On 2017/04/01 00:46:27, Daniel Erat wrote: > you're leaking memory in tests now, i think. > > just do this: > > void CopyObjectPath(dbus::ObjectPath* dest_path, > const dbus::ObjectPath& src_path) { > CHECK(dest_path); > *dest_path = src_path; > } > > and then in tests: > > dbus::ObjectPath returned_path("invalid"); > // pass base::Bind(&CopyObjectPath, &returned_path) > EXPECT_EQ(kFakeObjectPath, returned_path); > > then you don't have any new-allocated memory that can be leaked. > > i think i'd recommend initializing it to "invalid" or something similar just so > you can ensure that CopyObjectPath was actually called -- otherwise, you don't > have any way to distinguish between not being called or being called with an > empty path. Done. Yeah i thought of this but for some reason I thought using nullptr was the way to go. https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:36: std::vector<dbus::ObjectPath>** expected_object_paths_out, On 2017/04/01 00:46:27, Daniel Erat wrote: > same comment applies here. pass a std::vector<dbus::ObjectPath>* to this method > and then change the body to just be: > > CHECK(dest_object_paths); > *dest_object_paths = src_object_paths; Done. https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:257: callback_result = nullptr; On 2017/04/01 00:46:27, Daniel Erat wrote: > here's one of the places where memory is being leaked, for example. you'd need > to call delete in each of these places instead of just at the end of the test, > but see the earlier comment about how to avoid new/delete entirely. Oops. Yeah. https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:262: ASSERT_TRUE(callback_result); On 2017/04/01 00:46:27, Daniel Erat wrote: > after getting rid of the new calls, you won't need these null checks either Done. https://codereview.chromium.org/2567813002/diff/320001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:362: TEST_F(BiodClientTest, TestEnrollScanDoneObserver) { On 2017/04/01 00:46:27, Daniel Erat wrote: > nit: TestNotifyObserverAboutScan? Done. I opted to skip the AboutScan since I put the failures testing in here too.
thanks, lgtm with a few comments https://codereview.chromium.org/2567813002/diff/340001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/340001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:29: const dbus::ObjectPath kInvalidTestPath = we don't allow class-type static members: https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables just make this be const char kInvalidPath[] instead. https://codereview.chromium.org/2567813002/diff/340001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:246: // Initialize to a invalid path to differentiate if the callback was missed or nit: move this comment above the kInvalidTestPath constant so you don't need to repeat it in all of these tests, e.g.: // Value used to initialize dbus::ObjectPath objects in // tests to make it easier to determine when empty values // have been assigned.
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...
Patchset #14 (id:360001) has been deleted
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks Dan! https://codereview.chromium.org/2567813002/diff/340001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/340001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:29: const dbus::ObjectPath kInvalidTestPath = On 2017/04/01 01:29:24, Daniel Erat wrote: > we don't allow class-type static members: > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > just make this be const char kInvalidPath[] instead. Done. https://codereview.chromium.org/2567813002/diff/340001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:246: // Initialize to a invalid path to differentiate if the callback was missed or On 2017/04/01 01:29:24, Daniel Erat wrote: > nit: move this comment above the kInvalidTestPath constant so you don't need to > repeat it in all of these tests, e.g.: > > // Value used to initialize dbus::ObjectPath objects in > // tests to make it easier to determine when empty values > // have been assigned. Done.
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm https://codereview.chromium.org/2567813002/diff/380001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/380001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:31: const char* kInvalidTestPath = "/invalid/test/path"; one more nit: use an array instead: const char kInvalidTestPath[] = ...
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...
Thanks! https://codereview.chromium.org/2567813002/diff/380001/chromeos/dbus/biod/bio... File chromeos/dbus/biod/biod_client_unittest.cc (right): https://codereview.chromium.org/2567813002/diff/380001/chromeos/dbus/biod/bio... chromeos/dbus/biod/biod_client_unittest.cc:31: const char* kInvalidTestPath = "/invalid/test/path"; On 2017/04/03 13:52:34, Daniel Erat wrote: > one more nit: use an array instead: > > const char kInvalidTestPath[] = ... Done.
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
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, rkc@chromium.org, derat@chromium.org Link to the patchset: https://codereview.chromium.org/2567813002/#ps400001 (title: "Fixed patch set 14 errors.")
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": 400001, "attempt_start_ts": 1491246299322460, "parent_rev": "0672582d53d0b7f561d129e28a30d5ae5cf28605", "commit_rev": "0958b7f56e08e90312288a938205dc33fd69d56b"}
Message was sent while issue was closed.
Description was changed from ========== cros: DBUS client to interact with fingerprint DBUS API. Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after. Part 1 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 TEST=chromeos_unittest --gtest_filter="BiodClientTest.*" BUG=702675 ========== to ========== cros: DBUS client to interact with fingerprint DBUS API. Skeleton of DBUS client to interact with fingerprint DBUS API. Fake client and rest of methods/properties to come shortly after. Part 1 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 TEST=chromeos_unittest --gtest_filter="BiodClientTest.*" BUG=702675 Review-Url: https://codereview.chromium.org/2567813002 Cr-Commit-Position: refs/heads/master@{#461500} Committed: https://chromium.googlesource.com/chromium/src/+/0958b7f56e08e90312288a938205... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:400001) as https://chromium.googlesource.com/chromium/src/+/0958b7f56e08e90312288a938205... |