From 128d8006c125f003a006bbe15b36b98e1a2eca26 Mon Sep 17 00:00:00 2001 From: ahmedmoussa Date: Thu, 1 Jun 2023 02:24:18 +0000 Subject: [PATCH] Fix issues MirroringType::kOffscreenTab 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 a01e3d760b3b51cdcfe56a327619a3bdcfb8811a) Bug: 1446754 Change-Id: I771434cbeed2a69b2f783546945ac7311ac81ea7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4559072 Commit-Queue: Ahmed Moussa Reviewed-by: Mark Foltz Cr-Original-Commit-Position: refs/heads/main@{#1149311} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4575049 Reviewed-by: Tommy Steimel Cr-Commit-Position: refs/branch-heads/5790@{#207} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- chrome/browser/media/cast_mirroring_service_host.h | 8 ++++++-- .../cast_media_notification_producer.cc | 5 ++++- .../cast_media_notification_producer_unittest.cc | 9 +++++++-- .../media_notification_view_ash_impl.cc | 1 + .../media_message_center/media_notification_view_impl.cc | 1 + .../media_notification_view_modern_impl.cc | 1 + 6 files changed, 20 insertions(+), 5 deletions(-) diff --git a/chrome/browser/media/cast_mirroring_service_host.h b/chrome/browser/media/cast_mirroring_service_host.h index 1bc6f2a02ca35..042cfc0429b21 100644 --- a/chrome/browser/media/cast_mirroring_service_host.h +++ b/chrome/browser/media/cast_mirroring_service_host.h @@ -176,8 +176,6 @@ class CastMirroringServiceHost final : public MirroringServiceHost, // of |media_stream_ui_|. std::unique_ptr media_stream_ui_; - std::unique_ptr offscreen_tab_; - const bool tab_switching_ui_enabled_; // Represents the number of times a tab was switched during an active @@ -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 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 weak_factory_for_ui_{this}; diff --git a/chrome/browser/ui/global_media_controls/cast_media_notification_producer.cc b/chrome/browser/ui/global_media_controls/cast_media_notification_producer.cc index 0dd8363c22db2..126af65a76721 100644 --- a/chrome/browser/ui/global_media_controls/cast_media_notification_producer.cc +++ b/chrome/browser/ui/global_media_controls/cast_media_notification_producer.cc @@ -42,7 +42,10 @@ bool ShouldHideNotification(const raw_ptr 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) { diff --git a/chrome/browser/ui/global_media_controls/cast_media_notification_producer_unittest.cc b/chrome/browser/ui/global_media_controls/cast_media_notification_producer_unittest.cc index db621f87d9ef2..245ecd6911a43 100644 --- a/chrome/browser/ui/global_media_controls/cast_media_notification_producer_unittest.cc +++ b/chrome/browser/ui/global_media_controls/cast_media_notification_producer_unittest.cc @@ -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({}); diff --git a/components/media_message_center/media_notification_view_ash_impl.cc b/components/media_message_center/media_notification_view_ash_impl.cc index 0c76e58b3b052..7cafae9359a54 100644 --- a/components/media_message_center/media_notification_view_ash_impl.cc +++ b/components/media_message_center/media_notification_view_ash_impl.cc @@ -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); diff --git a/components/media_message_center/media_notification_view_impl.cc b/components/media_message_center/media_notification_view_impl.cc index ec11bbc5da300..756c2bb0ed47c 100644 --- a/components/media_message_center/media_notification_view_impl.cc +++ b/components/media_message_center/media_notification_view_impl.cc @@ -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 { diff --git a/components/media_message_center/media_notification_view_modern_impl.cc b/components/media_message_center/media_notification_view_modern_impl.cc index 5978ab6761403..fdd673c855599 100644 --- a/components/media_message_center/media_notification_view_modern_impl.cc +++ b/components/media_message_center/media_notification_view_modern_impl.cc @@ -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.