Skip to content

Commit

Permalink
Keep showing the media notification when paused from the browser
Browse files Browse the repository at this point in the history 
Previously, the media notification is hidden when the user pauses the
media in the browser (WebContents), which is inconsistent with pausing
from the notification.

Now we keep the notification visible when the media is paused from the
browser, and only hide it if there is nothing else to play or the user
actively hides it.

Implementation details:

If there are multiple players playing, and user pauses one player in the
browser, then we do in the old way: the paused player will be removed
from the MediaSesssion. The notification will keep showing.

If there is only one player playing, and user pauses it in the browser,
then we mark the player as "pending", and don't remove it from the
MediaSession. The MediaSession will be in SUSPENDED state.

The pending player is only valid when it was the last player in the
MediaSession, and the last operation is removing it from MediaSession by
a browser pause. Adding players to the MediaSession will invalidate the
pending player.

When the user requests to resume playing from the notification, if there
is a pending player, it will be added to the player set and resume
playing. Otherwise, we do in the old way: resume all players in the
player set.

BUG=539998

Review URL: https://codereview.chromium.org/1525613002

Cr-Commit-Position: refs/heads/master@{#365810}
  • Loading branch information
@xxyzzzq
xxyzzzq authored and Commit bot committed Dec 17, 2015
1 parent be8b776 commit 137780d
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 39 deletions.
27 changes: 17 additions & 10 deletions content/browser/media/android/browser_media_player_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,8 @@ BrowserMediaPlayerManager::~BrowserMediaPlayerManager() {
for (MediaPlayerAndroid* player : players_)
player->DeleteOnCorrectThread();

MediaSession::Get(web_contents())->RemovePlayers(this);
MediaSession::Get(web_contents())->RemovePlayers(
this, MediaSession::RemoveReason::DESTROYED);
players_.weak_clear();
}

Expand Down Expand Up @@ -302,7 +303,8 @@ void BrowserMediaPlayerManager::OnMediaMetadataChanged(

void BrowserMediaPlayerManager::OnPlaybackComplete(int player_id) {
Send(new MediaPlayerMsg_MediaPlaybackCompleted(RoutingID(), player_id));
MediaSession::Get(web_contents())->RemovePlayer(this, player_id);
MediaSession::Get(web_contents())->RemovePlayer(
this, player_id, MediaSession::RemoveReason::PLAYBACK_COMPLETE);

if (fullscreen_player_id_ == player_id)
video_view_->OnPlaybackComplete();
Expand Down Expand Up @@ -550,7 +552,7 @@ void BrowserMediaPlayerManager::OnInitialize(
<< "Media source players must have positive demuxer client IDs: "
<< media_player_params.demuxer_client_id;

RemovePlayer(media_player_params.player_id);
DestroyPlayer(media_player_params.player_id);

RenderProcessHostImpl* host = static_cast<RenderProcessHostImpl*>(
web_contents()->GetRenderProcessHost());
Expand Down Expand Up @@ -595,8 +597,10 @@ void BrowserMediaPlayerManager::OnPause(
if (player)
player->Pause(is_media_related_action);

if (is_media_related_action)
MediaSession::Get(web_contents())->RemovePlayer(this, player_id);
if (is_media_related_action) {
MediaSession::Get(web_contents())->RemovePlayer(
this, player_id, MediaSession::RemoveReason::USER_PAUSE);
}
}

void BrowserMediaPlayerManager::OnSetVolume(int player_id, double volume) {
Expand All @@ -614,8 +618,10 @@ void BrowserMediaPlayerManager::OnReleaseResources(int player_id) {
if (player) {
// Videos can't play in the background, so are removed from the media
// session.
if (player->GetVideoWidth() > 0)
MediaSession::Get(web_contents())->RemovePlayer(this, player_id);
if (player->GetVideoWidth() > 0) {
MediaSession::Get(web_contents())->RemovePlayer(
this, player_id, MediaSession::RemoveReason::INVISIBLE);
}

ReleasePlayer(player);
}
Expand All @@ -624,7 +630,7 @@ void BrowserMediaPlayerManager::OnReleaseResources(int player_id) {
}

void BrowserMediaPlayerManager::OnDestroyPlayer(int player_id) {
RemovePlayer(player_id);
DestroyPlayer(player_id);
if (fullscreen_player_id_ == player_id)
fullscreen_player_id_ = kInvalidMediaPlayerId;
}
Expand All @@ -643,7 +649,7 @@ void BrowserMediaPlayerManager::AddPlayer(MediaPlayerAndroid* player) {
players_.push_back(player);
}

void BrowserMediaPlayerManager::RemovePlayer(int player_id) {
void BrowserMediaPlayerManager::DestroyPlayer(int player_id) {
for (ScopedVector<MediaPlayerAndroid>::iterator it = players_.begin();
it != players_.end(); ++it) {
if ((*it)->player_id() == player_id) {
Expand All @@ -652,7 +658,8 @@ void BrowserMediaPlayerManager::RemovePlayer(int player_id) {
#endif
(*it)->DeleteOnCorrectThread();
players_.weak_erase(it);
MediaSession::Get(web_contents())->RemovePlayer(this, player_id);
MediaSession::Get(web_contents())->RemovePlayer(
this, player_id, MediaSession::RemoveReason::DESTROYED);
break;
}
}
Expand Down
2 changes: 1 addition & 1 deletion content/browser/media/android/browser_media_player_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class CONTENT_EXPORT BrowserMediaPlayerManager
void AddPlayer(media::MediaPlayerAndroid* player);

// Removes the player with the specified id.
void RemovePlayer(int player_id);
void DestroyPlayer(int player_id);

// Replaces a player with the specified id with a given MediaPlayerAndroid
// object. This will also return the original MediaPlayerAndroid object that
Expand Down
15 changes: 12 additions & 3 deletions content/browser/media/android/media_session.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,15 +93,24 @@ bool MediaSession::AddPlayer(MediaSessionObserver* observer,
}

void MediaSession::RemovePlayer(MediaSessionObserver* observer,
int player_id) {
int player_id,
RemoveReason remove_reason) {
auto it = players_.find(PlayerIdentifier(observer, player_id));
if (it != players_.end())
if (it != players_.end()) {
if (RemoveReason::USER_PAUSE == remove_reason && players_.size() == 1) {
if (!IsSuspended())
Suspend();
return;
}
players_.erase(it);
}

AbandonSystemAudioFocusIfNeeded();
}

void MediaSession::RemovePlayers(MediaSessionObserver* observer) {
void MediaSession::RemovePlayers(MediaSessionObserver* observer,
RemoveReason remove_reason) {
DCHECK(remove_reason == RemoveReason::DESTROYED);
for (auto it = players_.begin(); it != players_.end();) {
if (it->observer == observer)
players_.erase(it++);
Expand Down
13 changes: 11 additions & 2 deletions content/browser/media/android/media_session.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ class CONTENT_EXPORT MediaSession
Transient
};

enum class RemoveReason {
PLAYBACK_COMPLETE,
INVISIBLE,
USER_PAUSE,
DESTROYED,
};

static bool RegisterMediaSession(JNIEnv* env);

// Returns the MediaSession associated to this WebContents. Creates one if
Expand All @@ -54,11 +61,13 @@ class CONTENT_EXPORT MediaSession

// Removes the given player from the current media session. Abandons audio
// focus if that was the last player in the session.
void RemovePlayer(MediaSessionObserver* observer, int player_id);
void RemovePlayer(MediaSessionObserver* observer, int player_id,
RemoveReason remove_reason);

// Removes all the players associated with |observer|. Abandons audio focus if
// these were the last players in the session.
void RemovePlayers(MediaSessionObserver* observer);
void RemovePlayers(MediaSessionObserver* observer,
RemoveReason remove_reason);

// Called when the Android system requests the MediaSession to be suspended.
// Called by Java through JNI.
Expand Down
91 changes: 68 additions & 23 deletions content/browser/media/android/media_session_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,12 +134,14 @@ class MediaSessionBrowserTest : public content::ContentBrowserTest {
}

void RemovePlayer(MockMediaSessionObserver* media_session_observer,
int player_id) {
media_session_->RemovePlayer(media_session_observer, player_id);
int player_id, MediaSession::RemoveReason remove_reason) {
media_session_->RemovePlayer(
media_session_observer, player_id, remove_reason);
}

void RemovePlayers(MockMediaSessionObserver* media_session_observer) {
media_session_->RemovePlayers(media_session_observer);
void RemovePlayers(MockMediaSessionObserver* media_session_observer,
MediaSession::RemoveReason remove_reason) {
media_session_->RemovePlayers(media_session_observer, remove_reason);
}

bool HasAudioFocus() { return media_session_->IsActiveForTest(); }
Expand Down Expand Up @@ -334,11 +336,14 @@ IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest,
StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);
StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);

RemovePlayer(media_session_observer.get(), 0);
RemovePlayer(media_session_observer.get(), 0,
MediaSession::RemoveReason::USER_PAUSE);
EXPECT_TRUE(HasAudioFocus());
RemovePlayer(media_session_observer.get(), 1);
RemovePlayer(media_session_observer.get(), 1,
MediaSession::RemoveReason::USER_PAUSE);
EXPECT_TRUE(HasAudioFocus());
RemovePlayer(media_session_observer.get(), 2);
RemovePlayer(media_session_observer.get(), 2,
MediaSession::RemoveReason::USER_PAUSE);
EXPECT_FALSE(HasAudioFocus());
}

Expand All @@ -355,11 +360,14 @@ IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest,
StartNewPlayer(media_session_observer_2.get(), MediaSession::Type::Content);
StartNewPlayer(media_session_observer_3.get(), MediaSession::Type::Content);

RemovePlayer(media_session_observer_1.get(), 0);
RemovePlayer(media_session_observer_1.get(), 0,
MediaSession::RemoveReason::USER_PAUSE);
EXPECT_TRUE(HasAudioFocus());
RemovePlayer(media_session_observer_2.get(), 0);
RemovePlayer(media_session_observer_2.get(), 0,
MediaSession::RemoveReason::USER_PAUSE);
EXPECT_TRUE(HasAudioFocus());
RemovePlayer(media_session_observer_3.get(), 0);
RemovePlayer(media_session_observer_3.get(), 0,
MediaSession::RemoveReason::USER_PAUSE);
EXPECT_FALSE(HasAudioFocus());
}

Expand All @@ -375,9 +383,11 @@ IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest,
StartNewPlayer(media_session_observer_2.get(), MediaSession::Type::Content);
StartNewPlayer(media_session_observer_2.get(), MediaSession::Type::Content);

RemovePlayers(media_session_observer_1.get());
RemovePlayers(media_session_observer_1.get(),
MediaSession::RemoveReason::DESTROYED);
EXPECT_TRUE(HasAudioFocus());
RemovePlayers(media_session_observer_2.get());
RemovePlayers(media_session_observer_2.get(),
MediaSession::RemoveReason::DESTROYED);
EXPECT_FALSE(HasAudioFocus());
}

Expand All @@ -387,7 +397,8 @@ IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest, ResumePlayGivesAudioFocus) {

StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);

RemovePlayer(media_session_observer.get(), 0);
RemovePlayer(media_session_observer.get(), 0,
MediaSession::RemoveReason::USER_PAUSE);
EXPECT_FALSE(HasAudioFocus());

EXPECT_TRUE(
Expand Down Expand Up @@ -448,8 +459,10 @@ IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest,

StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);

RemovePlayer(media_session_observer.get(), 0);
RemovePlayer(media_session_observer.get(), 0);
RemovePlayer(media_session_observer.get(), 0,
MediaSession::RemoveReason::USER_PAUSE);
RemovePlayer(media_session_observer.get(), 0,
MediaSession::RemoveReason::USER_PAUSE);
}

IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest, MediaSessionType) {
Expand Down Expand Up @@ -536,7 +549,8 @@ IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest, ControlsHideWhenStopped) {

StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);

RemovePlayers(media_session_observer.get());
RemovePlayers(media_session_observer.get(),
MediaSession::RemoveReason::DESTROYED);

EXPECT_FALSE(IsControllable());
EXPECT_TRUE(IsSuspended());
Expand Down Expand Up @@ -591,14 +605,41 @@ IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest,

// Removing only content player doesn't hide the controls since the session
// is still active.
RemovePlayer(media_session_observer.get(), 0);
RemovePlayer(media_session_observer.get(), 0,
MediaSession::RemoveReason::DESTROYED);

EXPECT_TRUE(IsControllable());
EXPECT_FALSE(IsSuspended());
}

IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest,
ControlsHideWhenTheLastPlayerIsStopped) {
ControlsStayWhenTheLastPlayerIsRemovedFromPause) {
Expectation showControls = EXPECT_CALL(*mock_web_contents_observer(),
MediaSessionStateChanged(true, false));
EXPECT_CALL(*mock_web_contents_observer(),
MediaSessionStateChanged(true, true))
.After(showControls);
scoped_ptr<MockMediaSessionObserver> media_session_observer(
new MockMediaSessionObserver);

StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);
StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);

RemovePlayer(media_session_observer.get(), 0,
MediaSession::RemoveReason::USER_PAUSE);

EXPECT_TRUE(IsControllable());
EXPECT_FALSE(IsSuspended());

RemovePlayer(media_session_observer.get(), 1,
MediaSession::RemoveReason::USER_PAUSE);

EXPECT_TRUE(IsControllable());
EXPECT_TRUE(IsSuspended());
}

IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest,
ControlsHideWhenTheLastPlayerIsRemovedNotFromPause) {
Expectation showControls = EXPECT_CALL(*mock_web_contents_observer(),
MediaSessionStateChanged(true, false));
EXPECT_CALL(*mock_web_contents_observer(),
Expand All @@ -610,14 +651,16 @@ IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest,
StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);
StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);

RemovePlayer(media_session_observer.get(), 0);
RemovePlayer(media_session_observer.get(), 0,
MediaSession::RemoveReason::USER_PAUSE);

EXPECT_TRUE(IsControllable());
EXPECT_FALSE(IsSuspended());

RemovePlayer(media_session_observer.get(), 1);
RemovePlayer(media_session_observer.get(), 1,
MediaSession::RemoveReason::DESTROYED);

EXPECT_FALSE(IsControllable());
EXPECT_TRUE(!IsControllable());
EXPECT_TRUE(IsSuspended());
}

Expand All @@ -635,7 +678,8 @@ IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest,
StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);
StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);

RemovePlayers(media_session_observer.get());
RemovePlayers(media_session_observer.get(),
MediaSession::RemoveReason::DESTROYED);

EXPECT_FALSE(IsControllable());
EXPECT_TRUE(IsSuspended());
Expand Down Expand Up @@ -1178,7 +1222,8 @@ IN_PROC_BROWSER_TEST_F(MediaSessionBrowserTest,

StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);
clock->Advance(base::TimeDelta::FromMilliseconds(10000));
RemovePlayer(media_session_observer.get(), 0);
RemovePlayer(media_session_observer.get(), 0,
MediaSession::RemoveReason::DESTROYED);

StartNewPlayer(media_session_observer.get(), MediaSession::Type::Content);
clock->Advance(base::TimeDelta::FromMilliseconds(1000));
Expand Down

0 comments on commit 137780d

Please sign in to comment.