MediaRemoter shouldn't be created if MediaRouter is disabled |
|||||||
Issue descriptionWhen MediaRouter is disabled by an admin policy, it shouldn't be instantiated. So MediaRemoter that depends on MediaRouter should also not be created. The enabled/disabled state can be checked here [1]. MediaRemoter uses MediaRouter here [2]. Xiangjun, could you take a look? [1] https://cs.chromium.org/chromium/src/chrome/browser/media/router/media_router_feature.h?sq=package:chromium&dr=C&l=20 [2] https://cs.chromium.org/chromium/src/chrome/browser/media/cast_remoting_connector.cc?l=136
,
Apr 26 2018
,
Apr 26 2018
Requesting to merge the fix to M67. Very low risk.
,
Apr 26 2018
Before we approve merge to M67, please answer followings: * Is this M67 regression? Is it critical? * Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M67? * Any other important details to justify the merge. Please note M67 is already in Beta, so merge bar is very high. Thank you.
,
Apr 26 2018
+takumif@: Can you please address questions in #4? The fix was in yesterday. I am fine to wait for several other days to allow it baked in Canary, though the risk is pretty low.
,
Apr 26 2018
The regression is in M66. The Cast feature may send network requests despite being disabled by an admin policy (the feature itself would still be nonfunctional), so we'd like to fix that for M67. As seen in issue 834719 , there was a request from an enterprise user. The Media Router feature is covered by various automated and manual tests.
,
Apr 26 2018
Also Media Router feature is behind finch?
,
Apr 26 2018
The Media Router / Cast feature was enabled long time ago via Finch (M51), and currently can only be turned off by an administrator policy.
,
Apr 26 2018
Is CL listed at #1 looking good in canary? Or we can wait until more canary coverage?
,
Apr 27 2018
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 27 2018
This fix currently looks good in canary and the merge has very low risk. takumif@: Do we need to wait for more days to verify before merging?
,
Apr 27 2018
Given it's looking good in Canary and we have automated and manual test coverage, it should be good for merging.
,
Apr 27 2018
Approving merge for CL listed at #1 to M67 branch 3396 based on comments #6, #11 and #12. Pls merge ASAP. Thank you.
,
Apr 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/77d8bad685f4df01dde5634c820399392b586c1f commit 77d8bad685f4df01dde5634c820399392b586c1f Author: Xiangjun Zhang <xjz@chromium.org> Date: Fri Apr 27 20:52:13 2018 [Merge to M67] Disable CastRemotingConnector if MediaRouter is disabled. CastRemotingConnector depends on MediaRouter, which should not be created when MediaRouter is disabled by an admin policy. Bug: 836042 TBR=amp@chromium.org, miu@chromium.org, xjz@chromium.org (cherry picked from commit 8d98af56ca3e0b7d461ca2c8c6e3794031879f64) Change-Id: Ibd03b5f7c0cb875173cb8c6c573f34a12f0e22b4 Reviewed-on: https://chromium-review.googlesource.com/1026252 Reviewed-by: Adam Parker <amp@chromium.org> Reviewed-by: Yuri Wiitala <miu@chromium.org> Commit-Queue: Yuri Wiitala <miu@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#553932} Reviewed-on: https://chromium-review.googlesource.com/1033719 Reviewed-by: Xiangjun Zhang <xjz@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#358} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/77d8bad685f4df01dde5634c820399392b586c1f/chrome/browser/media/cast_remoting_connector.cc |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by bugdroid1@chromium.org
, Apr 26 2018