TetherService allows Magic Tether even when feature is supported by not enabled |
||||||||
Issue descriptionIn TetherService (see [1]), we grab the HostStatus from MultiDeviceSetupClient and check the Instant Tethering preference; if the host is verified and the preference is enabled, we move forward. However, this does not check whether the phone has set the feature to the "enabled" state. Thus, we should change this check to use MultiDeviceSetupClient::GetFeatureStates(). If the state for Instant Tethering is kEnabledByUser, we should continue; otherwise, we should not. There is no need to check the preference or the host status, as the feature state encapsulates both of those internally. [1] https://cs.chromium.org/chromium/src/chrome/browser/chromeos/tether/tether_service.h
,
Aug 18
That's actually not quite good enough. If the phone has the feature enabled and the Instant Tethering preference is enabled *but* the Better Together suite's preference is disabled, we shouldn't be initializing the component.
,
Aug 28
,
Aug 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7bddbb1336df98bcb11941ea2725e4466e7a60ab commit 7bddbb1336df98bcb11941ea2725e4466e7a60ab Author: Kyle Horimoto <khorimoto@google.com> Date: Fri Aug 31 01:15:42 2018 [CrOS MultiDevice] Only start up Instant Tethering when enabled. This CL updates TetherService and TetherHostFetcher to use the MultiDeviceSetupClient's GetFeatureState() function to determine whether the feature should be active or not. This fixes a few edge cases: (1) If the Instant Tethering user preference is enabled, but the Better Together suite is disabled, we should not start up the feature. (2) If the preference is enabled, but the host device does not support Instant Tethering, we should not start up the feature. This CL eliminates the MULTIDEVICE_HOST_UNVERIFIED metrics enum since an unverified host results in NO_AVAILABLE_HOSTS; it replaces that enum value with BETTER_TOGETHER_SUITE_DISABLED, which can occur in practice. Note that because the old metric value has not yet been pushed to users, it is okay to replace it with a new value instead of deprecating it. Bug: 875502 , 824568 Change-Id: Ia85f0ba31871c94ed9eb0cb2ef3c4699bb0000dd Reviewed-on: https://chromium-review.googlesource.com/1196172 Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#587894} [modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chrome/browser/chromeos/tether/tether_service.cc [modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chrome/browser/chromeos/tether/tether_service.h [modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chrome/browser/chromeos/tether/tether_service_unittest.cc [modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chromeos/components/tether/tether_component.h [modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chromeos/components/tether/tether_component_impl.cc [modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chromeos/components/tether/tether_host_fetcher_impl.cc [modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chromeos/components/tether/tether_host_fetcher_impl.h [modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chromeos/components/tether/tether_session_completion_logger.h [modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/chromeos/components/tether/tether_session_completion_logger_unittest.cc [modify] https://crrev.com/7bddbb1336df98bcb11941ea2725e4466e7a60ab/tools/metrics/histograms/enums.xml
,
Aug 31
,
Aug 31
Re comment 1: "There is no need to check the preference or the host status, as the feature state encapsulates both of those internally." This sounds potentially fragile, i.e., this may break if that contract changes in the future. Am I missing something?
,
Aug 31
Re comment 2: "If the phone has the feature enabled and the Instant Tethering preference is enabled *but* the Better Together suite's preference is disabled" How does this case happen? I think I'm getting confused by all of the different toggles in the multidevice world (and that causes me some concern).
,
Aug 31
Re: comment #6: The thing you're missing is that we still need to support both Instant Tethering and EasyUnlock with the MultiDevice flags turned *off*. Thus, we still need a way for the features to talk to the prefs directly in that case, but when the flag is on, these features should talk to MultiDeviceSetup service. Re: comment #7: This is how this situation happens: (1) Start with all toggles on, including both the Better Together suite and Instant Tethering. (2) Turn the Better Together suite toggle off, but do not touch the Instant Tethering toggle (it should still be on). (3) Instant Tethering should not be active, but its toggle is still on. Please read through go/cros-multidevice-prefs if you have more questions.
,
Aug 31
Looks like this CL didn't make it into M-70; merge requested please!
,
Sep 1
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 5
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 5
,
Sep 5
Merged: https://chromium-review.googlesource.com/c/chromium/src/+/1208139
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8665b5bb5f08911931fc5972311c509bc803323 commit d8665b5bb5f08911931fc5972311c509bc803323 Author: Ryan Hansberry <hansberry@chromium.org> Date: Wed Sep 05 21:51:21 2018 [CrOS MultiDevice] Only start up Instant Tethering when enabled. This CL updates TetherService and TetherHostFetcher to use the MultiDeviceSetupClient's GetFeatureState() function to determine whether the feature should be active or not. This fixes a few edge cases: (1) If the Instant Tethering user preference is enabled, but the Better Together suite is disabled, we should not start up the feature. (2) If the preference is enabled, but the host device does not support Instant Tethering, we should not start up the feature. This CL eliminates the MULTIDEVICE_HOST_UNVERIFIED metrics enum since an unverified host results in NO_AVAILABLE_HOSTS; it replaces that enum value with BETTER_TOGETHER_SUITE_DISABLED, which can occur in practice. Note that because the old metric value has not yet been pushed to users, it is okay to replace it with a new value instead of deprecating it. TBR=khorimoto@google.com (cherry picked from commit 7bddbb1336df98bcb11941ea2725e4466e7a60ab) Bug: 875502 , 824568 Change-Id: Ia85f0ba31871c94ed9eb0cb2ef3c4699bb0000dd Reviewed-on: https://chromium-review.googlesource.com/1196172 Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#587894} Reviewed-on: https://chromium-review.googlesource.com/1208139 Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#68} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chrome/browser/chromeos/tether/tether_service.cc [modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chrome/browser/chromeos/tether/tether_service.h [modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chrome/browser/chromeos/tether/tether_service_unittest.cc [modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chromeos/components/tether/tether_component.h [modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chromeos/components/tether/tether_component_impl.cc [modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chromeos/components/tether/tether_host_fetcher_impl.cc [modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chromeos/components/tether/tether_host_fetcher_impl.h [modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chromeos/components/tether/tether_session_completion_logger.h [modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/chromeos/components/tether/tether_session_completion_logger_unittest.cc [modify] https://crrev.com/d8665b5bb5f08911931fc5972311c509bc803323/tools/metrics/histograms/enums.xml |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by jlklein@chromium.org
, Aug 17