Commit db1fc486 authored by Alex Turner's avatar Alex Turner Committed by Chromium LUCI CQ

Reland "Finish base::Bind -> Once/Repeating migration for media/blink and /cdm"

This is a reland of 878c8c7d

Reason for reland: media_blink_unittests should no longer be flaky after
updating the condition in UpdateBackgroundVideoOptimzationState(). The
flakiness derived from CancelableOnceCallback/Closure::IsCancelled()
returning true after the callback is run.

Original change's description:
> Finish base::Bind -> Once/Repeating migration for media/blink and /cdm
>
> Use of base::Bind, ::Callback, etc. is deprecated in favor of the more
> explicit Once/Repeating versions. Despite being marked as fixed, a few
> directories have some remaining uses that were missed in the conversion.
> This cl fixes those stragglers in the media/blink and media/cdm
> directories. The presubmit is also enabled for these directories to
> avoid any future regressions.
>
> Bug: 1141533, 1102651, 1007802
> Change-Id: I0a58ba3165c1c8873a25de76065a7425495b62b1
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2505580
> Reviewed-by: Dirk Pranke <dpranke@google.com>
> Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
> Commit-Queue: Alex Turner <alexmt@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#822344}

Bug: 1141533
Bug: 1102651
Bug: 1007802
Change-Id: Id1f87603ca9d41fe4fad91d6a7b389c889669fba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2508930
Commit-Queue: Alex Turner <alexmt@chromium.org>
Reviewed-by: default avatarDirk Pranke <dpranke@google.com>
Reviewed-by: default avatarChrome Cunningham <chcunningham@chromium.org>
Reviewed-by: default avatarDale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838765}
parent 9ef5ad0f
...@@ -383,8 +383,6 @@ _NOT_CONVERTED_TO_MODERN_BIND_AND_CALLBACK = '|'.join(( ...@@ -383,8 +383,6 @@ _NOT_CONVERTED_TO_MODERN_BIND_AND_CALLBACK = '|'.join((
'^components/webcrypto/', '^components/webcrypto/',
'^extensions/browser/', '^extensions/browser/',
'^extensions/renderer/', '^extensions/renderer/',
'^media/blink/',
'^media/cdm/',
'^net/http/', '^net/http/',
'^net/url_request/', '^net/url_request/',
'^ppapi/proxy/', '^ppapi/proxy/',
......
...@@ -2891,7 +2891,8 @@ void WebMediaPlayerImpl::StartPipeline() { ...@@ -2891,7 +2891,8 @@ void WebMediaPlayerImpl::StartPipeline() {
// base::Unretained is safe because |this| owns memory_pressure_listener_. // base::Unretained is safe because |this| owns memory_pressure_listener_.
memory_pressure_listener_ = memory_pressure_listener_ =
std::make_unique<base::MemoryPressureListener>( std::make_unique<base::MemoryPressureListener>(
FROM_HERE, base::Bind(&WebMediaPlayerImpl::OnMemoryPressure, FROM_HERE,
base::BindRepeating(&WebMediaPlayerImpl::OnMemoryPressure,
base::Unretained(this))); base::Unretained(this)));
} }
} }
...@@ -3608,10 +3609,14 @@ void WebMediaPlayerImpl::UpdateBackgroundVideoOptimizationState() { ...@@ -3608,10 +3609,14 @@ void WebMediaPlayerImpl::UpdateBackgroundVideoOptimizationState() {
if (IsHidden()) { if (IsHidden()) {
if (ShouldPausePlaybackWhenHidden()) { if (ShouldPausePlaybackWhenHidden()) {
PauseVideoIfNeeded(); PauseVideoIfNeeded();
} else if (update_background_status_cb_.IsCancelled()) { } else if (update_background_status_cb_.IsCancelled() &&
!video_track_disabled_) {
// Note: `IsCancelled()` can means the callback is now null, which can be
// either because it already ran, or because it was cancelled.
// Only trigger updates when we don't have one already scheduled. // Only trigger updates when we don't have one already scheduled.
update_background_status_cb_.Reset( update_background_status_cb_.Reset(
base::Bind(&WebMediaPlayerImpl::DisableVideoTrackIfNeeded, base::BindOnce(&WebMediaPlayerImpl::DisableVideoTrackIfNeeded,
base::Unretained(this))); base::Unretained(this)));
// Defer disable track until we're sure the clip will be backgrounded for // Defer disable track until we're sure the clip will be backgrounded for
......
...@@ -994,7 +994,7 @@ class MEDIA_BLINK_EXPORT WebMediaPlayerImpl ...@@ -994,7 +994,7 @@ class MEDIA_BLINK_EXPORT WebMediaPlayerImpl
OverlayInfo overlay_info_; OverlayInfo overlay_info_;
base::CancelableClosure update_background_status_cb_; base::CancelableOnceClosure update_background_status_cb_;
mojo::Remote<mojom::MediaMetricsProvider> media_metrics_provider_; mojo::Remote<mojom::MediaMetricsProvider> media_metrics_provider_;
mojo::Remote<mojom::PlaybackEventsRecorder> playback_events_recorder_; mojo::Remote<mojom::PlaybackEventsRecorder> playback_events_recorder_;
......
...@@ -2303,6 +2303,7 @@ TEST_P(WebMediaPlayerImplBackgroundBehaviorTest, VideoOnly) { ...@@ -2303,6 +2303,7 @@ TEST_P(WebMediaPlayerImplBackgroundBehaviorTest, VideoOnly) {
TEST_P(WebMediaPlayerImplBackgroundBehaviorTest, AudioVideo) { TEST_P(WebMediaPlayerImplBackgroundBehaviorTest, AudioVideo) {
SetMetadata(true, true); SetMetadata(true, true);
BackgroundPlayer();
// Optimization requirements are the same for all platforms. // Optimization requirements are the same for all platforms.
bool matches_requirements = bool matches_requirements =
...@@ -2328,22 +2329,23 @@ TEST_P(WebMediaPlayerImplBackgroundBehaviorTest, AudioVideo) { ...@@ -2328,22 +2329,23 @@ TEST_P(WebMediaPlayerImplBackgroundBehaviorTest, AudioVideo) {
// These tests start in background mode prior to having metadata, so put the // These tests start in background mode prior to having metadata, so put the
// test back into a normal state. // test back into a normal state.
EXPECT_TRUE(IsDisableVideoTrackPending()); EXPECT_TRUE(IsDisableVideoTrackPending() || IsVideoTrackDisabled());
ForegroundPlayer(); ForegroundPlayer();
EXPECT_FALSE(IsVideoTrackDisabled());
// Testing IsVideoTrackDisabled() here is flaky as EnableVideoTrackIfNeeded()
// does not reenable the video track because IsPipelineRunning() is false.
EXPECT_FALSE(IsDisableVideoTrackPending()); EXPECT_FALSE(IsDisableVideoTrackPending());
// Should start background disable timer, but not disable immediately. // Should start background disable timer, but not disable immediately.
BackgroundPlayer(); BackgroundPlayer();
if (ShouldPausePlaybackWhenHidden()) { if (ShouldPausePlaybackWhenHidden()) {
EXPECT_FALSE(IsVideoTrackDisabled());
EXPECT_FALSE(IsDisableVideoTrackPending()); EXPECT_FALSE(IsDisableVideoTrackPending());
} else { } else {
// Testing IsVideoTrackDisabled() leads to flakyness even though there // Testing IsVideoTrackDisabled() alone leads to flakyness even though there
// should be a 10 minutes delay until it happens. Given that it doesn't // should be a 10 minutes delay until it happens. Given that it doesn't
// provides much of a benefit at the moment, this is being ignored. // provide much of a benefit at the moment, this is being ignored.
EXPECT_TRUE(IsDisableVideoTrackPending()); EXPECT_TRUE(IsDisableVideoTrackPending() || IsVideoTrackDisabled());
} }
} }
......
...@@ -252,13 +252,14 @@ class AesDecryptorTest : public testing::TestWithParam<TestType> { ...@@ -252,13 +252,14 @@ class AesDecryptorTest : public testing::TestWithParam<TestType> {
void SetUp() override { void SetUp() override {
if (GetParam() == TestType::kAesDecryptor) { if (GetParam() == TestType::kAesDecryptor) {
OnCdmCreated( OnCdmCreated(
new AesDecryptor(base::Bind(&MockCdmClient::OnSessionMessage, new AesDecryptor(
base::BindRepeating(&MockCdmClient::OnSessionMessage,
base::Unretained(&cdm_client_)), base::Unretained(&cdm_client_)),
base::Bind(&MockCdmClient::OnSessionClosed, base::BindRepeating(&MockCdmClient::OnSessionClosed,
base::Unretained(&cdm_client_)), base::Unretained(&cdm_client_)),
base::Bind(&MockCdmClient::OnSessionKeysChange, base::BindRepeating(&MockCdmClient::OnSessionKeysChange,
base::Unretained(&cdm_client_)), base::Unretained(&cdm_client_)),
base::Bind(&MockCdmClient::OnSessionExpirationUpdate, base::BindRepeating(&MockCdmClient::OnSessionExpirationUpdate,
base::Unretained(&cdm_client_))), base::Unretained(&cdm_client_))),
std::string()); std::string());
} else if (GetParam() == TestType::kCdmAdapter) { } else if (GetParam() == TestType::kCdmAdapter) {
...@@ -283,15 +284,16 @@ class AesDecryptorTest : public testing::TestWithParam<TestType> { ...@@ -283,15 +284,16 @@ class AesDecryptorTest : public testing::TestWithParam<TestType> {
std::unique_ptr<CdmAllocator> allocator(new SimpleCdmAllocator()); std::unique_ptr<CdmAllocator> allocator(new SimpleCdmAllocator());
std::unique_ptr<CdmAuxiliaryHelper> cdm_helper( std::unique_ptr<CdmAuxiliaryHelper> cdm_helper(
new MockCdmAuxiliaryHelper(std::move(allocator))); new MockCdmAuxiliaryHelper(std::move(allocator)));
CdmAdapter::Create(helper_->KeySystemName(), CdmAdapter::Create(
cdm_config, create_cdm_func, std::move(cdm_helper), helper_->KeySystemName(), cdm_config, create_cdm_func,
base::Bind(&MockCdmClient::OnSessionMessage, std::move(cdm_helper),
base::BindRepeating(&MockCdmClient::OnSessionMessage,
base::Unretained(&cdm_client_)), base::Unretained(&cdm_client_)),
base::Bind(&MockCdmClient::OnSessionClosed, base::BindRepeating(&MockCdmClient::OnSessionClosed,
base::Unretained(&cdm_client_)), base::Unretained(&cdm_client_)),
base::Bind(&MockCdmClient::OnSessionKeysChange, base::BindRepeating(&MockCdmClient::OnSessionKeysChange,
base::Unretained(&cdm_client_)), base::Unretained(&cdm_client_)),
base::Bind(&MockCdmClient::OnSessionExpirationUpdate, base::BindRepeating(&MockCdmClient::OnSessionExpirationUpdate,
base::Unretained(&cdm_client_)), base::Unretained(&cdm_client_)),
base::BindOnce(&AesDecryptorTest::OnCdmCreated, base::BindOnce(&AesDecryptorTest::OnCdmCreated,
base::Unretained(this))); base::Unretained(this)));
......
...@@ -226,7 +226,7 @@ CdmAdapter::CdmAdapter( ...@@ -226,7 +226,7 @@ CdmAdapter::CdmAdapter(
DCHECK(session_expiration_update_cb_); DCHECK(session_expiration_update_cb_);
helper_->SetFileReadCB( helper_->SetFileReadCB(
base::Bind(&CdmAdapter::OnFileRead, weak_factory_.GetWeakPtr())); base::BindRepeating(&CdmAdapter::OnFileRead, weak_factory_.GetWeakPtr()));
} }
CdmAdapter::~CdmAdapter() { CdmAdapter::~CdmAdapter() {
......
...@@ -140,15 +140,16 @@ class CdmAdapterTestBase : public testing::Test, ...@@ -140,15 +140,16 @@ class CdmAdapterTestBase : public testing::Test,
std::unique_ptr<StrictMock<MockCdmAuxiliaryHelper>> cdm_helper( std::unique_ptr<StrictMock<MockCdmAuxiliaryHelper>> cdm_helper(
new StrictMock<MockCdmAuxiliaryHelper>(std::move(allocator))); new StrictMock<MockCdmAuxiliaryHelper>(std::move(allocator)));
cdm_helper_ = cdm_helper.get(); cdm_helper_ = cdm_helper.get();
CdmAdapter::Create(GetKeySystemName(), cdm_config, GetCreateCdmFunc(), CdmAdapter::Create(
GetKeySystemName(), cdm_config, GetCreateCdmFunc(),
std::move(cdm_helper), std::move(cdm_helper),
base::Bind(&MockCdmClient::OnSessionMessage, base::BindRepeating(&MockCdmClient::OnSessionMessage,
base::Unretained(&cdm_client_)), base::Unretained(&cdm_client_)),
base::Bind(&MockCdmClient::OnSessionClosed, base::BindRepeating(&MockCdmClient::OnSessionClosed,
base::Unretained(&cdm_client_)), base::Unretained(&cdm_client_)),
base::Bind(&MockCdmClient::OnSessionKeysChange, base::BindRepeating(&MockCdmClient::OnSessionKeysChange,
base::Unretained(&cdm_client_)), base::Unretained(&cdm_client_)),
base::Bind(&MockCdmClient::OnSessionExpirationUpdate, base::BindRepeating(&MockCdmClient::OnSessionExpirationUpdate,
base::Unretained(&cdm_client_)), base::Unretained(&cdm_client_)),
base::BindOnce(&CdmAdapterTestBase::OnCdmCreated, base::BindOnce(&CdmAdapterTestBase::OnCdmCreated,
base::Unretained(this), expected_result)); base::Unretained(this), expected_result));
......
...@@ -327,10 +327,13 @@ ClearKeyCdm::ClearKeyCdm(HostInterface* host, const std::string& key_system) ...@@ -327,10 +327,13 @@ ClearKeyCdm::ClearKeyCdm(HostInterface* host, const std::string& key_system)
cdm_host_proxy_(new CdmHostProxyImpl<HostInterface>(host)), cdm_host_proxy_(new CdmHostProxyImpl<HostInterface>(host)),
cdm_(new ClearKeyPersistentSessionCdm( cdm_(new ClearKeyPersistentSessionCdm(
cdm_host_proxy_.get(), cdm_host_proxy_.get(),
base::Bind(&ClearKeyCdm::OnSessionMessage, base::Unretained(this)), base::BindRepeating(&ClearKeyCdm::OnSessionMessage,
base::Bind(&ClearKeyCdm::OnSessionClosed, base::Unretained(this)), base::Unretained(this)),
base::Bind(&ClearKeyCdm::OnSessionKeysChange, base::Unretained(this)), base::BindRepeating(&ClearKeyCdm::OnSessionClosed,
base::Bind(&ClearKeyCdm::OnSessionExpirationUpdate, base::Unretained(this)),
base::BindRepeating(&ClearKeyCdm::OnSessionKeysChange,
base::Unretained(this)),
base::BindRepeating(&ClearKeyCdm::OnSessionExpirationUpdate,
base::Unretained(this)))), base::Unretained(this)))),
key_system_(key_system) { key_system_(key_system) {
DCHECK(g_is_cdm_module_initialized); DCHECK(g_is_cdm_module_initialized);
......
...@@ -99,9 +99,9 @@ ClearKeyPersistentSessionCdm::ClearKeyPersistentSessionCdm( ...@@ -99,9 +99,9 @@ ClearKeyPersistentSessionCdm::ClearKeyPersistentSessionCdm(
session_message_cb_(session_message_cb), session_message_cb_(session_message_cb),
session_closed_cb_(session_closed_cb) { session_closed_cb_(session_closed_cb) {
cdm_ = base::MakeRefCounted<AesDecryptor>( cdm_ = base::MakeRefCounted<AesDecryptor>(
base::Bind(&ClearKeyPersistentSessionCdm::OnSessionMessage, base::BindRepeating(&ClearKeyPersistentSessionCdm::OnSessionMessage,
weak_factory_.GetWeakPtr()), weak_factory_.GetWeakPtr()),
base::Bind(&ClearKeyPersistentSessionCdm::OnSessionClosed, base::BindRepeating(&ClearKeyPersistentSessionCdm::OnSessionClosed,
weak_factory_.GetWeakPtr()), weak_factory_.GetWeakPtr()),
session_keys_change_cb, session_expiration_update_cb); session_keys_change_cb, session_expiration_update_cb);
} }
...@@ -129,7 +129,7 @@ void ClearKeyPersistentSessionCdm::CreateSessionAndGenerateRequest( ...@@ -129,7 +129,7 @@ void ClearKeyPersistentSessionCdm::CreateSessionAndGenerateRequest(
// Since it's a persistent session, we need to save the session ID after // Since it's a persistent session, we need to save the session ID after
// it's been created. // it's been created.
new_promise = std::make_unique<NewPersistentSessionCdmPromise>( new_promise = std::make_unique<NewPersistentSessionCdmPromise>(
base::Bind(&ClearKeyPersistentSessionCdm::AddPersistentSession, base::BindOnce(&ClearKeyPersistentSessionCdm::AddPersistentSession,
weak_factory_.GetWeakPtr()), weak_factory_.GetWeakPtr()),
std::move(promise)); std::move(promise));
} }
......
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