New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 856848 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Sep 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 881142


Participants' hotlists:
Better-Together-Launch-Blockers


Sign in to add a comment

"Existing user" notifications are never displayed

Project Member Reported by khorimoto@chromium.org, Jun 26 2018

Issue description

Specifically, it repeats some logic in EligibleHostDevicesProvider and HostStatusProvider.

Not a high priority now, but should be cleaned up at some point.
 
Labels: -Pri-3 M-70 Pri-1
Summary: "Existing user" notifications are never displayed (was: AccountStatusChangeDelegateNotifierImpl should utilize other helper classes)
There are two notifications displayed to existing users:
  * Multi-device host switched to another device
  * A new Chromebook was added which uses Better Together

However, AccountStatusChangeDelegateNotifierImpl never actually notifies the delegate for either of these events. This change was due to a change in our protocol. Previously, we planned to use the "Bluetooth verification" step, in which client devices would create a Bluetooth connection, then enable all of their supported client features. However, this plan changed when we removed the verification step; now, clients never actually enable the client features, and they remain in the "supported" state. (We may choose to change this design later at some point, but that change is not high priority as our existing design works as-is.)

AccountStatusChangeDelegateNotifierImpl keeps a boolean instance field called |local_device_is_enabled_client_| which is set when the local device has an "enabled" state for BETTER_TOGETHER_CLIENT. This boolean must be true in order for either of the two existing user notifications to be displayed. Since the BETTER_TOGETHER_CLIENT feature never becomes enabled, this boolean is never true.

Instead, AccountStatusChangeDelegateNotifierImpl should use HostStatusProvider; if the host status is kHostVerified, this means that the Chromebook is enabled as a client. Additionally, as mentioned in the original bug description, we can also refactor AccountStatusChangeDelegateNotifierImpl to utilize EligibleHostDevicesProvider to avoid repeating logic which is already implemented.
Status: Started (was: Assigned)
Blocking: 881142
Project Member

Comment 4 by bugdroid1@chromium.org, 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

Labels: Merge-Rejected-70
Labels: -Merge-Rejected-70 Merge-Request-70
Labels: -Type-Feature Type-Bug
Why is this a launch blocker?
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.
Labels: -Merge-Request-70 Merge-Approved-70
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 7

Labels: -merge-approved-70 merge-merged-3538
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

Status: Fixed (was: Started)

Sign in to add a comment