Chromium Code Reviews
Help | Chromium Project | Sign in
(2)

Issue 2567813002: cros: DBUS client to interact with fingerprint DBUS API.

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 months, 2 weeks ago by sammiequon
Modified:
4 hours, 24 minutes ago
CC:
chromium-reviews, hashimoto+watch_chromium.org, oshima+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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

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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+819 lines, -0 lines) Patch
M chromeos/BUILD.gn View 1 2 3 4 5 6 7 2 chunks +3 lines, -0 lines 0 comments Download
A chromeos/dbus/biod/biod_biometrics_manager_client.h View 1 2 3 4 5 6 7 8 1 chunk +115 lines, -0 lines 0 comments Download
A chromeos/dbus/biod/biod_biometrics_manager_client.cc View 1 2 3 4 5 6 7 8 1 chunk +306 lines, -0 lines 0 comments Download
A chromeos/dbus/biod/biod_biometrics_manager_client_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +395 lines, -0 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 48 (22 generated)
sammiequon
On 2016/12/13 01:40:01, sammiequon wrote: > mailto:sammiequon@chromium.org changed reviewers: > + mailto:steel@chromium.org steel@ - Please ...
3 months, 2 weeks ago (2016-12-13 01:40:17 UTC) #4
sammiequon
On 2016/12/13 01:40:17, sammiequon wrote: > On 2016/12/13 01:40:01, sammiequon wrote: > > mailto:sammiequon@chromium.org changed ...
2 weeks, 1 day ago (2017-03-14 19:06:01 UTC) #9
rkc
lgtm % comments https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_biometrics_manager_client.cc File chromeos/dbus/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_biometrics_manager_client.cc#newcode274 chromeos/dbus/biod_biometrics_manager_client.cc:274: DBusClientImplementationType type) { Unused? https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_biometrics_manager_client_unittest.cc File ...
1 week, 2 days ago (2017-03-20 23:24:34 UTC) #20
sammiequon
Thanks! https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_biometrics_manager_client.cc File chromeos/dbus/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/120001/chromeos/dbus/biod_biometrics_manager_client.cc#newcode274 chromeos/dbus/biod_biometrics_manager_client.cc:274: DBusClientImplementationType type) { On 2017/03/20 23:24:34, rkc wrote: ...
1 week, 2 days ago (2017-03-21 00:13:36 UTC) #22
sammiequon
stevenjb@ - Please take a look. Thanks!
1 week, 2 days ago (2017-03-21 01:11:09 UTC) #24
stevenjb
https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_biometrics_manager_client.cc File chromeos/dbus/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_biometrics_manager_client.cc#newcode23 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_biometrics_manager_client.cc#newcode195 chromeos/dbus/biod_biometrics_manager_client.cc:195: void ...
1 week, 1 day ago (2017-03-21 18:46:17 UTC) #25
sammiequon
https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_biometrics_manager_client.cc File chromeos/dbus/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/140001/chromeos/dbus/biod_biometrics_manager_client.cc#newcode23 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: ...
1 week, 1 day ago (2017-03-21 20:50:19 UTC) #27
stevenjb
lgtm
1 week, 1 day ago (2017-03-21 21:12:01 UTC) #28
sammiequon
On 2017/03/21 21:12:01, stevenjb wrote: > lgtm Thanks! Changed some naming to match the changes ...
1 week, 1 day ago (2017-03-21 23:26:06 UTC) #29
rkc
mostly lg, just a couple of comments. https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_biometrics_manager_client.cc File chromeos/dbus/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/180001/chromeos/dbus/biod_biometrics_manager_client.cc#newcode183 chromeos/dbus/biod_biometrics_manager_client.cc:183: void NameOwnerChangedReceived(const ...
5 days, 4 hours ago (2017-03-25 00:28:20 UTC) #30
rkc
Can we move this to a //chromeos/dbus/biod directory? It is probably time that we start ...
5 days, 3 hours ago (2017-03-25 00:56:04 UTC) #31
stevenjb
No reason not to create sub directories under chromeos/dbus that I can think of. derat@, ...
2 days, 12 hours ago (2017-03-27 16:45:11 UTC) #33
Daniel Erat
i didn't look at the test code but have comments. do you plan to add ...
2 days, 10 hours ago (2017-03-27 18:18:26 UTC) #34
rkc
I specifically want to move everything that I makes sense into sub-directories. I can take ...
2 days, 7 hours ago (2017-03-27 21:37:14 UTC) #35
sammiequon
I moved the files to a biod directory since there will a couple more joining ...
2 days, 5 hours ago (2017-03-27 23:21:49 UTC) #36
rkc
lgtm, but please wait for Dan's lgtm also.
2 days, 5 hours ago (2017-03-27 23:30:42 UTC) #37
sammiequon
On 2017/03/27 23:30:42, rkc wrote: > lgtm, but please wait for Dan's lgtm also. Thanks!
2 days, 5 hours ago (2017-03-27 23:33:59 UTC) #38
Daniel Erat
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/biod_biometrics_manager_client.cc File chromeos/dbus/biod/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/biod_biometrics_manager_client.cc#newcode143 chromeos/dbus/biod/biod_biometrics_manager_client.cc:143: LOG(ERROR) << "Failed to get response for " you ...
2 days, 4 hours ago (2017-03-28 00:14:35 UTC) #39
sammiequon
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/biod_biometrics_manager_client.cc File chromeos/dbus/biod/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/biod_biometrics_manager_client.cc#newcode143 chromeos/dbus/biod/biod_biometrics_manager_client.cc:143: LOG(ERROR) << "Failed to get response for " On ...
2 days, 3 hours ago (2017-03-28 01:31:06 UTC) #41
Daniel Erat
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/biod_biometrics_manager_client.h File chromeos/dbus/biod/biod_biometrics_manager_client.h (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/biod_biometrics_manager_client.h#newcode30 chromeos/dbus/biod/biod_biometrics_manager_client.h:30: // interface. On 2017/03/28 01:31:05, sammiequon wrote: > On ...
2 days, 3 hours ago (2017-03-28 01:44:30 UTC) #42
sammiequon
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/biod_biometrics_manager_client.h File chromeos/dbus/biod/biod_biometrics_manager_client.h (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/biod_biometrics_manager_client.h#newcode30 chromeos/dbus/biod/biod_biometrics_manager_client.h:30: // interface. On 2017/03/28 01:44:30, Daniel Erat wrote: > ...
2 days, 1 hour ago (2017-03-28 03:17:54 UTC) #43
Daniel Erat
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/biod_biometrics_manager_client.cc File chromeos/dbus/biod/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/biod_biometrics_manager_client.cc#newcode107 chromeos/dbus/biod/biod_biometrics_manager_client.cc:107: biod::kBiodServiceName, dbus::ObjectPath(biod::kBiodServicePath)); i think that this part is wrong. ...
6 hours, 37 minutes ago (2017-03-29 22:11:58 UTC) #44
sammiequon
On 2017/03/29 22:11:58, Daniel Erat wrote: > this is why i suggested not having biod ...
6 hours, 18 minutes ago (2017-03-29 22:30:45 UTC) #45
Daniel Erat
On 2017/03/29 22:30:45, sammiequon wrote: > On 2017/03/29 22:11:58, Daniel Erat wrote: > > this ...
6 hours, 13 minutes ago (2017-03-29 22:36:04 UTC) #46
sammiequon
https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/biod_biometrics_manager_client.cc File chromeos/dbus/biod/biod_biometrics_manager_client.cc (right): https://codereview.chromium.org/2567813002/diff/200001/chromeos/dbus/biod/biod_biometrics_manager_client.cc#newcode107 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: > ...
4 hours, 27 minutes ago (2017-03-30 00:22:27 UTC) #47
Daniel Erat
4 hours, 24 minutes ago (2017-03-30 00:25:38 UTC) #48
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46