"Existing user" notifications are never displayed |
|||||||||
Issue descriptionSpecifically, it repeats some logic in EligibleHostDevicesProvider and HostStatusProvider. Not a high priority now, but should be cleaned up at some point.
,
Aug 30
,
Sep 6
,
Sep 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4862d04376fae91c07fe483c2def1ce014730290 commit 4862d04376fae91c07fe483c2def1ce014730290 Author: Jordy Greenblatt <jordynass@chromium.org> Date: Thu Sep 06 22:13:13 2018 [CrOS MultiDevice] Fire notifications based on host (not client) state. As part of the MultiDevice Unified Setup project, the Chromebook is supposed to notify the user for each of the following situations: (1) A user who has not been through the Unified Setup Flow has a phone capable of hosting some MultiDevice features (a.k.a. 'new host'), (2) A user who has already been through the Unified Setup Flow has a new host phone on their accounts (a.k.a. 'host switched'), or (3) The currently logged in account went through the Unified Setup Flow on a different Chromebook (a.k.a. 'Chromebook added'). In the original design, implemented in https://chromium-review.googlesource.com/c/chromium/src/+/1081287 and https://chromium-review.googlesource.com/c/chromium/src/+/1089828 the Chromebook would have a bit in its preferences to indicate that it had been enabled as a MultiDevice client. However we no longer perform the verification necessary for to turn this on as part of the Unified Setup flow so the bit is longer a valid basis for determining whether/ when to fire the notifications listed above. This CL refactors the class responsible for this determination (formerly ObserverNotifier, now AccountStatusChangeDelegateNotifierImpl) so that it uses the status of an already set MultiDevice host (or the lack thereof) to decide whether/ when to fire the notifications. In particular, the HostStatusProvider class makes the raw data required readily available. ----- A minor functionality change was implemented in the 'Chromebook added' event. Prior to this CL, the check for that event, i.e. AccountStatusChangeDelegateNotifierImpl:: CheckForExistingUserChromebookAddedEvent, would not fire a notification if the SetupFlowCompletionRecorder says that the Chromebook was previously used for Unified Setup. We decided this is not ideal behavior because of the following case: A user goes through Unified Setup on Chromebook A, forgets the host on Chromebook A, and then go through Unified Setup on Chromebook B. This automatically enabled the MultiDevice features on Chromebook A. In the intended behavior before this CL (I say 'intended' because the implementation was incomplete) this would not trigger a notification and a user could be in a state in which they do not realize that Chromebook A is connecting to their host phone. Removing the check for whether Chromebook A has gone through Unified Setup prevents this from happening. It has a side effect that the user will see the 'Chromebook added' notification on Chromebook A immediately following following a successful setup on Chromebook A but this is a much smaller concern than the potential privacy issue described above. I filed a bug https://bugs.chromium.org/p/chromium/issues/detail?id=881549 to either use or get rid of the SetupFlowCompletionRecorder pointer in AccountStatusChangeDelegateNotifierImpl. ----- Note that most of the unit tests from the aforementioned CLs 1081287 and 1089828 remained relevant modulo some adaptations. A small number were dropped altogether as they were inextricably tied to the implementation. Bug: 856848 Change-Id: I49d884847f7e21516ea8af1a7264843386103c70 Reviewed-on: https://chromium-review.googlesource.com/1205495 Reviewed-by: Jeremy Klein <jlklein@chromium.org> Commit-Queue: Jordy Greenblatt <jordynass@chromium.org> Cr-Commit-Position: refs/heads/master@{#589334} [modify] https://crrev.com/4862d04376fae91c07fe483c2def1ce014730290/chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.cc [modify] https://crrev.com/4862d04376fae91c07fe483c2def1ce014730290/chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.h [modify] https://crrev.com/4862d04376fae91c07fe483c2def1ce014730290/chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl_unittest.cc [modify] https://crrev.com/4862d04376fae91c07fe483c2def1ce014730290/chromeos/services/multidevice_setup/multidevice_setup_impl.cc [modify] https://crrev.com/4862d04376fae91c07fe483c2def1ce014730290/chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc
,
Sep 6
,
Sep 6
,
Sep 7
,
Sep 7
Why is this a launch blocker?
,
Sep 7
There are some privacy and security issues involved with not notifying users that Better Together is enabled when they sign into a new Chromebook or get a new phone.
,
Sep 7
,
Sep 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2776356f3f462f937c5f8ba9a510e7e320fee0dc commit 2776356f3f462f937c5f8ba9a510e7e320fee0dc Author: Jordy Greenblatt <jordynass@chromium.org> Date: Fri Sep 07 22:16:21 2018 [CrOS MultiDevice] Fire notifications based on host (not client) state. As part of the MultiDevice Unified Setup project, the Chromebook is supposed to notify the user for each of the following situations: (1) A user who has not been through the Unified Setup Flow has a phone capable of hosting some MultiDevice features (a.k.a. 'new host'), (2) A user who has already been through the Unified Setup Flow has a new host phone on their accounts (a.k.a. 'host switched'), or (3) The currently logged in account went through the Unified Setup Flow on a different Chromebook (a.k.a. 'Chromebook added'). In the original design, implemented in https://chromium-review.googlesource.com/c/chromium/src/+/1081287 and https://chromium-review.googlesource.com/c/chromium/src/+/1089828 the Chromebook would have a bit in its preferences to indicate that it had been enabled as a MultiDevice client. However we no longer perform the verification necessary for to turn this on as part of the Unified Setup flow so the bit is longer a valid basis for determining whether/ when to fire the notifications listed above. This CL refactors the class responsible for this determination (formerly ObserverNotifier, now AccountStatusChangeDelegateNotifierImpl) so that it uses the status of an already set MultiDevice host (or the lack thereof) to decide whether/ when to fire the notifications. In particular, the HostStatusProvider class makes the raw data required readily available. ----- A minor functionality change was implemented in the 'Chromebook added' event. Prior to this CL, the check for that event, i.e. AccountStatusChangeDelegateNotifierImpl:: CheckForExistingUserChromebookAddedEvent, would not fire a notification if the SetupFlowCompletionRecorder says that the Chromebook was previously used for Unified Setup. We decided this is not ideal behavior because of the following case: A user goes through Unified Setup on Chromebook A, forgets the host on Chromebook A, and then go through Unified Setup on Chromebook B. This automatically enabled the MultiDevice features on Chromebook A. In the intended behavior before this CL (I say 'intended' because the implementation was incomplete) this would not trigger a notification and a user could be in a state in which they do not realize that Chromebook A is connecting to their host phone. Removing the check for whether Chromebook A has gone through Unified Setup prevents this from happening. It has a side effect that the user will see the 'Chromebook added' notification on Chromebook A immediately following following a successful setup on Chromebook A but this is a much smaller concern than the potential privacy issue described above. I filed a bug https://bugs.chromium.org/p/chromium/issues/detail?id=881549 to either use or get rid of the SetupFlowCompletionRecorder pointer in AccountStatusChangeDelegateNotifierImpl. ----- Note that most of the unit tests from the aforementioned CLs 1081287 and 1089828 remained relevant modulo some adaptations. A small number were dropped altogether as they were inextricably tied to the implementation. Bug: 856848 Change-Id: I49d884847f7e21516ea8af1a7264843386103c70 Reviewed-on: https://chromium-review.googlesource.com/1205495 Reviewed-by: Jeremy Klein <jlklein@chromium.org> Commit-Queue: Jordy Greenblatt <jordynass@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#589334}(cherry picked from commit 4862d04376fae91c07fe483c2def1ce014730290) Reviewed-on: https://chromium-review.googlesource.com/1214326 Cr-Commit-Position: refs/branch-heads/3538@{#172} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/2776356f3f462f937c5f8ba9a510e7e320fee0dc/chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.cc [modify] https://crrev.com/2776356f3f462f937c5f8ba9a510e7e320fee0dc/chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.h [modify] https://crrev.com/2776356f3f462f937c5f8ba9a510e7e320fee0dc/chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl_unittest.cc [modify] https://crrev.com/2776356f3f462f937c5f8ba9a510e7e320fee0dc/chromeos/services/multidevice_setup/multidevice_setup_impl.cc [modify] https://crrev.com/2776356f3f462f937c5f8ba9a510e7e320fee0dc/chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc
,
Sep 7
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by khorimoto@chromium.org
, Aug 28Summary: "Existing user" notifications are never displayed (was: AccountStatusChangeDelegateNotifierImpl should utilize other helper classes)