Commit e70f2255 authored by Takumi Fujimoto's avatar Takumi Fujimoto Committed by Commit Bot

Reland: [Global Media Controls] Hide sender page media items while casting

Relanding crrev.com/c/1955086 with a fix for crbug.com/1043484.

The only difference from the original patch is in
media_notification_service.cc where we now call
WebContentsPresentationManager::RemoveObserver() from
MediaNotificationService::Session::~Session() before an early exit, so
that it always gets called.

Original message:
When a Cast session is initiated from a web page and the page doesn't
get rid of its media session, we generally get duplicate media
notification items: one for the sender page and the other from the
receiver. This CL hides the former in such cases.

Bug: 1038447
Change-Id: I09a074ec4d08a1c19dc21de925c1974b5f9f47dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2013631Reviewed-by: default avatarTommy Steimel <steimel@chromium.org>
Reviewed-by: default avatarmark a. foltz <mfoltz@chromium.org>
Commit-Queue: Takumi Fujimoto <takumif@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734260}
parent 7394c42b
......@@ -115,6 +115,11 @@ class PresentationFrame {
receiver_connection_receiver);
void RemovePresentation(const std::string& presentation_id);
const base::small_map<std::map<std::string, MediaRoute>>&
presentation_id_to_route() const {
return presentation_id_to_route_;
}
private:
base::small_map<std::map<std::string, MediaRoute>> presentation_id_to_route_;
base::small_map<
......@@ -502,8 +507,7 @@ void PresentationServiceDelegateImpl::AddPresentation(
const MediaRoute& route) {
auto* presentation_frame = GetOrAddPresentationFrame(render_frame_host_id);
presentation_frame->AddPresentation(presentation_info, route);
// TODO(crbug.com/1031672): Notify WebContentsPresentationManager::Observer
// that the presentation routes have changed for the WebContents.
NotifyMediaRoutesChanged();
}
void PresentationServiceDelegateImpl::RemovePresentation(
......@@ -512,8 +516,7 @@ void PresentationServiceDelegateImpl::RemovePresentation(
const auto it = presentation_frames_.find(render_frame_host_id);
if (it != presentation_frames_.end())
it->second->RemovePresentation(presentation_id);
// TODO(crbug.com/1031672): Notify WebContentsPresentationManager::Observer
// that the presentation routes have changed for the WebContents.
NotifyMediaRoutesChanged();
}
void PresentationServiceDelegateImpl::StartPresentation(
......@@ -538,7 +541,8 @@ void PresentationServiceDelegateImpl::StartPresentation(
request,
base::BindOnce(
&PresentationServiceDelegateImpl::OnStartPresentationSucceeded,
GetWeakPtr(), render_frame_host_id, std::move(success_cb)),
weak_factory_.GetWeakPtr(), render_frame_host_id,
std::move(success_cb)),
std::move(error_cb));
if (start_presentation_cb_) {
start_presentation_cb_.Run(std::move(presentation_context));
......@@ -609,8 +613,8 @@ void PresentationServiceDelegateImpl::ReconnectPresentation(
MediaSource::ForPresentationUrl(presentation_url).id(), presentation_id,
request.frame_origin, web_contents_,
base::BindOnce(&PresentationServiceDelegateImpl::OnJoinRouteResponse,
GetWeakPtr(), render_frame_host_id, presentation_url,
presentation_id, std::move(success_cb),
weak_factory_.GetWeakPtr(), render_frame_host_id,
presentation_url, presentation_id, std::move(success_cb),
std::move(error_cb)),
base::TimeDelta(), incognito);
}
......@@ -718,7 +722,7 @@ void PresentationServiceDelegateImpl::OnPresentationResponse(
}
}
base::WeakPtr<PresentationServiceDelegateImpl>
base::WeakPtr<WebContentsPresentationManager>
PresentationServiceDelegateImpl::GetWeakPtr() {
return weak_factory_.GetWeakPtr();
}
......@@ -802,10 +806,20 @@ void PresentationServiceDelegateImpl::EnsurePresentationConnection(
void PresentationServiceDelegateImpl::NotifyDefaultPresentationChanged(
const content::PresentationRequest* request) {
for (WebContentsPresentationManager::Observer& presentation_observer :
presentation_observers_) {
for (auto& presentation_observer : presentation_observers_)
presentation_observer.OnDefaultPresentationChanged(request);
}
void PresentationServiceDelegateImpl::NotifyMediaRoutesChanged() {
std::vector<MediaRoute> routes;
for (const auto& presentation_frame : presentation_frames_) {
for (const auto& route :
presentation_frame.second->presentation_id_to_route()) {
routes.push_back(route.second);
}
}
for (auto& presentation_observer : presentation_observers_)
presentation_observer.OnMediaRoutesChanged(routes);
}
WEB_CONTENTS_USER_DATA_KEY_IMPL(PresentationServiceDelegateImpl)
......
......@@ -158,8 +158,7 @@ class PresentationServiceDelegateImpl
void OnPresentationResponse(const content::PresentationRequest& request,
mojom::RoutePresentationConnectionPtr connection,
const RouteRequestResult& result) override;
base::WeakPtr<PresentationServiceDelegateImpl> GetWeakPtr();
base::WeakPtr<WebContentsPresentationManager> GetWeakPtr() override;
// Returns the WebContents that owns this instance.
content::WebContents* web_contents() const { return web_contents_; }
......@@ -254,6 +253,7 @@ class PresentationServiceDelegateImpl
void NotifyDefaultPresentationChanged(
const content::PresentationRequest* request);
void NotifyMediaRoutesChanged();
// References to the WebContents that owns this instance, and associated
// browser profile's MediaRouter instance.
......@@ -261,7 +261,7 @@ class PresentationServiceDelegateImpl
MediaRouter* router_;
// References to the observers listening for changes to the default
// presentation of the associated WebContents.
// presentation and presentation MediaRoutes associated with the WebContents.
base::ObserverList<WebContentsPresentationManager::Observer>
presentation_observers_;
......
......@@ -393,8 +393,7 @@ TEST_F(PresentationServiceDelegateImplIncognitoTest,
RunDefaultPresentationUrlCallbackTest(true);
}
TEST_F(PresentationServiceDelegateImplTest,
NotifyWebContentsPresentationObservers) {
TEST_F(PresentationServiceDelegateImplTest, NotifyDefaultPresentationChanged) {
auto callback = base::BindRepeating(
&PresentationServiceDelegateImplTest::OnDefaultPresentationStarted,
base::Unretained(this));
......@@ -431,6 +430,28 @@ TEST_F(PresentationServiceDelegateImplTest,
callback);
}
TEST_F(PresentationServiceDelegateImplTest, NotifyMediaRoutesChanged) {
const int render_process_id = 100;
const int render_frame_id = 200;
content::PresentationRequest request(
content::GlobalFrameRoutingId(render_process_id, render_frame_id),
{presentation_url1_}, frame_origin_);
MediaRoute media_route("differentRouteId", source1_, "mediaSinkId", "", true,
true);
std::unique_ptr<RouteRequestResult> result =
RouteRequestResult::FromSuccess(media_route, kPresentationId);
StrictMock<MockWebContentsPresentationObserver> observer(GetWebContents());
EXPECT_CALL(observer,
OnMediaRoutesChanged(std::vector<MediaRoute>({media_route})));
delegate_impl_->OnPresentationResponse(request,
/** connection */ nullptr, *result);
EXPECT_CALL(observer, OnMediaRoutesChanged(std::vector<MediaRoute>()));
delegate_impl_->Terminate(render_process_id, render_frame_id,
kPresentationId);
}
TEST_F(PresentationServiceDelegateImplTest, ListenForConnnectionStateChange) {
content::WebContentsTester::For(GetWebContents())
->NavigateAndCommit(frame_url_);
......
......@@ -8,14 +8,27 @@
namespace media_router {
namespace {
WebContentsPresentationManager* g_test_instance = nullptr;
} // namespace
// static
base::WeakPtr<WebContentsPresentationManager>
WebContentsPresentationManager::Get(content::WebContents* web_contents) {
if (g_test_instance)
return g_test_instance->GetWeakPtr();
return PresentationServiceDelegateImpl::GetOrCreateForWebContents(
web_contents)
->GetWeakPtr();
}
// static
void WebContentsPresentationManager::SetTestInstance(
WebContentsPresentationManager* test_instance) {
g_test_instance = test_instance;
}
WebContentsPresentationManager::~WebContentsPresentationManager() = default;
} // namespace media_router
......@@ -46,6 +46,10 @@ class WebContentsPresentationManager {
static base::WeakPtr<WebContentsPresentationManager> Get(
content::WebContents* web_contents);
// Sets the instance to be returned by Get(). If this is set, Get() ignores
// the |web_contents| argument.
static void SetTestInstance(WebContentsPresentationManager* test_instance);
virtual ~WebContentsPresentationManager() = 0;
virtual void AddObserver(Observer* observer) = 0;
......@@ -68,6 +72,8 @@ class WebContentsPresentationManager {
mojom::RoutePresentationConnectionPtr connection,
const RouteRequestResult& result) = 0;
virtual base::WeakPtr<WebContentsPresentationManager> GetWeakPtr() = 0;
protected:
WebContentsPresentationManager() = default;
};
......
......@@ -63,6 +63,13 @@ bool IsWebContentsFocused(content::WebContents* web_contents) {
return browser->tab_strip_model()->GetActiveWebContents() == web_contents;
}
base::WeakPtr<media_router::WebContentsPresentationManager>
GetPresentationManager(content::WebContents* web_contents) {
return web_contents
? media_router::WebContentsPresentationManager::Get(web_contents)
: nullptr;
}
} // anonymous namespace
MediaNotificationService::Session::Session(
......@@ -74,14 +81,20 @@ MediaNotificationService::Session::Session(
: content::WebContentsObserver(web_contents),
owner_(owner),
id_(id),
item_(std::move(item)) {
item_(std::move(item)),
presentation_manager_(GetPresentationManager(web_contents)) {
DCHECK(owner_);
DCHECK(item_);
SetController(std::move(controller));
if (presentation_manager_)
presentation_manager_->AddObserver(this);
}
MediaNotificationService::Session::~Session() {
if (presentation_manager_)
presentation_manager_->RemoveObserver(this);
// If we've been marked inactive, then we've already recorded inactivity as
// the dismiss reason.
if (is_marked_inactive_)
......@@ -134,6 +147,12 @@ void MediaNotificationService::Session::MediaSessionPositionChanged(
OnSessionInteractedWith();
}
void MediaNotificationService::Session::OnMediaRoutesChanged(
const std::vector<media_router::MediaRoute>& routes) {
if (!routes.empty())
item_->Dismiss();
}
void MediaNotificationService::Session::SetController(
mojo::Remote<media_session::mojom::MediaController> controller) {
if (controller.is_bound()) {
......
......@@ -14,6 +14,7 @@
#include "base/optional.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "chrome/browser/media/router/presentation/web_contents_presentation_manager.h"
#include "chrome/browser/ui/global_media_controls/cast_media_notification_provider.h"
#include "chrome/browser/ui/global_media_controls/media_notification_container_observer.h"
#include "chrome/browser/ui/global_media_controls/overlay_media_notifications_manager.h"
......@@ -122,8 +123,10 @@ class MediaNotificationService
kMaxValue = kMediaSessionStopped,
};
class Session : public content::WebContentsObserver,
public media_session::mojom::MediaControllerObserver {
class Session
: public content::WebContentsObserver,
public media_session::mojom::MediaControllerObserver,
public media_router::WebContentsPresentationManager::Observer {
public:
Session(MediaNotificationService* owner,
const std::string& id,
......@@ -135,7 +138,7 @@ class MediaNotificationService
Session& operator=(const Session&) = delete;
~Session() override;
// content::WebContentsObserver implementation.
// content::WebContentsObserver:
void WebContentsDestroyed() override;
// media_session::mojom::MediaControllerObserver:
......@@ -152,6 +155,10 @@ class MediaNotificationService
void MediaSessionPositionChanged(
const base::Optional<media_session::MediaPosition>& position) override;
// media_router::WebContentsPresentationManager::Observer:
void OnMediaRoutesChanged(
const std::vector<media_router::MediaRoute>& routes) override;
media_message_center::MediaSessionNotificationItem* item() {
return item_.get();
}
......@@ -207,6 +214,9 @@ class MediaNotificationService
// Used to receive updates to the Media Session playback state.
mojo::Receiver<media_session::mojom::MediaControllerObserver>
observer_receiver_{this};
base::WeakPtr<media_router::WebContentsPresentationManager>
presentation_manager_;
};
void OnReceivedAudioFocusRequests(
......
......@@ -6,6 +6,9 @@
#include "base/run_loop.h"
#include "base/test/scoped_feature_list.h"
#include "chrome/browser/media/router/media_router_factory.h"
#include "chrome/browser/media/router/presentation/web_contents_presentation_manager.h"
#include "chrome/browser/media/router/test/mock_media_router.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/global_media_controls/media_toolbar_button_observer.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
......@@ -17,6 +20,7 @@
#include "chrome/test/base/interactive_test_utils.h"
#include "chrome/test/base/ui_test_utils.h"
#include "components/media_message_center/media_notification_view_impl.h"
#include "content/public/browser/presentation_request.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/media_start_stop_observer.h"
#include "media/base/media_switches.h"
......@@ -90,6 +94,17 @@ class MediaToolbarButtonWatcher : public MediaToolbarButtonObserver,
Wait();
}
void WaitForNotificationCount(int count) {
if (GetNotificationCount() == count)
return;
waiting_for_notification_count_ = true;
expected_notification_count_ = count;
observed_dialog_ = MediaDialogView::GetDialogViewForTesting();
observed_dialog_->AddObserver(this);
Wait();
}
private:
void CheckDialogForText() {
if (!waiting_for_dialog_to_contain_text_)
......@@ -102,12 +117,24 @@ class MediaToolbarButtonWatcher : public MediaToolbarButtonObserver,
MaybeStopWaiting();
}
void CheckNotificationCount() {
if (!waiting_for_notification_count_)
return;
if (GetNotificationCount() != expected_notification_count_)
return;
waiting_for_notification_count_ = false;
MaybeStopWaiting();
}
void MaybeStopWaiting() {
if (!run_loop_)
return;
if (!waiting_for_dialog_opened_ && !waiting_for_button_shown_ &&
!waiting_for_dialog_to_contain_text_) {
!waiting_for_dialog_to_contain_text_ &&
!waiting_for_notification_count_) {
run_loop_->Quit();
}
}
......@@ -129,26 +156,98 @@ class MediaToolbarButtonWatcher : public MediaToolbarButtonObserver,
if (view->title_label_for_testing()->GetText().find(text) !=
std::string::npos ||
view->artist_label_for_testing()->GetText().find(text) !=
std::string::npos) {
std::string::npos ||
view->GetSourceTitleForTesting().find(text) != std::string::npos) {
return true;
}
}
return false;
}
int GetNotificationCount() {
return MediaDialogView::GetDialogViewForTesting()
->GetNotificationsForTesting()
.size();
}
MediaToolbarButtonView* const button_;
std::unique_ptr<base::RunLoop> run_loop_;
bool waiting_for_dialog_opened_ = false;
bool waiting_for_button_shown_ = false;
bool waiting_for_notification_count_ = false;
MediaDialogView* observed_dialog_ = nullptr;
bool waiting_for_dialog_to_contain_text_ = false;
base::string16 expected_text_;
int expected_notification_count_ = 0;
DISALLOW_COPY_AND_ASSIGN(MediaToolbarButtonWatcher);
};
class TestWebContentsPresentationManager
: public media_router::WebContentsPresentationManager {
public:
void NotifyMediaRoutesChanged(
const std::vector<media_router::MediaRoute>& routes) {
for (auto& observer : observers_) {
observer.OnMediaRoutesChanged(routes);
}
}
void AddObserver(Observer* observer) override {
observers_.AddObserver(observer);
}
void RemoveObserver(Observer* observer) override {
observers_.RemoveObserver(observer);
}
MOCK_CONST_METHOD0(HasDefaultPresentationRequest, bool());
MOCK_CONST_METHOD0(GetDefaultPresentationRequest,
const content::PresentationRequest&());
void OnPresentationResponse(
const content::PresentationRequest& presentation_request,
media_router::mojom::RoutePresentationConnectionPtr connection,
const media_router::RouteRequestResult& result) override {}
base::WeakPtr<WebContentsPresentationManager> GetWeakPtr() override {
return weak_factory_.GetWeakPtr();
}
private:
base::ObserverList<Observer> observers_;
base::WeakPtrFactory<TestWebContentsPresentationManager> weak_factory_{this};
};
class TestMediaRouter : public media_router::MockMediaRouter {
public:
static std::unique_ptr<KeyedService> Create(
content::BrowserContext* context) {
return std::make_unique<TestMediaRouter>();
}
void RegisterMediaRoutesObserver(
media_router::MediaRoutesObserver* observer) override {
routes_observers_.push_back(observer);
}
void UnregisterMediaRoutesObserver(
media_router::MediaRoutesObserver* observer) override {
base::Erase(routes_observers_, observer);
}
void NotifyMediaRoutesChanged(
const std::vector<media_router::MediaRoute>& routes) {
for (auto* observer : routes_observers_)
observer->OnRoutesUpdated(routes, {});
}
private:
std::vector<media_router::MediaRoutesObserver*> routes_observers_;
};
} // anonymous namespace
class MediaDialogViewBrowserTest : public InProcessBrowserTest {
......@@ -163,10 +262,38 @@ class MediaDialogViewBrowserTest : public InProcessBrowserTest {
}
void SetUp() override {
feature_list_.InitAndEnableFeature(media::kGlobalMediaControls);
feature_list_.InitWithFeatures(
{media::kGlobalMediaControls, media::kGlobalMediaControlsForCast}, {});
presentation_manager_ =
std::make_unique<TestWebContentsPresentationManager>();
media_router::WebContentsPresentationManager::SetTestInstance(
presentation_manager_.get());
InProcessBrowserTest::SetUp();
}
void TearDown() override {
InProcessBrowserTest::TearDown();
media_router::WebContentsPresentationManager::SetTestInstance(nullptr);
}
void SetUpInProcessBrowserTestFixture() override {
subscription_ =
BrowserContextDependencyManager::GetInstance()
->RegisterWillCreateBrowserContextServicesCallbackForTesting(
base::BindRepeating(&MediaDialogViewBrowserTest::
OnWillCreateBrowserContextServices,
base::Unretained(this)));
}
void OnWillCreateBrowserContextServices(content::BrowserContext* context) {
media_router_ = static_cast<TestMediaRouter*>(
media_router::MediaRouterFactory::GetInstance()
->SetTestingFactoryAndUse(
context, base::BindRepeating(&TestMediaRouter::Create)));
}
MediaToolbarButtonView* GetToolbarIcon() {
return BrowserView::GetBrowserViewForBrowser(browser())
->toolbar()
......@@ -228,6 +355,10 @@ class MediaDialogViewBrowserTest : public InProcessBrowserTest {
.WaitForDialogToContainText(text);
}
void WaitForNotificationCount(int count) {
MediaToolbarButtonWatcher(GetToolbarIcon()).WaitForNotificationCount(count);
}
void ClickPauseButtonOnDialog() {
base::RunLoop().RunUntilIdle();
ASSERT_TRUE(MediaDialogView::IsShowing());
......@@ -252,6 +383,10 @@ class MediaDialogViewBrowserTest : public InProcessBrowserTest {
return browser()->tab_strip_model()->GetActiveWebContents();
}
protected:
std::unique_ptr<TestWebContentsPresentationManager> presentation_manager_;
TestMediaRouter* media_router_ = nullptr;
private:
void ClickButton(views::Button* button) {
base::RunLoop closure_loop;
......@@ -302,6 +437,9 @@ class MediaDialogViewBrowserTest : public InProcessBrowserTest {
}
base::test::ScopedFeatureList feature_list_;
std::unique_ptr<
base::CallbackList<void(content::BrowserContext*)>::Subscription>
subscription_;
DISALLOW_COPY_AND_ASSIGN(MediaDialogViewBrowserTest);
};
......@@ -408,3 +546,26 @@ IN_PROC_BROWSER_TEST_F(MediaDialogViewBrowserTest,
ClickNotificationByTitle(base::ASCIIToUTF16("Big Buck Bunny"));
EXPECT_EQ(first_web_contents, GetActiveWebContents());
}
IN_PROC_BROWSER_TEST_F(MediaDialogViewBrowserTest, ShowsCastSession) {
OpenTestURL();
StartPlayback();
WaitForStart();
const std::string route_description = "Casting: Big Buck Bunny";
const std::string sink_name = "My Sink";
media_router::MediaRoute route("id", media_router::MediaSource("source_id"),
"sink_id", route_description, true, true);
route.set_media_sink_name(sink_name);
route.set_controller_type(media_router::RouteControllerType::kGeneric);
media_router_->NotifyMediaRoutesChanged({route});
base::RunLoop().RunUntilIdle();
presentation_manager_->NotifyMediaRoutesChanged({route});
WaitForVisibleToolbarIcon();
ClickToolbarIcon();
WaitForDialogOpened();
WaitForDialogToContainText(
base::UTF8ToUTF16(route_description + " \xC2\xB7 " + sink_name));
WaitForNotificationCount(1);
}
......@@ -5826,6 +5826,7 @@ if (!is_android) {
}
if (!is_chromeos) {
sources += [ "../browser/ui/views/global_media_controls/media_dialog_view_interactive_browsertest.cc" ]
deps += [ "../browser/media/router:test_support" ]
}
if (use_aura || is_mac) {
deps += [ "//ui/touch_selection" ]
......
......@@ -394,6 +394,10 @@ views::Button* MediaNotificationViewImpl::GetHeaderRowForTesting() const {
return header_row_;
}
base::string16 MediaNotificationViewImpl::GetSourceTitleForTesting() const {
return header_row_->app_name_for_testing();
}
void MediaNotificationViewImpl::UpdateActionButtonsVisibility() {
base::flat_set<MediaSessionAction> ignored_actions = {
GetPlayPauseIgnoredAction(GetActionFromButtonTag(*play_pause_button_))};
......
......@@ -85,6 +85,7 @@ class COMPONENT_EXPORT(MEDIA_MESSAGE_CENTER) MediaNotificationViewImpl
const views::Label* artist_label_for_testing() const { return artist_label_; }
views::Button* GetHeaderRowForTesting() const;
base::string16 GetSourceTitleForTesting() const;
private:
friend class MediaNotificationViewImplTest;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment