Skip to content

Commit

Permalink
Revert "Enable GlobalMediaControlsForCast and CastMediaRouteProvider …
Browse files Browse the repository at this point in the history
…by default"

This reverts commit ba96414.

Reason for revert: failing unit_tests on linux-chromeos-chrome with NOTREACHED(). Failing tests:
MediaToolbarButtonControllerTest.DoesNotDisableButtonIfDialogIsOpen
MediaToolbarButtonControllerTest.DoesNotHideIfMediaStartsPlayingWithinTimeout
MediaToolbarButtonControllerTest.HidesAfterTimeoutAndShowsAgainOnPlay

Example:
 RUN      ] MediaToolbarButtonControllerTest.HidesAfterTimeoutAndShowsAgainOnPlay
2021-02-03T04:04:47.546579Z 564170770 ERROR unit_tests[2535:2535]: [pref_change_registrar.cc(40)] NOTREACHED() hit.
Received signal 11 SEGV_MAPERR 000000000038
#0 0x5611b0dbfe29 base::debug::CollectStackTrace()
#1 0x5611b0d2f4d3 base::debug::StackTrace::StackTrace()
#2 0x5611b0dbf9d1 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7f9eb128f390 (/lib/x86_64-linux-gnu/libpthread-2.23.so+0x1138f)
#4 0x5611ac59c5fe std::__1::__hash_table<>::find<>()
#5 0x5611b1c25762 PrefService::FindPreference()
#6 0x5611b11a2169 media_router::GetCastAllowAllIPsPref()
#7 0x5611b2fd159b media_router::CastMediaSinkService::CreateImpl()
#8 0x5611b2fd138f media_router::CastMediaSinkService::Start()
#9 0x5611b2fad704 media_router::DualMediaSinkService::DualMediaSinkService()
#10 0x5611b2fad539 media_router::DualMediaSinkService::GetInstance()
#11 0x5611b2f93b44 media_router::MediaRouterDesktop::MediaRouterDesktop()
#12 0x5611b2f93030 media_router::ChromeMediaRouterFactory::BuildServiceInstanceFor()
#13 0x5611b2bdb30f BrowserContextKeyedServiceFactory::BuildServiceInstanceFor()
#14 0x5611b1cd2837 KeyedServiceFactory::GetServiceForContext()
#15 0x5611b37297b6 CastMediaNotificationProvider::CastMediaNotificationProvider()
#16 0x5611b372c274 MediaNotificationService::MediaNotificationService()
#17 0x5611abdba814 MediaToolbarButtonControllerTest::SetUp()

first failing build:
https://ci.chromium.org/ui/p/chrome/builders/ci/linux-chromeos-chrome/11825/overview

Original change's description:
> Enable GlobalMediaControlsForCast and CastMediaRouteProvider by default
>
> The features were enabled via a config change in M87 (CastMRP on CrOS
> will soon be enabled in M88). This change enables them by default.
>
> Bug: 1042330, 869214
> Change-Id: I111bd1f6d13bf70cf7557004205e7de1dc3e23ec
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2631191
> Reviewed-by: Peter Kasting <[email protected]>
> Reviewed-by: John Williams <[email protected]>
> Reviewed-by: Mounir Lamouri <[email protected]>
> Commit-Queue: Takumi Fujimoto <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#849910}

[email protected],[email protected],[email protected],[email protected],[email protected]

