Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionHost 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 #Messages
Total messages: 105 (80 generated)
The CQ bit was checked by xiaoyinh@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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== 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. ========== to ========== 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. ==========
xiaoyinh@chromium.org changed reviewers: + blundell@chromium.org, rkc@chromium.org, rockot@chromium.org, tsepez@chromium.org
The CQ bit was checked by xiaoyinh@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by xiaoyinh@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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2664353002/diff/60001/device/fingerprint/publ... File device/fingerprint/public/interfaces/fingerprint.mojom (right): https://codereview.chromium.org/2664353002/diff/60001/device/fingerprint/publ... device/fingerprint/public/interfaces/fingerprint.mojom:9: OnScanned(uint32 scan_result, bool is_complete); is scan_result really a uint32, or some sort of enum? https://codereview.chromium.org/2664353002/diff/60001/device/fingerprint/publ... device/fingerprint/public/interfaces/fingerprint.mojom:15: GetFingerprintsList() => (array<string> enrollments); Is this a privileged interface? Which clients do you expect to call this? Are the callers more trusted than the service itself?
The CQ bit was checked by xiaoyinh@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Hi Sarah, Two high-level comments: (1) The goal is for all the features that the Device Service hosts to be migrated to live entirely within //services/device. Since we're starting the fingerprint service from scratch, we can do it the right way from the start. That means: - fingerprint.mojom lives in //services/device/public/interfaces/ (you'll have to create this directory and its BUILD.gn file) - The impl lives in //services/device/fingerprint/. Give it its own BUILD.gn file (as you're doing now) and restrict its visibility to //services/device. This will ensure that all clients of the fingerprint feature access it solely via the Device Service. (2) The problem you're seeing on the bots is because you're building ChromeOS-specific code on all platforms. What you'll want to do is add a default implementation that is do-nothing and used on all platforms but ChromeOS. You'll declare the static Create() function in the common header and then implement it in both the default (empty) impl and the ChromeOS impl to create the approriate instance in each case. For a model, see //devices/vibration. Feel free to reach out if you'd like more clarity on either of these.
On 2017/02/02 13:02:46, blundell wrote: > Hi Sarah, > > Two high-level comments: > > (1) The goal is for all the features that the Device Service hosts to be > migrated to live entirely within //services/device. Since we're starting the > fingerprint service from scratch, we can do it the right way from the start. > That means: > - fingerprint.mojom lives in //services/device/public/interfaces/ (you'll have > to create this directory and its BUILD.gn file) (actually, this directory already exists, so even better) > - The impl lives in //services/device/fingerprint/. Give it its own BUILD.gn > file (as you're doing now) and restrict its visibility to //services/device. > This will ensure that all clients of the fingerprint feature access it solely > via the Device Service. > > (2) The problem you're seeing on the bots is because you're building > ChromeOS-specific code on all platforms. What you'll want to do is add a default > implementation that is do-nothing and used on all platforms but ChromeOS. You'll > declare the static Create() function in the common header and then implement it > in both the default (empty) impl and the ChromeOS impl to create the approriate > instance in each case. For a model, see //devices/vibration. > > Feel free to reach out if you'd like more clarity on either of these.
The CQ bit was checked by xiaoyinh@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by xiaoyinh@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_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
xiaoyinh@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb@ for changes in services/device/fingerprint/DEPS https://codereview.chromium.org/2664353002/diff/60001/device/fingerprint/publ... File device/fingerprint/public/interfaces/fingerprint.mojom (right): https://codereview.chromium.org/2664353002/diff/60001/device/fingerprint/publ... device/fingerprint/public/interfaces/fingerprint.mojom:9: OnScanned(uint32 scan_result, bool is_complete); On 2017/02/01 16:30:17, Tom Sepez wrote: > is scan_result really a uint32, or some sort of enum? Thanks for your comment. You're right, scan_result is a enum and it will be defined in third_party/cros_system_api/dbus/service_constants.h along with other fingerprint related constants. I can keep a copy of the enum here if that's desired. https://codereview.chromium.org/2664353002/diff/60001/device/fingerprint/publ... device/fingerprint/public/interfaces/fingerprint.mojom:15: GetFingerprintsList() => (array<string> enrollments); On 2017/02/01 16:30:17, Tom Sepez wrote: > Is this a privileged interface? Which clients do you expect to call this? Are > the callers more trusted than the service itself? I think right now only md-settings and lock screen will call this. And they are inside the chrome process, so I assume they are more trusted than the service itself. Let me know if you think otherwise.
The CQ bit was checked by xiaoyinh@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by xiaoyinh@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Hi Sarah, As a general rule I only send out patches for review when all the trybots are green, unless you're explicitly looking for help fixing red trybots (since otherwise reviewers are reviewing code that is known to be incorrect). If you do want help investigating the red trybots, let me know and I'm happy to lend a pair of eyes :). Thanks, Colin
On 2017/02/03 08:39:16, blundell wrote: > Hi Sarah, > > As a general rule I only send out patches for review when all the trybots are > green, unless you're explicitly looking for help fixing red trybots (since > otherwise reviewers are reviewing code that is known to be incorrect). If you do > want help investigating the red trybots, let me know and I'm happy to lend a > pair of eyes :). > > Thanks, > > Colin Thank for your comment. I will investigate more on this. And please ignore my new patches if they are still trybot-red. Thanks!
The CQ bit was checked by xiaoyinh@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.
Yay! Trybots are all green now. Please take a look.
OK, LGTM.
Thanks, this looks good! Please add OWNERS for the fingerprint feature (ideally at least two). I didn't review the code of fingerprint_impl_chromeos.*. https://codereview.chromium.org/2664353002/diff/180001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_manifest_overlay.json (right): https://codereview.chromium.org/2664353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_manifest_overlay.json:54: "device": [ "device:fingerprint" ] This seems better-suited to the CL where you introduce usage of this interface. https://codereview.chromium.org/2664353002/diff/180001/services/device/device... File services/device/device_service.h (right): https://codereview.chromium.org/2664353002/diff/180001/services/device/device... services/device/device_service.h:27: public service_manager::InterfaceFactory<mojom::Fingerprint> { tiny OCD-ish nit: hoist Fingerpint up above PowerMonitor. https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... File services/device/fingerprint/BUILD.gn (right): https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... services/device/fingerprint/BUILD.gn:7: component("fingerprint") { Please restrict the visibility of this target to //services/device ("gn help visibility"). https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... File services/device/fingerprint/fingerprint_impl.h (right): https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... services/device/fingerprint/fingerprint_impl.h:13: class FingerprintImpl { nit: This class can be called Fingerprint (that's one of the reasons the nested mojom namespace was introduced for generated code). https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... services/device/fingerprint/fingerprint_impl.h:15: SERVICES_DEVICE_FINGERPRINT_EXPORT static void Create( nit: A comment would be useful here. ("Implemented in platform-specific subclasses"). https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... File services/device/fingerprint/fingerprint_impl_chromeos.h (right): https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... services/device/fingerprint/fingerprint_impl_chromeos.h:16: class FingerprintImplChromeOS : public mojom::Fingerprint { nit: Could use a class comment. https://codereview.chromium.org/2664353002/diff/180001/services/device/public... File services/device/public/interfaces/fingerprint.mojom (right): https://codereview.chromium.org/2664353002/diff/180001/services/device/public... services/device/public/interfaces/fingerprint.mojom:7: interface BiodObserver{ These interfaces and methods should have comments explaining their usage.
The CQ bit was checked by xiaoyinh@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...
steel@chromium.org changed reviewers: + steel@chromium.org
https://codereview.chromium.org/2664353002/diff/200001/services/device/finger... File services/device/fingerprint/fingerprint_impl_default.h (right): https://codereview.chromium.org/2664353002/diff/200001/services/device/finger... services/device/fingerprint/fingerprint_impl_default.h:15: class FingerprintImplDefault : public mojom::Fingerprint { Is this class strictly necessary? Can we instead just an empty definition of class Fingerprint? https://codereview.chromium.org/2664353002/diff/200001/services/device/public... File services/device/public/interfaces/fingerprint.mojom (right): https://codereview.chromium.org/2664353002/diff/200001/services/device/public... services/device/public/interfaces/fingerprint.mojom:20: OnAttempt(uint32 scan_result, array<string> recognized_user_ids); Please sync up with Zach to match this with the changes he is making to the biod D-Bus service. I believe one of the changes was that we'd return a map of user_ids, with each element pointing to a list of labels.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
One more thing - tests? :)
The CQ bit was checked by xiaoyinh@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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by xiaoyinh@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2664353002/diff/260001/services/device/finger... File services/device/fingerprint/BUILD.gn (right): https://codereview.chromium.org/2664353002/diff/260001/services/device/finger... services/device/fingerprint/BUILD.gn:29: "//dbus", I don't see any actual chromeos/dbus dependencies here, can we add these if/when we need tem?
The CQ bit was checked by xiaoyinh@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by xiaoyinh@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
On 2017/02/15 21:51:17, stevenjb wrote: > https://codereview.chromium.org/2664353002/diff/260001/services/device/finger... > File services/device/fingerprint/BUILD.gn (right): > > https://codereview.chromium.org/2664353002/diff/260001/services/device/finger... > services/device/fingerprint/BUILD.gn:29: "//dbus", > I don't see any actual chromeos/dbus dependencies here, can we add these if/when > we need tem? Thanks for the comments. I've removed the chromeos deps, //dbus are needed as FingerprintImplChromeOS is including "dbus/object_path.h"
Incorporated the above comments, also add a unittest: fingerprint_impl_chromeos_unittest.cc https://codereview.chromium.org/2664353002/diff/180001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_manifest_overlay.json (right): https://codereview.chromium.org/2664353002/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_manifest_overlay.json:54: "device": [ "device:fingerprint" ] On 2017/02/06 14:52:42, blundell wrote: > This seems better-suited to the CL where you introduce usage of this interface. Thanks, removed. https://codereview.chromium.org/2664353002/diff/180001/services/device/device... File services/device/device_service.h (right): https://codereview.chromium.org/2664353002/diff/180001/services/device/device... services/device/device_service.h:27: public service_manager::InterfaceFactory<mojom::Fingerprint> { On 2017/02/06 14:52:43, blundell wrote: > tiny OCD-ish nit: hoist Fingerpint up above PowerMonitor. Done. https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... File services/device/fingerprint/BUILD.gn (right): https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... services/device/fingerprint/BUILD.gn:7: component("fingerprint") { On 2017/02/06 14:52:43, blundell wrote: > Please restrict the visibility of this target to //services/device ("gn help > visibility"). Done. https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... File services/device/fingerprint/fingerprint_impl.h (right): https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... services/device/fingerprint/fingerprint_impl.h:13: class FingerprintImpl { On 2017/02/06 14:52:43, blundell wrote: > nit: This class can be called Fingerprint (that's one of the reasons the nested > mojom namespace was introduced for generated code). Done. https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... services/device/fingerprint/fingerprint_impl.h:15: SERVICES_DEVICE_FINGERPRINT_EXPORT static void Create( On 2017/02/06 14:52:43, blundell wrote: > nit: A comment would be useful here. ("Implemented in platform-specific > subclasses"). Done. https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... File services/device/fingerprint/fingerprint_impl_chromeos.h (right): https://codereview.chromium.org/2664353002/diff/180001/services/device/finger... services/device/fingerprint/fingerprint_impl_chromeos.h:16: class FingerprintImplChromeOS : public mojom::Fingerprint { On 2017/02/06 14:52:43, blundell wrote: > nit: Could use a class comment. Done. https://codereview.chromium.org/2664353002/diff/180001/services/device/public... File services/device/public/interfaces/fingerprint.mojom (right): https://codereview.chromium.org/2664353002/diff/180001/services/device/public... services/device/public/interfaces/fingerprint.mojom:7: interface BiodObserver{ On 2017/02/06 14:52:43, blundell wrote: > These interfaces and methods should have comments explaining their usage. Done. https://codereview.chromium.org/2664353002/diff/200001/services/device/finger... File services/device/fingerprint/fingerprint_impl_default.h (right): https://codereview.chromium.org/2664353002/diff/200001/services/device/finger... services/device/fingerprint/fingerprint_impl_default.h:15: class FingerprintImplDefault : public mojom::Fingerprint { On 2017/02/06 21:31:57, Rahul Chaturvedi wrote: > Is this class strictly necessary? Can we instead just an empty definition of > class Fingerprint? Done. https://codereview.chromium.org/2664353002/diff/200001/services/device/public... File services/device/public/interfaces/fingerprint.mojom (right): https://codereview.chromium.org/2664353002/diff/200001/services/device/public... services/device/public/interfaces/fingerprint.mojom:20: OnAttempt(uint32 scan_result, array<string> recognized_user_ids); On 2017/02/06 21:31:57, Rahul Chaturvedi wrote: > Please sync up with Zach to match this with the changes he is making to the biod > D-Bus service. I believe one of the changes was that we'd return a map of > user_ids, with each element pointing to a list of labels. Thanks, I will incorporate the API change once that's landed.
Introduction of fingerprint into Device Service lgtm, thanks! I didn't review the internals of fingerprint_chromeos*. 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.... services/device/BUILD.gn:53: deps += [ "//services/device/fingerprint" ] nit: not needed. https://codereview.chromium.org/2664353002/diff/300001/services/device/device... File services/device/device_service.cc (right): https://codereview.chromium.org/2664353002/diff/300001/services/device/device... services/device/device_service.cc:53: Fingerprint::CreateFingerprint(std::move(request)); naming nit: probably just Fingerprint::Create(...)? https://codereview.chromium.org/2664353002/diff/300001/services/device/finger... File services/device/fingerprint/fingerprint.h (right): https://codereview.chromium.org/2664353002/diff/300001/services/device/finger... services/device/fingerprint/fingerprint.h:15: // This function will be implemented in platform-specific subclasses. nit: s/will be/is https://codereview.chromium.org/2664353002/diff/300001/services/device/finger... File services/device/fingerprint/fingerprint_impl_chromeos.h (right): https://codereview.chromium.org/2664353002/diff/300001/services/device/finger... services/device/fingerprint/fingerprint_impl_chromeos.h:18: // operations. It will also observe signals from biod. nit: s/will also observe/observes https://codereview.chromium.org/2664353002/diff/300001/services/device/public... File services/device/public/interfaces/fingerprint.mojom (right): https://codereview.chromium.org/2664353002/diff/300001/services/device/public... services/device/public/interfaces/fingerprint.mojom:5: module device.mojom; Could you add an OWNERS file in this directory and have per-file ownership of fingerprint.mojom that points to //services/device/fingerprint (see //components/OWNERS for examples of what I mean)? https://codereview.chromium.org/2664353002/diff/300001/services/device/public... services/device/public/interfaces/fingerprint.mojom:7: // Interface for obeserving biod signals. hmm, is biod ChromeOS-specific? Seems a little strange to have as a concept in the interface if so. https://codereview.chromium.org/2664353002/diff/300001/services/device/public... services/device/public/interfaces/fingerprint.mojom:57: // Add biod observers and notify them when receiving signals. nit: Adds ... and notifies
DEPS change lgtm with one request. https://codereview.chromium.org/2664353002/diff/300001/services/device/finger... File services/device/fingerprint/fingerprint_impl_chromeos.h (right): https://codereview.chromium.org/2664353002/diff/300001/services/device/finger... services/device/fingerprint/fingerprint_impl_chromeos.h:11: #include "dbus/object_path.h" You should be able to forward declare ObjectPath and move the include to the C++ file.
DEPS change lgtm with one request.
The CQ bit was checked by xiaoyinh@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.
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.... services/device/BUILD.gn:53: deps += [ "//services/device/fingerprint" ] On 2017/02/22 15:20:06, blundell wrote: > nit: not needed. fingerprint_impl_chromeos_unittest.cc includes fingerprint_impl_chromeos.h, so I think this is probably needed. https://codereview.chromium.org/2664353002/diff/300001/services/device/device... File services/device/device_service.cc (right): https://codereview.chromium.org/2664353002/diff/300001/services/device/device... services/device/device_service.cc:53: Fingerprint::CreateFingerprint(std::move(request)); On 2017/02/22 15:20:06, blundell wrote: > naming nit: probably just Fingerprint::Create(...)? Done. https://codereview.chromium.org/2664353002/diff/300001/services/device/finger... File services/device/fingerprint/fingerprint.h (right): https://codereview.chromium.org/2664353002/diff/300001/services/device/finger... services/device/fingerprint/fingerprint.h:15: // This function will be implemented in platform-specific subclasses. On 2017/02/22 15:20:06, blundell wrote: > nit: s/will be/is Done. https://codereview.chromium.org/2664353002/diff/300001/services/device/finger... File services/device/fingerprint/fingerprint_impl_chromeos.h (right): https://codereview.chromium.org/2664353002/diff/300001/services/device/finger... services/device/fingerprint/fingerprint_impl_chromeos.h:11: #include "dbus/object_path.h" On 2017/02/22 16:32:21, stevenjb wrote: > You should be able to forward declare ObjectPath and move the include to the C++ > file. Done. https://codereview.chromium.org/2664353002/diff/300001/services/device/finger... services/device/fingerprint/fingerprint_impl_chromeos.h:18: // operations. It will also observe signals from biod. On 2017/02/22 15:20:06, blundell wrote: > nit: s/will also observe/observes Done. https://codereview.chromium.org/2664353002/diff/300001/services/device/public... File services/device/public/interfaces/fingerprint.mojom (right): https://codereview.chromium.org/2664353002/diff/300001/services/device/public... services/device/public/interfaces/fingerprint.mojom:5: module device.mojom; On 2017/02/22 15:20:06, blundell wrote: > Could you add an OWNERS file in this directory and have per-file ownership of > fingerprint.mojom that points to //services/device/fingerprint (see > //components/OWNERS for examples of what I mean)? This directory already have a OWNER file that link all the mojom files to SECURITY_OWNERS per-file *.mojom=file://ipc/SECURITY_OWNERS Is that possible to link a secondary owner to the same file? https://codereview.chromium.org/2664353002/diff/300001/services/device/public... services/device/public/interfaces/fingerprint.mojom:7: // Interface for obeserving biod signals. On 2017/02/22 15:20:09, blundell wrote: > hmm, is biod ChromeOS-specific? Seems a little strange to have as a concept in > the interface if so. Yes, biod is chromeos-specific, and fingerprint interface is also chromeos-specific. The purpose of defining a mojom interface is to make easier communication(less layers) with webui and arc++. Please let me know if/how I can make things more clear. https://codereview.chromium.org/2664353002/diff/300001/services/device/public... services/device/public/interfaces/fingerprint.mojom:57: // Add biod observers and notify them when receiving signals. On 2017/02/22 15:20:08, blundell wrote: > nit: Adds ... and notifies Done.
https://codereview.chromium.org/2664353002/diff/320001/services/device/public... File services/device/public/interfaces/fingerprint.mojom (right): https://codereview.chromium.org/2664353002/diff/320001/services/device/public... services/device/public/interfaces/fingerprint.mojom:7: // Interface for obeserving biod signals. It's a little strange that we're putting a ChromeOS-specific interface in the Device Service (as opposed to a generic interface that at the present time only has a ChromeOS implementation). Can you add a comment at the top of this file that says something like: "This interface is ChromeOS-specific (e.g., the references to Biod). If it is ever desired to support a more general fingerprint service across more platforms, the interface would need to be generalized."
The CQ bit was checked by xiaoyinh@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 xiaoyinh@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_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xiaoyinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, stevenjb@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2664353002/#ps380001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % comment https://codereview.chromium.org/2664353002/diff/360001/services/device/finger... File services/device/fingerprint/BUILD.gn (right): https://codereview.chromium.org/2664353002/diff/360001/services/device/finger... services/device/fingerprint/BUILD.gn:25: if (is_chromeos) { This looks a little backwards. This would be a lot clearer if we had something like, sources = [ "fingerprint.h", "fingerprint_export.h" ] if (is_chromeos) { sources += [ "fingerprint_impl_chromeos.h", "fingerprint_impl_default.cc", ] deps += [ "//dbus" ] } else { sources += [ "fingerprint_impl_default.cc" ] }
The CQ bit was unchecked by xiaoyinh@chromium.org
https://codereview.chromium.org/2664353002/diff/360001/services/device/finger... File services/device/fingerprint/BUILD.gn (right): https://codereview.chromium.org/2664353002/diff/360001/services/device/finger... services/device/fingerprint/BUILD.gn:25: if (is_chromeos) { On 2017/02/27 22:26:43, Rahul Chaturvedi wrote: > This looks a little backwards. > > This would be a lot clearer if we had something like, > > sources = [ > "fingerprint.h", > "fingerprint_export.h" > ] > > if (is_chromeos) { > sources += [ > "fingerprint_impl_chromeos.h", > "fingerprint_impl_default.cc", > ] > deps += [ "//dbus" ] > } else { > sources += [ "fingerprint_impl_default.cc" ] > } Done.
The CQ bit was checked by xiaoyinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, steel@chromium.org, tsepez@chromium.org, blundell@chromium.org Link to the patchset: https://codereview.chromium.org/2664353002/#ps400001 (title: "comments")
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": 1488241986449190,
"parent_rev": "a6f225f3f8a8fb31228e038217484ffff259463c", "commit_rev":
"a84f21653890b621907da8446a217875d2bf51c1"}
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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/+/a84f21653890b621907da8446a21... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:400001) as https://chromium.googlesource.com/chromium/src/+/a84f21653890b621907da8446a21...
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()'. |
