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

Issue 2664353002: Host fingerprint mojo service within the device service. (Closed)

Created:
3 years, 10 months ago by xiaoyinh(OOO Sep 11-29)
Modified:
3 years, 9 months ago
CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin-cc_chromium.org, darin (slow to review)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Host fingerprint mojo service within the device service. BUG=687393 This service will get fingerprint information from biod through dbus and provide it to md-settings. Since biod APIs are not finalized yet, FingerprintImpl is created as a stub for now. Review-Url: https://codereview.chromium.org/2664353002 Cr-Commit-Position: refs/heads/master@{#453458} Committed: https://chromium.googlesource.com/chromium/src/+/a84f21653890b621907da8446a217875d2bf51c1

Patch Set 1 #

Patch Set 2 : cleanup #

Patch Set 3 : try to fix BUILD.gn #

Total comments: 4

Patch Set 4 : try to fix BUILD.gn #

Patch Set 5 : Incorporate comments from blundell@ #

Patch Set 6 : fix trybot #

Patch Set 7 : Adding fingerprint_export.h #

Patch Set 8 : modify BUILD.gn #

Patch Set 9 : Modify BUILD.gn #

Total comments: 14

Patch Set 10 : incorporate comments and rebase #

Total comments: 4

Patch Set 11 : Incorporate comments #

Patch Set 12 : rebase #

Patch Set 13 : fix trybot #

Total comments: 1

Patch Set 14 : trybot #

Patch Set 15 : rebase and trybot #

Total comments: 16

Patch Set 16 : Incorporate comments from blundell@ and stevenjb@ #

Total comments: 1

Patch Set 17 : Add comments in fingerprint.mojom #

Patch Set 18 : Remove biod reference in mojom interface #

Total comments: 2

Patch Set 19 : rebase #

Patch Set 20 : comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+530 lines, -1 line) Patch
M services/device/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +6 lines, -0 lines 0 comments Download
M services/device/device_service.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +6 lines, -0 lines 0 comments Download
M services/device/device_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +7 lines, -0 lines 0 comments Download
A services/device/fingerprint/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +35 lines, -0 lines 0 comments Download
A services/device/fingerprint/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -0 lines 0 comments Download
A services/device/fingerprint/OWNERS View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
A services/device/fingerprint/fingerprint.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +22 lines, -0 lines 0 comments Download
A services/device/fingerprint/fingerprint_export.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +30 lines, -0 lines 0 comments Download
A services/device/fingerprint/fingerprint_impl_chromeos.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +83 lines, -0 lines 0 comments Download
A services/device/fingerprint/fingerprint_impl_chromeos.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +135 lines, -0 lines 0 comments Download
A services/device/fingerprint/fingerprint_impl_chromeos_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +108 lines, -0 lines 0 comments Download
A services/device/fingerprint/fingerprint_impl_default.cc View 1 2 3 4 5 6 7 8 9 10 13 14 15 1 chunk +27 lines, -0 lines 0 comments Download
M services/device/manifest.json View 1 chunk +2 lines, -1 line 0 comments Download
M services/device/public/interfaces/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
A services/device/public/interfaces/fingerprint.mojom View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +63 lines, -0 lines 0 comments Download

Messages

