Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2578323004:
cros: Fake implementation of DBUS biometrics client. (Closed)
|
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: Fake implementation of DBUS biometrics client.
Fake implementation of biometrics clients. Mostly stubs, functionality added in a later CL.
Part 3 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=none
BUG=702675
Review-Url: https://codereview.chromium.org/2578323004
Cr-Commit-Position: refs/heads/master@{#462253}
Committed: https://chromium.googlesource.com/chromium/src/+/86bf7840e543a6ec7959b0b1639a17e3a0d9fc9b
Patch Set 1 #Patch Set 2 : Rebased to master. #
Total comments: 2
Patch Set 3 : Removed weak ptr factorys in fakes. #Patch Set 4 : Rebased. #Patch Set 5 : Rebased. #
Total comments: 4
Patch Set 6 : Fixed patch set 5 errors. #
Total comments: 4
Patch Set 7 : Fixed patch set 6 errors. #
Total comments: 4
Patch Set 8 : Fixed patch set 7 errors. #Patch Set 9 : Oopsies. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 40 (21 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== cros: Fake implementation of DBUS biometrics client. Fake implementation of main biometrics DBUS functions. Missing the other interfaces for now. TEST=chromeos_unittest --gtest_filter="FakeBiometricsClientTest.*" BUG=672269 ========== to ========== cros: Fake implementation of DBUS biometrics client. Fake implementation of biometrics clients. Mostly stubs, functionality added in a later CL. TEST=none BUG=702675 ==========
sammiequon@chromium.org changed reviewers: + steel@chromium.org
steel@ - Please take a look. Thanks!
rkc@chromium.org changed reviewers: + rkc@chromium.org
https://codereview.chromium.org/2578323004/diff/60001/chromeos/dbus/fake_biod... File chromeos/dbus/fake_biod_enroll_session_client.h (right): https://codereview.chromium.org/2578323004/diff/60001/chromeos/dbus/fake_biod... chromeos/dbus/fake_biod_enroll_session_client.h:28: base::WeakPtrFactory<FakeBiodEnrollSessionClient> weak_ptr_factory_; Can we instead have this a protected member of BiodEnrollSessionClient? Both the implementation and fake can use it then. Same with the other fake/impl classes.
Description was changed from ========== cros: Fake implementation of DBUS biometrics client. Fake implementation of biometrics clients. Mostly stubs, functionality added in a later CL. TEST=none BUG=702675 ========== to ========== cros: Fake implementation of DBUS biometrics client. Fake implementation of biometrics clients. Mostly stubs, functionality added in a later CL. Part 3 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=none BUG=702675 ==========
https://codereview.chromium.org/2578323004/diff/60001/chromeos/dbus/fake_biod... File chromeos/dbus/fake_biod_enroll_session_client.h (right): https://codereview.chromium.org/2578323004/diff/60001/chromeos/dbus/fake_biod... chromeos/dbus/fake_biod_enroll_session_client.h:28: base::WeakPtrFactory<FakeBiodEnrollSessionClient> weak_ptr_factory_; On 2017/03/21 00:01:32, rkc wrote: > Can we instead have this a protected member of BiodEnrollSessionClient? Both the > implementation and fake can use it then. > > Same with the other fake/impl classes. Removed this. Seems like it won't be needed.
lgtm provided comments on previous patches are addressed and merged.
Patchset #5 (id:120001) has been deleted
sammiequon@chromium.org changed reviewers: + derat@chromium.org
On 2017/03/25 00:51:23, rkc wrote: > lgtm provided comments on previous patches are addressed and merged. Thanks! derat@ - Please take a look. Thanks!
(i'm in promo committee tomorrow and probably won't get to this until wednesday)
On 2017/04/04 00:20:23, Daniel Erat wrote: > (i'm in promo committee tomorrow and probably won't get to this until wednesday) Ok, no problem :)
sorry, i was wrong when i said i wouldn't have time to take a look :-) https://codereview.chromium.org/2578323004/diff/140001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2578323004/diff/140001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:14: class ObjectPath; nit: also forward-declare Bus https://codereview.chromium.org/2578323004/diff/140001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:48: void OnEnrollScanDoneReceived(biod::ScanResult type_result, bool is_complete); what are these for? they're private and don't look like they're called anywhere. if you're intending to call them from tests to notify observers, they should be public instead (and placed above the BiodClient overrides). i'd probably also rename them to e.g. NotifyAuthScanDoneReceived, NotifySessionFailedReceived, etc. -- "On" usually implies that something is a callback.
https://codereview.chromium.org/2578323004/diff/140001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2578323004/diff/140001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:14: class ObjectPath; On 2017/04/04 02:10:57, Daniel Erat wrote: > nit: also forward-declare Bus Done. https://codereview.chromium.org/2578323004/diff/140001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:48: void OnEnrollScanDoneReceived(biod::ScanResult type_result, bool is_complete); On 2017/04/04 02:10:57, Daniel Erat wrote: > what are these for? they're private and don't look like they're called anywhere. > if you're intending to call them from tests to notify observers, they should be > public instead (and placed above the BiodClient overrides). i'd probably also > rename them to e.g. NotifyAuthScanDoneReceived, NotifySessionFailedReceived, > etc. -- "On" usually implies that something is a callback. These are supposed to be callbacks for a fake implementation of a fingerprint (in part 5). But I think that seperate class can be removed, now that there is only one fake class as apposed to before, so Ill go with your suggestion.
lgtm with a nit and a question https://codereview.chromium.org/2578323004/diff/160001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.cc (right): https://codereview.chromium.org/2578323004/diff/160001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:32: const ObjectPathCallback& callback) {} in the future, you may need to post all of these callbacks to the message loop, right? https://codereview.chromium.org/2578323004/diff/160001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2578323004/diff/160001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:26: void NotifyEnrollScanDoneReceived(biod::ScanResult type_result, nit: add a comment above these like: // Notifies |observers_| about various events.
Thanks! https://codereview.chromium.org/2578323004/diff/160001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.cc (right): https://codereview.chromium.org/2578323004/diff/160001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:32: const ObjectPathCallback& callback) {} On 2017/04/04 05:53:34, Daniel Erat wrote: > in the future, you may need to post all of these callbacks to the message loop, > right? Yeah, these are done in part 5 (though a bit of modifications will need to be made since we went from multiple clients to one). https://codereview.chromium.org/2578323004/diff/160001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2578323004/diff/160001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:26: void NotifyEnrollScanDoneReceived(biod::ScanResult type_result, On 2017/04/04 05:53:34, Daniel Erat wrote: > nit: add a comment above these like: > > // Notifies |observers_| about various events. Done.
lgtm
sammiequon@chromium.org changed reviewers: + stevenjb@chromium.org
On 2017/04/04 16:31:05, Daniel Erat wrote: > lgtm Thanks! stevenjb@ - Please take a look. Thanks!
https://codereview.chromium.org/2578323004/diff/180001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.cc (right): https://codereview.chromium.org/2578323004/diff/180001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:32: const ObjectPathCallback& callback) {} Typically the fake should invoke the callback asynchronously to mimic the real behavior, e.g. base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback); (for all of these callbacks) https://codereview.chromium.org/2578323004/diff/180001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2578323004/diff/180001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:31: void NotifySessionFailedReceived(); Could you add a comment about where / how these are going to be used? I don't see them getting called in the followup CLs, and since they don't override anything it is unclear where / how they ever would be called.
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...
https://codereview.chromium.org/2578323004/diff/180001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.cc (right): https://codereview.chromium.org/2578323004/diff/180001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.cc:32: const ObjectPathCallback& callback) {} On 2017/04/04 17:07:31, stevenjb wrote: > Typically the fake should invoke the callback asynchronously to mimic the real > behavior, e.g. > > base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback); > > (for all of these callbacks) > Done. https://codereview.chromium.org/2578323004/diff/180001/chromeos/dbus/biod/fak... File chromeos/dbus/biod/fake_biod_client.h (right): https://codereview.chromium.org/2578323004/diff/180001/chromeos/dbus/biod/fak... chromeos/dbus/biod/fake_biod_client.h:31: void NotifySessionFailedReceived(); On 2017/04/04 17:07:31, stevenjb wrote: > Could you add a comment about where / how these are going to be used? I don't > see them getting called in the followup CLs, and since they don't override > anything it is unclear where / how they ever would be called. I changed it a bit to better match how it is planned on being used.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/04/04 18:35:54, stevenjb wrote: > lgtm Thanks!
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org, derat@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2578323004/#ps220001 (title: "Oopsies.")
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": 220001, "attempt_start_ts": 1491427833725830,
"parent_rev": "6eb8350b9a05a09b216f684155b7423bd4716084", "commit_rev":
"86bf7840e543a6ec7959b0b1639a17e3a0d9fc9b"}
Message was sent while issue was closed.
Description was changed from ========== cros: Fake implementation of DBUS biometrics client. Fake implementation of biometrics clients. Mostly stubs, functionality added in a later CL. Part 3 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=none BUG=702675 ========== to ========== cros: Fake implementation of DBUS biometrics client. Fake implementation of biometrics clients. Mostly stubs, functionality added in a later CL. Part 3 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=none BUG=702675 Review-Url: https://codereview.chromium.org/2578323004 Cr-Commit-Position: refs/heads/master@{#462253} Committed: https://chromium.googlesource.com/chromium/src/+/86bf7840e543a6ec7959b0b1639a... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:220001) as https://chromium.googlesource.com/chromium/src/+/86bf7840e543a6ec7959b0b1639a... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
