Create new Smart Lock user pref to ensure SmartLock can be disabled correctly |
||||||||
Issue descriptionTrying to migrate from the existing Smart Lock pref is difficult and causes several edge cases, which are hardly worth dealing with because the code will be ripped out soon. For example, EasyUnlockServiceRegular as-is sets the user pref when we don't want it to. Additionally, when users go through Unified Setup flow, they'll need re-enable Smart Lock anyway, so there will be no harm in moving on to a new Smart Lock pref at that moment. This is a launch blocker for M70. (using deprecated UI>ProximityAuth component for tracking purposes for now until a proper hotlist is created)
,
Sep 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7 commit 92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7 Author: Ryan Hansberry <hansberry@chromium.org> Date: Thu Sep 06 21:58:30 2018 Smart Lock: Create a new user pref and make EasyUnlockService listen on it. Use a new Smart Lock user pref to track if Smart Lock is fully enabled. This removes the need to consider additional complexity when migrating to Unified Setup. Additionally, make EasyUnlockServiceRegular and ProximityAuthProfilePrefManager now consult MultiDeviceSetupClient if Smart Lock is enabled (and listen on feature state changes from it). To be clear, these two classes no longer directly set the Smart Lock user pref, and instead only determine if Smart Lock is enabled by asking MultiDeviceSetupClient. R=jhawkins@chromium.org, jlklein@chromium.org Bug: 881435 Change-Id: I0a8535c19789e826d277258f76ce384e27cc8652 Reviewed-on: https://chromium-review.googlesource.com/1211246 Reviewed-by: Jeremy Klein <jlklein@chromium.org> Reviewed-by: James Hawkins <jhawkins@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> Commit-Queue: Ryan Hansberry <hansberry@chromium.org> Cr-Commit-Position: refs/heads/master@{#589319} [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_factory.cc [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_regular.cc [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_regular.h [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_unittest_chromeos.cc [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chromeos/components/proximity_auth/BUILD.gn [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chromeos/components/proximity_auth/proximity_auth_profile_pref_manager.cc [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chromeos/components/proximity_auth/proximity_auth_profile_pref_manager.h [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chromeos/components/proximity_auth/proximity_auth_profile_pref_manager_unittest.cc [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chromeos/components/proximity_auth/proximity_auth_system_unittest.cc [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chromeos/components/proximity_auth/proximity_monitor_impl_unittest.cc [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chromeos/services/multidevice_setup/public/cpp/prefs.cc [modify] https://crrev.com/92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7/chromeos/services/multidevice_setup/public/cpp/prefs.h
,
Sep 6
Requesting merge into M70. Smart Lock cannot correctly turn off without this change, so it's very important to get in.
,
Sep 7
,
Sep 7
,
Sep 7
Your change meets the bar and is auto-approved for M70. Please go ahead and merge the CL to branch 3538 manually. Please contact milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/94ee6a7232d530cf06c897ee572eea5d488767e4 commit 94ee6a7232d530cf06c897ee572eea5d488767e4 Author: Ryan Hansberry <hansberry@chromium.org> Date: Fri Sep 07 22:09:59 2018 Smart Lock: Create a new user pref and make EasyUnlockService listen on it. Use a new Smart Lock user pref to track if Smart Lock is fully enabled. This removes the need to consider additional complexity when migrating to Unified Setup. Additionally, make EasyUnlockServiceRegular and ProximityAuthProfilePrefManager now consult MultiDeviceSetupClient if Smart Lock is enabled (and listen on feature state changes from it). To be clear, these two classes no longer directly set the Smart Lock user pref, and instead only determine if Smart Lock is enabled by asking MultiDeviceSetupClient. R=jhawkins@chromium.org, jlklein@chromium.org Bug: 881435 Change-Id: I0a8535c19789e826d277258f76ce384e27cc8652 Reviewed-on: https://chromium-review.googlesource.com/1211246 Reviewed-by: Jeremy Klein <jlklein@chromium.org> Reviewed-by: James Hawkins <jhawkins@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> Commit-Queue: Ryan Hansberry <hansberry@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#589319}(cherry picked from commit 92625bc14a8afa0f7bc7d5c62d2ecd69f6950cf7) Reviewed-on: https://chromium-review.googlesource.com/1214324 Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#171} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_factory.cc [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_regular.cc [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_regular.h [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chrome/browser/chromeos/login/easy_unlock/easy_unlock_service_unittest_chromeos.cc [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chromeos/components/proximity_auth/BUILD.gn [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chromeos/components/proximity_auth/proximity_auth_profile_pref_manager.cc [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chromeos/components/proximity_auth/proximity_auth_profile_pref_manager.h [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chromeos/components/proximity_auth/proximity_auth_profile_pref_manager_unittest.cc [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chromeos/components/proximity_auth/proximity_auth_system_unittest.cc [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chromeos/components/proximity_auth/proximity_monitor_impl_unittest.cc [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chromeos/services/multidevice_setup/public/cpp/prefs.cc [modify] https://crrev.com/94ee6a7232d530cf06c897ee572eea5d488767e4/chromeos/services/multidevice_setup/public/cpp/prefs.h
,
Sep 7
,
Sep 10
The fix in question changes the preference value for EasyUnlock in the pre- and post-setup world. However, we want to maintain the same preference that was previously stored there, so we should alter that small part of this change.
,
Sep 10
Kyle, can you explain why maintaining the same pref before and after setup is a priority? We're forcing the user to go through setup again, so I don't see why this is a concern. It's a fairly large untangling/refactoring effort to maintain that same pref and we determined that it wasn't worth trying to clean up that old easyunlock code right now when it's so fragile.
,
Sep 10
Apologies for the lack of context here Kyle -- James and I consciously decided that it was simpler to simply go ahead with creating a new pref -- Jeremy listed the two primary reasons.
,
Sep 10
If the user has disabled EasyUnlock in the pre-setup world, then the user goes through Unified Setup, shouldn't EasyUnlock still be disabled? We do the same for Instant Tethering, so I don't see why we should change it for EasyUnlock. Can you explain what the large untangling/refactoring effort is? Unless I'm missing something, it seems we could have just reused the existing pref and not added a new one.
,
Sep 10
Unified Setup is a non-backwards compatible change and we are OK with requiring the user to make the decision to enable/disable SmartLock (or any other feature) again.
,
Sep 10
SGTM |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by jlklein@chromium.org
, Sep 6