Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(101)

Issue 2799043007: CrOS: Add success/failure indicators for biod methods with no callback. (Closed)

Created:
3 years, 8 months ago by sammiequon
Modified:
3 years, 8 months ago
Reviewers:
Daniel Erat
CC:
chromium-reviews, oshima+watch_chromium.org, hashimoto+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

CrOS: Add success/failure indicators for biod methods with no callback. A couple methods in biod_client.h do not have any indicators of success or failure. Modify these functions to uses VoidDBusMethodCallbaks so users have some indication of D-Bus failures. TEST=chromeos_unittests --gtest_filter="BiodClientTest.*" BUG=700933 Review-Url: https://codereview.chromium.org/2799043007 Cr-Commit-Position: refs/heads/master@{#463385} Committed: https://chromium.googlesource.com/chromium/src/+/63e20eb86328f72e4c6f2707d49d66a55f9ee12e

Patch Set 1 #

Total comments: 26

Patch Set 2 : Fixed patch set 1 errors. #

Total comments: 22

Patch Set 3 : Fixed patch set 2 errors. #

Total comments: 6

Patch Set 4 : Empty -> null. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+171 lines, -66 lines) Patch
M chromeos/dbus/biod/biod_client.h View 1 2 2 chunks +19 lines, -11 lines 0 comments Download
M chromeos/dbus/biod/biod_client.cc View 1 2 6 chunks +26 lines, -13 lines 0 comments Download
M chromeos/dbus/biod/biod_client_unittest.cc View 1 2 3 8 chunks +93 lines, -31 lines 0 comments Download
M chromeos/dbus/biod/fake_biod_client.h View 1 chunk +9 lines, -6 lines 0 comments Download
M chromeos/dbus/biod/fake_biod_client.cc View 2 chunks +24 lines, -5 lines 0 comments Download

Messages

Total messages: 28 (15 generated)
sammiequon
derat@ - Could you take a look? Thanks!
3 years, 8 months ago (2017-04-07 17:44:59 UTC) #3
Daniel Erat
https://codereview.chromium.org/2799043007/diff/20001/chromeos/dbus/biod/biod_client.cc File chromeos/dbus/biod/biod_client.cc (right): https://codereview.chromium.org/2799043007/diff/20001/chromeos/dbus/biod/biod_client.cc#newcode74 chromeos/dbus/biod/biod_client.cc:74: biod_proxy_->CallMethodWithErrorCallback( instead of using CallMethodWithErrorCallback, i think you can ...
3 years, 8 months ago (2017-04-08 00:12:35 UTC) #4
sammiequon
https://codereview.chromium.org/2799043007/diff/20001/chromeos/dbus/biod/biod_client.cc File chromeos/dbus/biod/biod_client.cc (right): https://codereview.chromium.org/2799043007/diff/20001/chromeos/dbus/biod/biod_client.cc#newcode74 chromeos/dbus/biod/biod_client.cc:74: biod_proxy_->CallMethodWithErrorCallback( On 2017/04/08 00:12:34, Daniel Erat wrote: > instead ...
3 years, 8 months ago (2017-04-08 01:03:30 UTC) #5
Daniel Erat
https://codereview.chromium.org/2799043007/diff/20001/chromeos/dbus/biod/biod_client.cc File chromeos/dbus/biod/biod_client.cc (right): https://codereview.chromium.org/2799043007/diff/20001/chromeos/dbus/biod/biod_client.cc#newcode74 chromeos/dbus/biod/biod_client.cc:74: biod_proxy_->CallMethodWithErrorCallback( On 2017/04/08 01:03:30, sammiequon wrote: > On 2017/04/08 ...
3 years, 8 months ago (2017-04-08 01:06:03 UTC) #6
Daniel Erat
https://codereview.chromium.org/2799043007/diff/40001/chromeos/dbus/biod/biod_client.cc File chromeos/dbus/biod/biod_client.cc (left): https://codereview.chromium.org/2799043007/diff/40001/chromeos/dbus/biod/biod_client.cc#oldcode261 chromeos/dbus/biod/biod_client.cc:261: void OnRequestRecordLabel(const LabelCallback& callback, did you re-run the tests? ...
3 years, 8 months ago (2017-04-08 01:13:32 UTC) #7
sammiequon
https://codereview.chromium.org/2799043007/diff/20001/chromeos/dbus/biod/biod_client.cc File chromeos/dbus/biod/biod_client.cc (right): https://codereview.chromium.org/2799043007/diff/20001/chromeos/dbus/biod/biod_client.cc#newcode74 chromeos/dbus/biod/biod_client.cc:74: biod_proxy_->CallMethodWithErrorCallback( On 2017/04/08 01:06:03, Daniel Erat wrote: > On ...
3 years, 8 months ago (2017-04-08 01:31:48 UTC) #9
Daniel Erat
https://codereview.chromium.org/2799043007/diff/40001/chromeos/dbus/biod/biod_client_unittest.cc File chromeos/dbus/biod/biod_client_unittest.cc (right): https://codereview.chromium.org/2799043007/diff/40001/chromeos/dbus/biod/biod_client_unittest.cc#newcode387 chromeos/dbus/biod/biod_client_unittest.cc:387: biod::BiometricType::BIOMETRIC_TYPE_FINGERPRINT; On 2017/04/08 01:31:48, sammiequon wrote: > On 2017/04/08 ...
3 years, 8 months ago (2017-04-08 01:39:24 UTC) #10
sammiequon
https://codereview.chromium.org/2799043007/diff/40001/chromeos/dbus/biod/biod_client_unittest.cc File chromeos/dbus/biod/biod_client_unittest.cc (right): https://codereview.chromium.org/2799043007/diff/40001/chromeos/dbus/biod/biod_client_unittest.cc#newcode387 chromeos/dbus/biod/biod_client_unittest.cc:387: biod::BiometricType::BIOMETRIC_TYPE_FINGERPRINT; On 2017/04/08 01:39:23, Daniel Erat wrote: > On ...
3 years, 8 months ago (2017-04-08 04:43:46 UTC) #13
Daniel Erat
thanks! lgtm with a few comment corrections https://codereview.chromium.org/2799043007/diff/80001/chromeos/dbus/biod/biod_client_unittest.cc File chromeos/dbus/biod/biod_client_unittest.cc (right): https://codereview.chromium.org/2799043007/diff/80001/chromeos/dbus/biod/biod_client_unittest.cc#newcode340 chromeos/dbus/biod/biod_client_unittest.cc:340: // Return ...
3 years, 8 months ago (2017-04-08 14:55:55 UTC) #16
sammiequon
Thanks! https://codereview.chromium.org/2799043007/diff/80001/chromeos/dbus/biod/biod_client_unittest.cc File chromeos/dbus/biod/biod_client_unittest.cc (right): https://codereview.chromium.org/2799043007/diff/80001/chromeos/dbus/biod/biod_client_unittest.cc#newcode340 chromeos/dbus/biod/biod_client_unittest.cc:340: // Return an empty response to simulate failure. ...
3 years, 8 months ago (2017-04-10 18:10:59 UTC) #19
Daniel Erat
lgtm
3 years, 8 months ago (2017-04-10 18:20:54 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2799043007/120001
3 years, 8 months ago (2017-04-10 20:35:56 UTC) #25
commit-bot: I haz the power
3 years, 8 months ago (2017-04-10 20:44:26 UTC) #28
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/63e20eb86328f72e4c6f2707d49d...

Powered by Google App Engine
This is Rietveld 408576698