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

Issue 896324 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Oct 24
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug


Participants' hotlists:
Better-Together-Launch-Blockers


Sign in to add a comment

Instant Tethering item in multi-device subpage does not adhere to policy

Project Member Reported by khorimoto@chromium.org, Oct 17

Issue description

It should show a policy indicator when prohibited, and it should not allow toggling the feature on/off in this state.
 
khorimoto@ and I discussed and decided the best course of action here is just to replace the network-summary-item in our code with a multidevice-feature-item. We already had to factor out all the substantive network logic when we put the network-summary feature in originally so it shouldn't be more than a day of turn around. 
This issue is marked as RBB. Beta is coming very soon. Can this be fixed very soon or some way to mitigate the issue?
Status: Started (was: Assigned)
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.
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.

Jeremy, Jordy, or Kyle: Will one of you update the bug with status update of when we expect to get this done?
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.
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.
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
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).
That's going to miss the next dev (if you merge tomorrow); can we go for beta or are we blocked?
That was written last night; I was referring to 10/23
Ah, cool.  Please tag as merge requested when confirmed.
Owner: khorimoto@chromium.org
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.
Labels: Merge-Request-71
Project Member

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

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.

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.
Labels: -Merge-Request-71 Merge-Approved-71
Approving merge to M71 Chrome OS.

Thanks :-)
Also, can you tag as fixed if verified?
Status: Verified (was: Started)
Project Member

Comment 21 by bugdroid1@chromium.org, Oct 24

Labels: -merge-approved-71 merge-merged-3578
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

Labels: Merge-Merged-71-3578
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