Instant Tethering item in multi-device subpage does not adhere to policy |
||||||||
Issue descriptionIt should show a policy indicator when prohibited, and it should not allow toggling the feature on/off in this state.
,
Oct 18
This issue is marked as RBB. Beta is coming very soon. Can this be fixed very soon or some way to mitigate the issue?
,
Oct 18
Sorry about that - it looks like this bug was never marked as "Started," but there is an in-progress CL to fix this issue: https://chromium-review.googlesource.com/c/chromium/src/+/1287185.
,
Oct 22
Thanks for the reply in #3. Per #2, For M71: Please expedite the review, verify the fix, and get the merge in soon given our Beta schedule. Thanks.
,
Oct 22
Jeremy, Jordy, or Kyle: Will one of you update the bug with status update of when we expect to get this done?
,
Oct 22
I'm mid debug effort for a lingering bug in which the visible network status in both the internet page and the multidevice subpage disagrees with the actual connection state.
,
Oct 22
I'm trying to repro with logs at the key points. I will update when I see if that yields anything useful. That should be complete in 30 mins to an hour as long as we have no further network errors due to the LAX office shuffle.
,
Oct 22
My dev Chromebook is not connecting to my host device on master as well as on my branch so I can't run useful experiments. And I am blocked. I am going to ask colleagues momentarily for help with the connection issue.a
,
Oct 23
Update: I observed the same behavior on my dev Chromebook from the master branch so I'm going to file a separate bug for that and move forward with this change. We should be able to merge it to master tonight or tomorrow morning (Pacific).
,
Oct 23
That's going to miss the next dev (if you merge tomorrow); can we go for beta or are we blocked?
,
Oct 23
That was written last night; I was referring to 10/23
,
Oct 23
Ah, cool. Please tag as merge requested when confirmed.
,
Oct 23
Jordy said he wouldn't be able to complete this task so he asked me to take it off his plate. Working on it now.
,
Oct 24
,
Oct 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6b1ff066b5f169587b4693f0b295745383e10ea2 commit 6b1ff066b5f169587b4693f0b295745383e10ea2 Author: Kyle Horimoto <khorimoto@google.com> Date: Wed Oct 24 01:04:00 2018 [CrOS MultiDevice] Update Instant Tethering item (multi-device subpage). Before this CL, the multi-device settings displayed Instant Tethering settings using exactly the same mechanism used by the network settings. However, this is undesirable because (1) it does not display the "prohibited" icon when a device administrator prohibits the feature and (2) it does not display the correct text as a label for the item. This CL changes the settings subpage to use the same infrastructure as the rest of the features. As a result, this also requires that TetherService be updated; previously, TetherService was responsible for changing the "enabled" user pref itself, but now it needs to support that mode as well as responding to changes of the "enabled" pref. Bug: 896324 , 884830 Change-Id: I083a8e8e07725130058db35ee540dd6ad733d421 Reviewed-on: https://chromium-review.googlesource.com/c/1297594 Reviewed-by: Tommy Li <tommycli@chromium.org> Reviewed-by: Jeremy Klein <jlklein@chromium.org> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#602199} [modify] https://crrev.com/6b1ff066b5f169587b4693f0b295745383e10ea2/chrome/browser/chromeos/tether/tether_service.cc [modify] https://crrev.com/6b1ff066b5f169587b4693f0b295745383e10ea2/chrome/browser/chromeos/tether/tether_service.h [modify] https://crrev.com/6b1ff066b5f169587b4693f0b295745383e10ea2/chrome/browser/chromeos/tether/tether_service_unittest.cc [modify] https://crrev.com/6b1ff066b5f169587b4693f0b295745383e10ea2/chrome/browser/resources/settings/multidevice_page/BUILD.gn [modify] https://crrev.com/6b1ff066b5f169587b4693f0b295745383e10ea2/chrome/browser/resources/settings/multidevice_page/multidevice_feature_behavior.js [modify] https://crrev.com/6b1ff066b5f169587b4693f0b295745383e10ea2/chrome/browser/resources/settings/multidevice_page/multidevice_feature_item.html [modify] https://crrev.com/6b1ff066b5f169587b4693f0b295745383e10ea2/chrome/browser/resources/settings/multidevice_page/multidevice_feature_item.js [modify] https://crrev.com/6b1ff066b5f169587b4693f0b295745383e10ea2/chrome/browser/resources/settings/multidevice_page/multidevice_subpage.html [modify] https://crrev.com/6b1ff066b5f169587b4693f0b295745383e10ea2/chrome/browser/resources/settings/multidevice_page/multidevice_subpage.js [modify] https://crrev.com/6b1ff066b5f169587b4693f0b295745383e10ea2/chrome/browser/resources/settings/multidevice_page/multidevice_tether_item.html [modify] https://crrev.com/6b1ff066b5f169587b4693f0b295745383e10ea2/chrome/browser/resources/settings/multidevice_page/multidevice_tether_item.js [modify] https://crrev.com/6b1ff066b5f169587b4693f0b295745383e10ea2/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
,
Oct 24
This change impacts a number of areas. Can you add context regarding testing on ToT/M72 or otherwise prior to a merge? I'd like to understand the merge risk. Thanks.
,
Oct 24
Manual verification with multi-device flags on: Tested enabling and disabling Instant Tethering via quick settings, Internet settings, and Connected Devices settings. Manual verification with multi-device flags off: Tested enabling and disabling Instant Tethering via quick settings and Internet settings. Also manually verified that changing flags from on to off and back to on does not cause any problem.
,
Oct 24
Approving merge to M71 Chrome OS. Thanks :-)
,
Oct 24
Also, can you tag as fixed if verified?
,
Oct 24
,
Oct 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18 commit bad68c8dd887cab1dcf1efe4a4318c71ac52ed18 Author: Kyle Horimoto <khorimoto@google.com> Date: Wed Oct 24 18:06:34 2018 [CrOS MultiDevice] Update Instant Tethering item (multi-device subpage). Before this CL, the multi-device settings displayed Instant Tethering settings using exactly the same mechanism used by the network settings. However, this is undesirable because (1) it does not display the "prohibited" icon when a device administrator prohibits the feature and (2) it does not display the correct text as a label for the item. This CL changes the settings subpage to use the same infrastructure as the rest of the features. As a result, this also requires that TetherService be updated; previously, TetherService was responsible for changing the "enabled" user pref itself, but now it needs to support that mode as well as responding to changes of the "enabled" pref. Bug: 896324 , 884830 Change-Id: I083a8e8e07725130058db35ee540dd6ad733d421 Reviewed-on: https://chromium-review.googlesource.com/c/1297594 Reviewed-by: Tommy Li <tommycli@chromium.org> Reviewed-by: Jeremy Klein <jlklein@chromium.org> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#602199}(cherry picked from commit 6b1ff066b5f169587b4693f0b295745383e10ea2) Reviewed-on: https://chromium-review.googlesource.com/c/1298293 Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#298} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18/chrome/browser/chromeos/tether/tether_service.cc [modify] https://crrev.com/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18/chrome/browser/chromeos/tether/tether_service.h [modify] https://crrev.com/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18/chrome/browser/chromeos/tether/tether_service_unittest.cc [modify] https://crrev.com/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18/chrome/browser/resources/settings/multidevice_page/BUILD.gn [modify] https://crrev.com/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18/chrome/browser/resources/settings/multidevice_page/multidevice_feature_behavior.js [modify] https://crrev.com/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18/chrome/browser/resources/settings/multidevice_page/multidevice_feature_item.html [modify] https://crrev.com/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18/chrome/browser/resources/settings/multidevice_page/multidevice_feature_item.js [modify] https://crrev.com/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18/chrome/browser/resources/settings/multidevice_page/multidevice_subpage.html [modify] https://crrev.com/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18/chrome/browser/resources/settings/multidevice_page/multidevice_subpage.js [modify] https://crrev.com/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18/chrome/browser/resources/settings/multidevice_page/multidevice_tether_item.html [modify] https://crrev.com/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18/chrome/browser/resources/settings/multidevice_page/multidevice_tether_item.js [modify] https://crrev.com/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18/chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
,
Oct 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bad68c8dd887cab1dcf1efe4a4318c71ac52ed18 Commit: bad68c8dd887cab1dcf1efe4a4318c71ac52ed18 Author: khorimoto@google.com Commiter: khorimoto@chromium.org Date: 2018-10-24 18:06:34 +0000 UTC [CrOS MultiDevice] Update Instant Tethering item (multi-device subpage). Before this CL, the multi-device settings displayed Instant Tethering settings using exactly the same mechanism used by the network settings. However, this is undesirable because (1) it does not display the "prohibited" icon when a device administrator prohibits the feature and (2) it does not display the correct text as a label for the item. This CL changes the settings subpage to use the same infrastructure as the rest of the features. As a result, this also requires that TetherService be updated; previously, TetherService was responsible for changing the "enabled" user pref itself, but now it needs to support that mode as well as responding to changes of the "enabled" pref. Bug: 896324 , 884830 Change-Id: I083a8e8e07725130058db35ee540dd6ad733d421 Reviewed-on: https://chromium-review.googlesource.com/c/1297594 Reviewed-by: Tommy Li <tommycli@chromium.org> Reviewed-by: Jeremy Klein <jlklein@chromium.org> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#602199}(cherry picked from commit 6b1ff066b5f169587b4693f0b295745383e10ea2) Reviewed-on: https://chromium-review.googlesource.com/c/1298293 Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3578@{#298} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by jordynass@chromium.org
, Oct 17