Total messages: 105 (80 generated)
Tom Sepez
https://codereview.chromium.org/2664353002/diff/60001/device/fingerprint/public/interfaces/fingerprint.mojom File device/fingerprint/public/interfaces/fingerprint.mojom (right): https://codereview.chromium.org/2664353002/diff/60001/device/fingerprint/public/interfaces/fingerprint.mojom#newcode9 device/fingerprint/public/interfaces/fingerprint.mojom:9: OnScanned(uint32 scan_result, bool is_complete); is scan_result really a uint32, ...
3 years, 10 months ago (2017-02-01 16:30:17 UTC) #16
blundell
Hi Sarah, Two high-level comments: (1) The goal is for all the features that the ...
3 years, 10 months ago (2017-02-02 13:02:46 UTC) #21
blundell
On 2017/02/02 13:02:46, blundell wrote: > Hi Sarah, > > Two high-level comments: > > ...
3 years, 10 months ago (2017-02-02 13:37:34 UTC) #22
xiaoyinh(OOO Sep 11-29)
+stevenjb@ for changes in services/device/fingerprint/DEPS https://codereview.chromium.org/2664353002/diff/60001/device/fingerprint/public/interfaces/fingerprint.mojom File device/fingerprint/public/interfaces/fingerprint.mojom (right): https://codereview.chromium.org/2664353002/diff/60001/device/fingerprint/public/interfaces/fingerprint.mojom#newcode9 device/fingerprint/public/interfaces/fingerprint.mojom:9: OnScanned(uint32 scan_result, bool is_complete); ...
3 years, 10 months ago (2017-02-02 23:25:59 UTC) #32
blundell
Hi Sarah, As a general rule I only send out patches for review when all ...
3 years, 10 months ago (2017-02-03 08:39:16 UTC) #41
xiaoyinh(OOO Sep 11-29)
On 2017/02/03 08:39:16, blundell wrote: > Hi Sarah, > > As a general rule I ...
3 years, 10 months ago (2017-02-03 17:58:51 UTC) #42
xiaoyinh(OOO Sep 11-29)
Yay! Trybots are all green now. Please take a look.
3 years, 10 months ago (2017-02-03 19:17:15 UTC) #47
Tom Sepez
OK, LGTM.
3 years, 10 months ago (2017-02-03 20:06:52 UTC) #48
blundell
Thanks, this looks good! Please add OWNERS for the fingerprint feature (ideally at least two). ...
3 years, 10 months ago (2017-02-06 14:52:43 UTC) #49
Rahul Chaturvedi
https://codereview.chromium.org/2664353002/diff/200001/services/device/fingerprint/fingerprint_impl_default.h File services/device/fingerprint/fingerprint_impl_default.h (right): https://codereview.chromium.org/2664353002/diff/200001/services/device/fingerprint/fingerprint_impl_default.h#newcode15 services/device/fingerprint/fingerprint_impl_default.h:15: class FingerprintImplDefault : public mojom::Fingerprint { Is this class ...
3 years, 10 months ago (2017-02-06 21:31:57 UTC) #53
Rahul Chaturvedi
One more thing - tests? :)
3 years, 10 months ago (2017-02-06 23:49:44 UTC) #56
stevenjb
https://codereview.chromium.org/2664353002/diff/260001/services/device/fingerprint/BUILD.gn File services/device/fingerprint/BUILD.gn (right): https://codereview.chromium.org/2664353002/diff/260001/services/device/fingerprint/BUILD.gn#newcode29 services/device/fingerprint/BUILD.gn:29: "//dbus", I don't see any actual chromeos/dbus dependencies here, ...
3 years, 10 months ago (2017-02-15 21:51:17 UTC) #65
xiaoyinh(OOO Sep 11-29)
On 2017/02/15 21:51:17, stevenjb wrote: > https://codereview.chromium.org/2664353002/diff/260001/services/device/fingerprint/BUILD.gn > File services/device/fingerprint/BUILD.gn (right): > > https://codereview.chromium.org/2664353002/diff/260001/services/device/fingerprint/BUILD.gn#newcode29 > ...
3 years, 10 months ago (2017-02-21 23:59:55 UTC) #74
xiaoyinh(OOO Sep 11-29)
Incorporated the above comments, also add a unittest: fingerprint_impl_chromeos_unittest.cc https://codereview.chromium.org/2664353002/diff/180001/chrome/browser/chrome_content_browser_manifest_overlay.json File chrome/browser/chrome_content_browser_manifest_overlay.json (right): https://codereview.chromium.org/2664353002/diff/180001/chrome/browser/chrome_content_browser_manifest_overlay.json#newcode54 chrome/browser/chrome_content_browser_manifest_overlay.json:54: ...
3 years, 10 months ago (2017-02-22 00:04:50 UTC) #75
blundell
Introduction of fingerprint into Device Service lgtm, thanks! I didn't review the internals of fingerprint_chromeos*. ...
3 years, 10 months ago (2017-02-22 15:20:12 UTC) #76
stevenjb
DEPS change lgtm with one request. https://codereview.chromium.org/2664353002/diff/300001/services/device/fingerprint/fingerprint_impl_chromeos.h File services/device/fingerprint/fingerprint_impl_chromeos.h (right): https://codereview.chromium.org/2664353002/diff/300001/services/device/fingerprint/fingerprint_impl_chromeos.h#newcode11 services/device/fingerprint/fingerprint_impl_chromeos.h:11: #include "dbus/object_path.h" You ...
3 years, 10 months ago (2017-02-22 16:32:21 UTC) #77
stevenjb
DEPS change lgtm with one request.
3 years, 10 months ago (2017-02-22 16:32:26 UTC) #78
xiaoyinh(OOO Sep 11-29)
https://codereview.chromium.org/2664353002/diff/300001/services/device/BUILD.gn File services/device/BUILD.gn (right): https://codereview.chromium.org/2664353002/diff/300001/services/device/BUILD.gn#newcode53 services/device/BUILD.gn:53: deps += [ "//services/device/fingerprint" ] On 2017/02/22 15:20:06, blundell ...
3 years, 10 months ago (2017-02-22 21:03:17 UTC) #83
blundell
https://codereview.chromium.org/2664353002/diff/320001/services/device/public/interfaces/fingerprint.mojom File services/device/public/interfaces/fingerprint.mojom (right): https://codereview.chromium.org/2664353002/diff/320001/services/device/public/interfaces/fingerprint.mojom#newcode7 services/device/public/interfaces/fingerprint.mojom:7: // Interface for obeserving biod signals. It's a little ...
3 years, 10 months ago (2017-02-23 14:10:41 UTC) #84
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/2664353002/380001
3 years, 9 months ago (2017-02-27 21:45:26 UTC) #95
Rahul Chaturvedi
lgtm % comment https://codereview.chromium.org/2664353002/diff/360001/services/device/fingerprint/BUILD.gn File services/device/fingerprint/BUILD.gn (right): https://codereview.chromium.org/2664353002/diff/360001/services/device/fingerprint/BUILD.gn#newcode25 services/device/fingerprint/BUILD.gn:25: if (is_chromeos) { This looks a ...
3 years, 9 months ago (2017-02-27 22:26:43 UTC) #96
xiaoyinh(OOO Sep 11-29)
https://codereview.chromium.org/2664353002/diff/360001/services/device/fingerprint/BUILD.gn File services/device/fingerprint/BUILD.gn (right): https://codereview.chromium.org/2664353002/diff/360001/services/device/fingerprint/BUILD.gn#newcode25 services/device/fingerprint/BUILD.gn:25: if (is_chromeos) { On 2017/02/27 22:26:43, Rahul Chaturvedi wrote: ...
3 years, 9 months ago (2017-02-28 00:32:48 UTC) #98
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/2664353002/400001
3 years, 9 months ago (2017-02-28 00:33:31 UTC) #101
commit-bot: I haz the power
Committed patchset #20 (id:400001) as https://chromium.googlesource.com/chromium/src/+/a84f21653890b621907da8446a217875d2bf51c1
3 years, 9 months ago (2017-02-28 02:17:40 UTC) #104
Tom Anderson
3 years, 9 months ago (2017-02-28 03:18:10 UTC) #105
Message was sent while issue was closed.
A revert of this CL (patchset #20 id:400001) has been created in
https://codereview.chromium.org/2719833005/ by thomasanderson@chromium.org.

The reason for reverting is: Causing build failure in Linux ChromiumOS Builder
(dbg)
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...

[5011/6465] LINK ./service_unittests
FAILED: service_unittests 
../../third_party/llvm-build/Release+Asserts/bin/clang++ -Wl,--fatal-warnings
-fPIC -Wl,-z,noexecstack -Wl,-z,now -Wl,-z,relro -Wl,-z,defs -Wl,--no-as-needed
-lpthread -Wl,--as-needed -fuse-ld=gold
-B../../third_party/binutils/Linux_x64/Release/bin -Wl,--threads
-Wl,--thread-count=4 -Wl,--icf=all -m64 -pthread -Werror
--sysroot=../../build/linux/ubuntu_precise_amd64-sysroot
-L/b/c/b/linux_chromeos/src/build/linux/ubuntu_precise_amd64-sysroot/lib/x86_64-linux-gnu
-Wl,-rpath-link=/b/c/b/linux_chromeos/src/build/linux/ubuntu_precise_amd64-sysroot/lib/x86_64-linux-gnu
-L/b/c/b/linux_chromeos/src/build/linux/ubuntu_precise_amd64-sysroot/usr/lib/x86_64-linux-gnu
-Wl,-rpath-link=/b/c/b/linux_chromeos/src/build/linux/ubuntu_precise_amd64-sysroot/usr/lib/x86_64-linux-gnu
-L/b/c/b/linux_chromeos/src/build/linux/ubuntu_precise_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6
-Wl,-rpath-link=/b/c/b/linux_chromeos/src/build/linux/ubuntu_precise_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6
-L/b/c/b/linux_chromeos/src/build/linux/ubuntu_precise_amd64-sysroot/usr/lib
-Wl,-rpath-link=/b/c/b/linux_chromeos/src/build/linux/ubuntu_precise_amd64-sysroot/usr/lib
-Wl,-rpath-link=. -Wl,--disable-new-dtags -Wl,-rpath=\$ORIGIN/.
-Wl,-rpath-link=. -Wl,--export-dynamic -o "./service_unittests"
-Wl,--start-group @"./service_unittests.rsp" ./libbase.so ./libbase_i18n.so
./libicui18n.so ./libicuuc.so ./libbindings.so ./libmojo_public_system_cpp.so
./libmojo_public_system.so ./libipc.so ./liburl.so ./libfingerprint.so
./libskia.so ./libcontent.so ./libgpu.so ./libgles2_utils.so ./libgfx.so
./libgeometry.so ./librange.so ./libgl_wrapper.so ./libplatform.so
./libgl_init.so ./libui_base.so ./libui_data_pack.so ./libevents_base.so
./libkeycodes_x11.so ./libui_base_x.so ./libnet.so ./libprotobuf_lite.so
./libcrcrypto.so ./libboringssl.so ./libmedia.so ./libshared_memory_support.so
./libmojo_common_lib.so ./libgfx_ipc.so ./libgfx_ipc_geometry.so
./libgfx_ipc_skia.so ./libcapture_base.so ./libgfx_ipc_color.so ./libcc.so
./libui_base_ime.so ./libdisplay.so ./libdisplay_types.so ./libcapture_lib.so
./libcc_ipc.so ./libcc_surfaces.so ./libaccessibility.so ./libsurface.so
./liburl_ipc.so ./libmojo_system_impl.so ./libstorage_common.so
./libstorage_browser.so ./libgin.so ./libv8.so ./libblink_platform.so
./libcc_animation.so ./libcc_paint.so ./libgles2_c_lib.so ./libwtf.so
./libblink_web.so ./libsandbox_services.so ./libsuid_sandbox_client.so
./libseccomp_bpf.so ./libstartup_tracing.so -Wl,--end-group  -ldl -lrt
-lgmodule-2.0 -lgobject-2.0 -lgthread-2.0 -lglib-2.0 -lnss3 -lnssutil3 -lsmime3
-lplds4 -lplc4 -lnspr4 -lfontconfig 
../../services/device/fingerprint/fingerprint_impl_chromeos_unittest.cc:58:
error: undefined reference to
'device::FingerprintImplChromeOS::BiodBiometricClientRestarted()'
../../services/device/fingerprint/fingerprint_impl_chromeos_unittest.cc:61:
error: undefined reference to
'device::FingerprintImplChromeOS::BiometricsScanEventReceived(unsigned int,
bool)'
../../services/device/fingerprint/fingerprint_impl_chromeos_unittest.cc:66:
error: undefined reference to
'device::FingerprintImplChromeOS::BiometricsAttemptEventReceived(unsigned int,
std::__debug::vector<std::string, std::allocator<std::string> > const&)'
../../services/device/fingerprint/fingerprint_impl_chromeos_unittest.cc:69:
error: undefined reference to
'device::FingerprintImplChromeOS::BiometricsFailureReceived()'
../../services/device/fingerprint/fingerprint_impl_chromeos_unittest.cc:52:
error: undefined reference to
'device::FingerprintImplChromeOS::FingerprintImplChromeOS()'.

Powered by Google App Engine
This is Rietveld 408576698