Skip to content

Commit

Permalink
Fix issues MirroringType::kOffscreenTab
Browse files Browse the repository at this point in the history
1) Zenith dialog is not shown after cast session starts, in case of `MirroringType::kOffscreenTab`. The cause was that ShouldHideNotification() returns true.

2) Casting PresentationRequest (i.e. MirroringType::kOffscreenTab), cause chrome to crash upon trying to stop casting.

3) URL shown as title in Zenith is tailed elided which might cause url spoofing.

(cherry picked from commit a01e3d7)

Bug: 1446754
Change-Id: I771434cbeed2a69b2f783546945ac7311ac81ea7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4559072
Commit-Queue: Ahmed Moussa <ahmedmoussa@google.com>
Reviewed-by: Mark Foltz <mfoltz@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1149311}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4575049
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#207}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
ahmedmoussa authored and Chromium LUCI CQ committed Jun 1, 2023
1 parent 5b05ece commit 128d800
Show file tree
Hide file tree
Showing 6 changed files with 20 additions and 5 deletions.
8 changes: 6 additions & 2 deletions chrome/browser/media/cast_mirroring_service_host.h
Expand Up @@ -176,8 +176,6 @@ class CastMirroringServiceHost final : public MirroringServiceHost,
// of |media_stream_ui_|.
std::unique_ptr<content::MediaStreamUI> media_stream_ui_;

std::unique_ptr<OffscreenTab> offscreen_tab_;

const bool tab_switching_ui_enabled_;

// Represents the number of times a tab was switched during an active
Expand All @@ -195,6 +193,12 @@ class CastMirroringServiceHost final : public MirroringServiceHost,
base::UnguessableToken ignored_token_ = base::UnguessableToken::Create();
const media::VideoCaptureParams ignored_params_;

// Notes on order: `offscreen_tab_` needs to be defined after
// `video_capture_host_`. This guarantees that during class destruction
// `video_capture_host_` gets destroyed before the captured WebContents (i.e
// `offscreen_tab_`).
std::unique_ptr<OffscreenTab> offscreen_tab_;

// Used for calls supplied to `media_stream_ui_`, mainly to handle callbacks
// for TabSharingUIViews. Invalidated every time a new UI is created.
base::WeakPtrFactory<CastMirroringServiceHost> weak_factory_for_ui_{this};
Expand Down
Expand Up @@ -42,7 +42,10 @@ bool ShouldHideNotification(const raw_ptr<Profile> profile,
// Hide a route if it contains a Streaming App, i.e. Tab/Desktop Mirroring
// and Remote Playback routes.
if (source && source->ContainsStreamingApp()) {
return true;
// Don't hide it in case of MirroringType::kOffscreenTab.
// This happens when 1UA mode is being used. It uses a URL for MediaSource
// and a streaming receiver app for CastMediaSource.
return !route.media_source().url().SchemeIsHTTPOrHTTPS();
}
} else if (route.controller_type() !=
media_router::RouteControllerType::kGeneric) {
Expand Down
Expand Up @@ -62,19 +62,24 @@ TEST_F(CastMediaNotificationProducerTest, AddAndRemoveRoute) {
const std::string route_id_1 = "route-id-1";
const std::string route_id_2 = "route-id-2";
const std::string route_id_3 = "route-id-3";
const std::string route_id_4 = "route-id-4";
MediaRoute cast_route = CreateRoute(route_id_1);
MediaRoute site_initiated_mirroring_route =
CreateRoute(route_id_2, "cast:0F5096E8");
MediaRoute dial_route = CreateRoute(route_id_3, "dial:123456");
MediaRoute cast_off_screen_tab_route =
CreateRoute(route_id_4, "https://example.com/");

EXPECT_CALL(item_manager_, OnItemsChanged());
notification_producer_->OnRoutesUpdated(
{cast_route, site_initiated_mirroring_route, dial_route});
{cast_route, site_initiated_mirroring_route, dial_route,
cast_off_screen_tab_route});
testing::Mock::VerifyAndClearExpectations(&item_manager_);
EXPECT_EQ(3u, notification_producer_->GetActiveItemCount());
EXPECT_EQ(4u, notification_producer_->GetActiveItemCount());
EXPECT_NE(nullptr, notification_producer_->GetMediaItem(route_id_1));
EXPECT_NE(nullptr, notification_producer_->GetMediaItem(route_id_2));
EXPECT_NE(nullptr, notification_producer_->GetMediaItem(route_id_3));
EXPECT_NE(nullptr, notification_producer_->GetMediaItem(route_id_4));

EXPECT_CALL(item_manager_, OnItemsChanged());
notification_producer_->OnRoutesUpdated({});
Expand Down
Expand Up @@ -265,6 +265,7 @@ void MediaNotificationViewAshImpl::UpdateWithMediaSessionInfo(

void MediaNotificationViewAshImpl::UpdateWithMediaMetadata(
const media_session::MediaMetadata& metadata) {
source_label_->SetElideBehavior(gfx::ELIDE_HEAD);
source_label_->SetText(metadata.source_title);
title_label_->SetText(metadata.title);
artist_label_->SetText(metadata.artist);
Expand Down
Expand Up @@ -383,6 +383,7 @@ void MediaNotificationViewImpl::UpdateWithMediaMetadata(
metadata.source_title.empty() ? default_app_name_ : metadata.source_title;

if (header_row_) {
header_row_->SetAppNameElideBehavior(gfx::ELIDE_HEAD);
header_row_->SetAppName(app_name);
header_row_->SetSummaryText(metadata.album);
} else {
Expand Down
Expand Up @@ -454,6 +454,7 @@ void MediaNotificationViewModernImpl::UpdateWithMediaSessionInfo(
void MediaNotificationViewModernImpl::UpdateWithMediaMetadata(
const media_session::MediaMetadata& metadata) {
title_label_->SetText(metadata.title);
subtitle_label_->SetElideBehavior(gfx::ELIDE_HEAD);
subtitle_label_->SetText(metadata.source_title);

// Stores the text to be read by screen readers describing the notification.
Expand Down

0 comments on commit 128d800

Please sign in to comment.