Change-Id: I215a231257e91dd81348577221892fb6b297677d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1042330
Bug: 869214
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2671781
Reviewed-by: Olga Sharonova <[email protected]>
Commit-Queue: Olga Sharonova <[email protected]>
Cr-Commit-Position: refs/heads/master@{#850027}
  • Loading branch information
Olga Sharonova authored and Chromium LUCI CQ committed Feb 3, 2021
1 parent 6be3dbb commit 001ba4e
Show file tree
Hide file tree
Showing 7 changed files with 25 additions and 49 deletions.
7 changes: 6 additions & 1 deletion chrome/browser/media/router/media_router_feature.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,18 @@
namespace media_router {

#if !defined(OS_ANDROID)
#if !defined(OFFICIAL_BUILD)
// Enables the media router. Can be useful to disable for local
// development on Mac because DIAL local discovery opens a local port
// and triggers a permission prompt. Only toggleable for developer builds.
const base::Feature kMediaRouter{"MediaRouter",
base::FEATURE_ENABLED_BY_DEFAULT};
#endif // !defined(OFFICIAL_BUILD)
// Controls if browser side DialMediaRouteProvider is enabled.
const base::Feature kDialMediaRouteProvider{"DialMediaRouteProvider",
base::FEATURE_ENABLED_BY_DEFAULT};
const base::Feature kCastMediaRouteProvider{"CastMediaRouteProvider",
base::FEATURE_ENABLED_BY_DEFAULT};
base::FEATURE_DISABLED_BY_DEFAULT};
const base::Feature kCastAllowAllIPsFeature{"CastAllowAllIPs",
base::FEATURE_DISABLED_BY_DEFAULT};
const base::Feature kGlobalMediaControlsCastStartStop{
Expand Down
7 changes: 0 additions & 7 deletions chrome/browser/media/router/media_router_feature.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,11 @@ bool MediaRouterEnabled(content::BrowserContext* context);

#if !defined(OS_ANDROID)

// Enables the media router. Can be disabled in tests unrelated to
// Media Router where it interferes. Can also be useful to disable for local
// development on Mac because DIAL local discovery opens a local port
// and triggers a permission prompt.
extern const base::Feature kMediaRouter;

// TODO(crbug.com/1028753): Remove default-enabled kDialMediaRouteProvider after
// tests stop disabling it.
extern const base::Feature kDialMediaRouteProvider;

extern const base::Feature kCastMediaRouteProvider;

// If enabled, allows Media Router to connect to Cast devices on all IP
// addresses, not just RFC1918/RFC4193 private addresses. Workaround for
// https://crbug.com/813974.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,11 @@ class MediaRouterDesktopTest : public MediaRouterMojoTest {
protected:
std::unique_ptr<MediaRouterMojoImpl> CreateMediaRouter() override {
std::unique_ptr<MockCastMediaSinkService> cast_media_sink_service;
// We disable the DIAL and Cast MRPs because initializing the MRPs requires
// initialization of objects they depend on, which is outside the scope of
// this unit test. MRP initialization is covered by Media Router browser
// tests.
feature_list_.InitWithFeatures(
{}, /* disabled_features */ {kDialMediaRouteProvider,
kCastMediaRouteProvider});
// We disable the DIAL MRP because initializing the DIAL MRP requires
// initialization of objects it depends on, which is outside the scope of
// this unit test. DIAL MRP initialization is covered by Media Router
// browser tests.
feature_list_.InitAndDisableFeature(kDialMediaRouteProvider);
cast_media_sink_service = std::make_unique<MockCastMediaSinkService>();
cast_media_sink_service_ = cast_media_sink_service.get();
media_sink_service_ =
Expand Down
12 changes: 2 additions & 10 deletions chrome/browser/ui/ash/media_notification_provider_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#include "chrome/browser/ui/ash/media_notification_provider_impl.h"

#include "ash/public/cpp/media_notification_provider_observer.h"
#include "base/test/scoped_feature_list.h"
#include "base/unguessable_token.h"
#include "chrome/browser/chromeos/login/users/fake_chrome_user_manager.h"
#include "chrome/browser/chromeos/profiles/profile_helper.h"
Expand All @@ -16,7 +15,6 @@
#include "chrome/test/base/testing_profile_manager.h"
#include "components/session_manager/core/session_manager.h"
#include "content/public/test/browser_task_environment.h"
#include "media/base/media_switches.h"
#include "services/media_session/public/mojom/audio_focus.mojom.h"
#include "services/media_session/public/mojom/media_session.mojom.h"
#include "testing/gmock/include/gmock/gmock.h"
Expand Down Expand Up @@ -53,11 +51,6 @@ class MediaNotificationProviderImplTest : public testing::Test {

void SetUp() override {
testing::Test::SetUp();

// Disable a feature unrelated to the unit test. The use of cast features
// requires setting up extra dependencies.
feature_list_.InitAndDisableFeature(media::kGlobalMediaControlsForCast);

user_manager_->Initialize();
CHECK(testing_profile_manager_.SetUp());

Expand Down Expand Up @@ -114,16 +107,15 @@ class MediaNotificationProviderImplTest : public testing::Test {

MediaNotificationProviderImpl* provider() { return provider_.get(); }

content::BrowserTaskEnvironment browser_environment_;
content::BrowserTaskEnvironment browser_environment;

private:
session_manager::SessionManager session_manager_;
chromeos::FakeChromeUserManager* user_manager_{
new chromeos::FakeChromeUserManager()};
TestingProfileManager testing_profile_manager_{
TestingBrowserProcess::GetGlobal()};
views::LayoutProvider layout_provider_;
base::test::ScopedFeatureList feature_list_;
views::LayoutProvider layout_provider;

std::unique_ptr<MockMediaNotificationProviderObserver> mock_observer_;
std::unique_ptr<MediaNotificationProviderImpl> provider_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@
#include <memory>

#include "base/strings/utf_string_conversions.h"
#include "base/test/scoped_feature_list.h"
#include "base/unguessable_token.h"
#include "chrome/browser/media/router/media_router_feature.h"
#include "chrome/browser/ui/global_media_controls/media_dialog_delegate.h"
#include "chrome/browser/ui/global_media_controls/media_notification_service.h"
#include "chrome/browser/ui/global_media_controls/media_toolbar_button_controller_delegate.h"
Expand Down Expand Up @@ -89,15 +87,13 @@ class MediaToolbarButtonControllerTest : public testing::Test {
public:
MediaToolbarButtonControllerTest()
: task_environment_(base::test::TaskEnvironment::TimeSource::MOCK_TIME,
base::test::TaskEnvironment::MainThreadType::UI) {}
base::test::TaskEnvironment::MainThreadType::UI),
service_(&profile_, false) {}
~MediaToolbarButtonControllerTest() override = default;

void SetUp() override {
// Disable Media Router, which has many dependencies of its own.
feature_list_.InitAndDisableFeature(media_router::kMediaRouter);
service_ = std::make_unique<MediaNotificationService>(&profile_, false);
controller_ = std::make_unique<MediaToolbarButtonController>(
&delegate_, service_.get());
controller_ =
std::make_unique<MediaToolbarButtonController>(&delegate_, &service_);
}

void TearDown() override { controller_.reset(); }
Expand Down Expand Up @@ -128,13 +124,13 @@ class MediaToolbarButtonControllerTest : public testing::Test {

void SimulateFocusGained(const base::UnguessableToken& id,
bool controllable) {
service_->OnFocusGained(CreateFocusRequest(id, controllable));
service_.OnFocusGained(CreateFocusRequest(id, controllable));
}

void SimulateFocusLost(const base::UnguessableToken& id) {
AudioFocusRequestStatePtr focus(AudioFocusRequestState::New());
focus->request_id = id;
service_->OnFocusLost(std::move(focus));
service_.OnFocusLost(std::move(focus));
}

void SimulateNecessaryMetadata(const base::UnguessableToken& id) {
Expand All @@ -144,8 +140,8 @@ class MediaToolbarButtonControllerTest : public testing::Test {
// service, but since the service doesn't run for this test, we'll manually
// grab the MediaNotificationItem from the MediaNotificationService and
// set the metadata.
auto item_itr = service_->sessions_.find(id.ToString());
ASSERT_NE(service_->sessions_.end(), item_itr);
auto item_itr = service_.sessions_.find(id.ToString());
ASSERT_NE(service_.sessions_.end(), item_itr);

media_session::MediaMetadata metadata;
metadata.title = base::ASCIIToUTF16("title");
Expand All @@ -154,7 +150,7 @@ class MediaToolbarButtonControllerTest : public testing::Test {
}

void SimulateDialogOpened(MockMediaDialogDelegate* delegate) {
delegate->Open(service_.get());
delegate->Open(&service_);
}

MockMediaToolbarButtonControllerDelegate& delegate() { return delegate_; }
Expand All @@ -163,9 +159,8 @@ class MediaToolbarButtonControllerTest : public testing::Test {
content::BrowserTaskEnvironment task_environment_;
MockMediaToolbarButtonControllerDelegate delegate_;
TestingProfile profile_;
std::unique_ptr<MediaNotificationService> service_;
MediaNotificationService service_;
std::unique_ptr<MediaToolbarButtonController> controller_;
base::test::ScopedFeatureList feature_list_;

DISALLOW_COPY_AND_ASSIGN(MediaToolbarButtonControllerTest);
};
Expand Down
9 changes: 1 addition & 8 deletions chrome/browser/ui/views/frame/test_with_browser_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
#include <memory>

#include "base/macros.h"
#include "base/test/scoped_feature_list.h"
#include "chrome/browser/media/router/media_router_feature.h"
#include "chrome/test/base/browser_with_test_window_test.h"

class BrowserView;
Expand All @@ -20,11 +18,7 @@ class BrowserView;
class TestWithBrowserView : public BrowserWithTestWindowTest {
public:
template <typename... Args>
explicit TestWithBrowserView(Args... args)
: BrowserWithTestWindowTest(args...) {
// Media Router requires the IO thread, which doesn't exist in this setup.
feature_list_.InitAndDisableFeature(media_router::kMediaRouter);
}
TestWithBrowserView(Args... args) : BrowserWithTestWindowTest(args...) {}

~TestWithBrowserView() override;

Expand All @@ -39,7 +33,6 @@ class TestWithBrowserView : public BrowserWithTestWindowTest {

private:
BrowserView* browser_view_; // Not owned.
base::test::ScopedFeatureList feature_list_;

DISALLOW_COPY_AND_ASSIGN(TestWithBrowserView);
};
Expand Down
2 changes: 1 addition & 1 deletion media/base/media_switches.cc
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,7 @@ const base::Feature kGlobalMediaControlsAutoDismiss{
// Show Cast sessions in Global Media Controls. It is no-op if
// kGlobalMediaControls is not enabled.
const base::Feature kGlobalMediaControlsForCast{
"GlobalMediaControlsForCast", base::FEATURE_ENABLED_BY_DEFAULT};
"GlobalMediaControlsForCast", base::FEATURE_DISABLED_BY_DEFAULT};

// Allow Global Media Controls in system tray of CrOS.
const base::Feature kGlobalMediaControlsForChromeOS{
Expand Down

0 comments on commit 001ba4e

Please sign in to comment.