Hold off on 'Chromebook added' notification until host verification is complete |
|||||
Issue descriptionExplanation paraphrased from khorimoto@: We were supposed to show the "Chromebook added" notification when a user sets up Better Together. Here's the flow: User goes through setup. Chromebook sends a message to the server telling it that a phone has been set. The server sends out its updated data, which indicates that a phone has been set. At this point, the Chromebook is in the "waiting for verification" state. Currently, this is when the notification is shown. The phone notices that it has been set as the host, then it sends a message to the server indicating that it is now verified. The server sends out its updated data, and the laptop now knows that it can use the features. We want to show the notification at this point instead.
,
Oct 3
How does this reconcile with crbug.com/884373 ?
,
Oct 3
,
Oct 3
I thought a little about it this morning but at the moment I'm not sure if crbug.com/884373 will be a little more involved. I believe this one should be a fairly straightforward modification of the check for the 'Chromebook added' event, i.e. this function: https://cs.chromium.org/chromium/src/chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.h?q=CheckForExistingUserChromebookAddedEvent+f:chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.h&g=0&l=88 In particular, I don't think this fix will require any information about other Chromebooks whereas I'm not sure if the other fix will. Once I have a CL for crbugs.com/890958 (my top priority ATM), I will work through this carefully and provide an update here.
,
Oct 4
,
Oct 4
As per offline discussions, we want both the 'Chromebook added' and 'host switched' events to only care about verified host. This probably simplifies things a little because we can just change the checks for set hosts to checks for verified hosts on a high level in AccountStatusChangeDelegateNotifierImpl without changing much (if any) lower level code.
,
Oct 5
,
Oct 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8fb52d77b30c8ecee1250d527496bae12de94ec2 commit 8fb52d77b30c8ecee1250d527496bae12de94ec2 Author: Jordy Greenblatt <jordynass@chromium.org> Date: Mon Oct 08 18:45:03 2018 [CrOS MultiDevice] Reserve 'existing user' notifications for verified hosts This CL adapts the MultiDevice notification functionality to the new spec that only considers a new phone added or switched to once it is verified rather than just set. I added tests for this behavior for both of the 'existing host' events and an extra test for the edge case of a set (but unverified) host Phone A is replace by a different host Phone B that is verified. Bug: 891822 Change-Id: Iebd9d0d209b020602a284de200fae168f34759c2 Reviewed-on: https://chromium-review.googlesource.com/c/1263425 Commit-Queue: Jordy Greenblatt <jordynass@chromium.org> Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#597628} [modify] https://crrev.com/8fb52d77b30c8ecee1250d527496bae12de94ec2/chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.cc [modify] https://crrev.com/8fb52d77b30c8ecee1250d527496bae12de94ec2/chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl.h [modify] https://crrev.com/8fb52d77b30c8ecee1250d527496bae12de94ec2/chromeos/services/multidevice_setup/account_status_change_delegate_notifier_impl_unittest.cc
,
Oct 8
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by jlklein@chromium.org
, Oct 3