Fix #18152: Cast should be disabled on toggling off media router switch in extension settings #10116

jumde · 2021-09-16T04:31:28Z

Resolves brave/brave-browser#18152

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Desktop

Enable network filter for little snitch
Using a new profile, open Brave Browser
Navigate to brave://settings/extensions and verify that cast media router is not enabled by default
Verify that SSDP requests are not sent by default
Navigate to brave://settings/extensions and toggle cast switch
Restart button should be visible
Redirect to brave://flags and navigate back to brave://settings/extensions. Verify the restart button is still visible
Restart browser
Verify cast works
Navigate to brave://settings/extensions - Disable cast - Restart
Verify the cast no longer works.

Android

Verify that cast works on android by default

mariospr

LGTM

SergeyZhukovsky

++

bridiver · 2021-09-21T16:15:46Z

browser/brave_profile_prefs.cc

@@ -409,6 +409,8 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {

  registry->SetDefaultPrefValue(prefs::kEnableMediaRouter, base::Value(false));

+  registry->RegisterBooleanPref(kBraveMediaRouter, false);


I don't understand why we're adding a new pref and how is this different from prefs::kEnableMediaRouter?

Cast extension requires a browser restart once the setting is toggled. We use the pref kBraveMediaRouter as a proxy to identify the current state of the switch and prefs::kEnableMediaRouter is updated to kBraveMediaRouter on browser start.

I thought we already had something like this?

bridiver · 2021-09-21T16:16:12Z

common/pref_names.h

@@ -92,6 +92,7 @@ extern const char kSafetynetStatus[];
 extern const char kDefaultBrowserLaunchingCount[];
 extern const char kTabsSearchShow[];
 extern const char kDontAskForCrashReporting[];
+extern const char kBraveMediaRouter[];


see above these prefs need comments explaining them

comment added: https://github.com/brave/brave-core/pull/10116/files#diff-f455d9b18791c2fce32eb6e88efd157bfe6b73c7ed50feebdcc7a5ceeb9ecfcbR99

bridiver · 2021-09-21T16:17:05Z

chromium_src/chrome/browser/media/router/media_router_feature.cc

@@ -5,6 +5,7 @@

 #include "chrome/browser/media/router/media_router_feature.h"

+#include "brave/common/pref_names.h"


this doesn't appear to be used

bridiver · 2021-09-21T16:19:21Z

chromium_src/chrome/browser/ui/media_router/media_router_ui_service_factory.cc

+
+KeyedService* MediaRouterUIServiceFactory::BuildServiceInstanceFor(
+    BrowserContext* context) const {
+#if !defined(OS_ANDROID)


this also needs a comment because I don't understand why we're doing this or why it's happening here because this definitely doesn't seem like the right place. If kEnableMediaRouter needs to be kept in sync with kBraveMediaRouter, there should be a pref observer for it

Comment added: https://github.com/brave/brave-core/pull/10116/files#diff-c1bca8803e527698720c7fb05fa895bbcc5537f9accbf845c48b2709cdecd26eR25

bridiver · 2021-09-21T16:56:52Z

common/pref_names.cc

+// kBraveMediaRouter is used as a proxy to identify the current state of the
+// switch and prefs::kEnableMediaRouter is updated to kBraveMediaRouter
+// on restart.
+const char kBraveMediaRouter[] = "brave.media_router";


this naming is confusing imo. It makes it sound like we have our own media router. If this pref is needed (not sure why we can't just restart when changing kEnableMediaRouter) it should be renamed to something like kEnableMediaRouterOnRestart

also the comment should go in the .h file, not the .cc file

bridiver · 2021-09-21T16:59:29Z

chromium_src/chrome/browser/ui/media_router/media_router_ui_service_factory.cc

+
+KeyedService* MediaRouterUIServiceFactory::BuildServiceInstanceFor(
+    BrowserContext* context) const {
+// Chromecast is enabled by default on Android.


I think this should go in BraveProfileManager::InitProfileUserPrefs

chromium_src/chrome/browser/media/router/media_router_feature.cc

bridiver · 2021-09-21T17:55:50Z

chromium_src/chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc

@@ -366,11 +368,15 @@ void BraveAddAboutStrings(content::WebUIDataSource* html_source,
  html_source->AddString("aboutProductLicense", license);
 }

-void BraveAddSocialBlockingLoadTimeData(content::WebUIDataSource* html_source,
-                                        Profile* profile) {
+void BraveAddExtensionSettingsLoadTimeData(


the old name didn't make sense, but neither does this one and I don't think this needs to be a method anyway, just put them directly in BraveAddLocalizedStrings

bridiver · 2021-09-21T21:04:05Z

browser/profiles/brave_profile_manager.cc

+// on restart
+#if !defined(OS_ANDROID)
+  auto* pref_service = profile->GetPrefs();
+  auto enabled = pref_service->GetBoolean(kEnableMediaRouterOnRestart);


you need to check IsDefaultValue or this will disable for everyone

bridiver · 2021-09-29T17:26:49Z

common/pref_names.cc

@@ -101,6 +101,13 @@ const char kMRUCyclingEnabled[] = "brave.mru_cycling_enabled";
 const char kTabsSearchShow[] = "brave.tabs_search_show";
 const char kDontAskForCrashReporting[] = "brave.dont_ask_for_crash_reporting";

+// Cast extension requires a browser restart once the setting is toggled.


the comment should be in the .h file

bridiver · 2021-09-29T17:28:04Z

chromium_src/chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc

+      "signInAllowedOnNextStartupInitialValue",
+      profile->GetPrefs()->GetBoolean(prefs::kSigninAllowedOnNextStartup));
+
+  html_source->AddBoolean("mediaRouterEnabledInitialValue",


the ts/js naming convention should match c++ to avoid confusion. isMediaRouterEnabled

bridiver · 2021-09-29T17:28:38Z

...r/resources/settings/brave_default_extensions_page/brave_default_extensions_browser_proxy.js

@@ -21,6 +21,7 @@ cr.define('settings', function () {
    isWidevineEnabled() {}
    getRestartNeeded () {}
    wasSignInEnabledAtStartup () {}
+    wasMediaRouterEnabledAtStartup () {}


same as the pref isMediaRouterEnabled fully captures what this value is and is consistent with c++

…ch in extension settings

Fix #18152: Cast should be disabled on toggling off media router switch in extension settings

kjozwiak · 2021-10-08T07:22:14Z

Verification PASSED on Win 11 x64 using the following build:

Brave | 1.32.56 Chromium: 95.0.4638.40 (Official Build) nightly (64-bit)
-- | --
Revision | e3e7c76ba0284b16087cf4cf3153abfaef6470c7-refs/branch-heads/4638@{#624}
OS | Windows 11 Version 21H2 (Build 22000.194)

ensured that Media Router is disabled by default via brave://settings/extensions
ensured that there's no SSDP packets being sent while Media Router is disabled

`Disabled`	`Enabled`

ensured that there's a Relaunch button once Media Router has been enabled
ensured that switching between brave://settings/extensions & brave://flags doesn't remove the Relaunch button

ensured that Cast... appears under the Hamburger Menu when Media Router is enabled
ensured that Cast.... is removed under the Hamburger Menu when Media Router has been disabled
ensured that you cannot cast nor see any SSDP packets after disabling Media Router via brave://settings/extensions

`Casting Tab/YouTube`

`Example`	`Example`

`Casting Desktop`

`Example`	`Example`

`Casting Local File`

`Example`	`Example`

Verification PASSED on Samsung S10+ running Android 11 using the following build:

Brave | 1.32.56 Chromium: 95.0.4638.40

ensured that Media Router was enabled by default by visiting https://gem.cbc.ca/live/channel/ottawa and ensuring that the Cast icon was being displayed via the media player
ensured that tapping on the Cast button displayed a modal with all the devices that can cast

`Example`	`Example`	`Example`	`Example`

jumde requested a review from a team as a code owner September 16, 2021 04:31

github-actions bot assigned jumde Sep 16, 2021

mariospr approved these changes Sep 16, 2021

View reviewed changes

SergeyZhukovsky approved these changes Sep 16, 2021

View reviewed changes

jumde force-pushed the cast_disable branch from d0f1ca0 to 10a4793 Compare September 17, 2021 18:44

jumde requested a review from a team as a code owner September 17, 2021 18:44

jumde force-pushed the cast_disable branch 2 times, most recently from 05b9008 to 39409da Compare September 17, 2021 20:19

bridiver reviewed Sep 21, 2021

View reviewed changes

chromium_src/chrome/browser/media/router/media_router_feature.cc Show resolved Hide resolved

bridiver reviewed Sep 21, 2021

View reviewed changes

jumde force-pushed the cast_disable branch from a4b0b5a to 1feca6b Compare September 21, 2021 20:51

bridiver reviewed Sep 21, 2021

View reviewed changes

jumde force-pushed the cast_disable branch 8 times, most recently from 32bcea1 to 92b0031 Compare September 29, 2021 00:40

jumde requested a review from bridiver September 29, 2021 02:07

jumde force-pushed the cast_disable branch from 92b0031 to c104662 Compare September 29, 2021 14:55

bridiver reviewed Sep 29, 2021

View reviewed changes

jumde force-pushed the cast_disable branch from c104662 to e09311f Compare September 30, 2021 17:19

jumde requested a review from bridiver September 30, 2021 17:54

bridiver approved these changes Oct 1, 2021

View reviewed changes

jumde mentioned this pull request Oct 2, 2021

Cast is not disabled on toggling off media router switch in extension settings brave/brave-browser#18152

Closed

Fix #18152: Cast should be disabled on toggling off media router swit…

01403f0

…ch in extension settings

jumde force-pushed the cast_disable branch from e09311f to 01403f0 Compare October 2, 2021 20:08

jumde merged commit 11701aa into master Oct 3, 2021

jumde deleted the cast_disable branch October 3, 2021 22:34

jumde added this to the 1.32.x - Nightly milestone Oct 3, 2021

jumde added a commit that referenced this pull request Oct 4, 2021

Merge pull request #10116 from brave/cast_disable

37af820

Fix #18152: Cast should be disabled on toggling off media router switch in extension settings

jumde mentioned this pull request Oct 4, 2021

Fix #18152: Cast should be disabled on toggling off media router switch in extension settings (uplift to 1.31.x) #10350

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #18152: Cast should be disabled on toggling off media router switch in extension settings #10116

Fix #18152: Cast should be disabled on toggling off media router switch in extension settings #10116

jumde commented Sep 16, 2021 •

edited

Loading

mariospr left a comment

SergeyZhukovsky left a comment

bridiver Sep 21, 2021

jumde Sep 21, 2021

bridiver Sep 21, 2021

bridiver Sep 21, 2021

jumde Sep 21, 2021

bridiver Sep 21, 2021

jumde Sep 21, 2021

bridiver Sep 21, 2021

jumde Sep 21, 2021

bridiver Sep 21, 2021 •

edited

Loading

bridiver Sep 21, 2021

jumde Sep 21, 2021

bridiver Sep 21, 2021

jumde Sep 21, 2021

bridiver Sep 21, 2021

jumde Sep 21, 2021

bridiver Sep 21, 2021

jumde Sep 30, 2021

bridiver Sep 29, 2021

jumde Sep 30, 2021

bridiver Sep 29, 2021

jumde Sep 30, 2021

bridiver Sep 29, 2021

kjozwiak commented Oct 8, 2021 •

edited

Loading

		@@ -409,6 +409,8 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {

		registry->SetDefaultPrefValue(prefs::kEnableMediaRouter, base::Value(false));

		registry->RegisterBooleanPref(kBraveMediaRouter, false);

		@@ -5,6 +5,7 @@

		#include "chrome/browser/media/router/media_router_feature.h"

		#include "brave/common/pref_names.h"

Fix #18152: Cast should be disabled on toggling off media router switch in extension settings #10116

Fix #18152: Cast should be disabled on toggling off media router switch in extension settings #10116

Conversation

jumde commented Sep 16, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Desktop

Android

mariospr left a comment

Choose a reason for hiding this comment

SergeyZhukovsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjozwiak commented Oct 8, 2021 • edited Loading

Casting Tab/YouTube

Casting Desktop

Casting Local File

jumde commented Sep 16, 2021 •

edited

Loading

bridiver Sep 21, 2021 •

edited

Loading

kjozwiak commented Oct 8, 2021 •

edited

Loading

`Casting Tab/YouTube`

`Casting Desktop`

`Casting Local File`