Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1707)

Unified Diff: Source/web/WebMediaPlayerClientImpl.cpp

Issue 302093011: Oilpan: move the MediaPlayer and MediaPlayerClient objects to the heap. (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master
Patch Set: Have MediaController weakly track its media elements Created 6 years, 7 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: Source/web/WebMediaPlayerClientImpl.cpp
diff --git a/Source/web/WebMediaPlayerClientImpl.cpp b/Source/web/WebMediaPlayerClientImpl.cpp
index ce4a9341a33b358954fe4897dfa37803f1a51bbc..9363fcc4701022cb3d69969702f1271c60a822c2 100644
--- a/Source/web/WebMediaPlayerClientImpl.cpp
+++ b/Source/web/WebMediaPlayerClientImpl.cpp
@@ -70,10 +70,52 @@ WebMediaPlayer* WebMediaPlayerClientImpl::webMediaPlayer() const
WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl()
{
+#if ENABLE(OILPAN)
+#if ENABLE(WEB_AUDIO)
+ if (audioSourceProvider())
haraken 2014/06/03 09:19:47 I think this cannot be 0.
+ audioSourceProvider()->setClient(0);
+#endif
+ // See detach() comment why we are clearing this reference.
+ m_client.clear();
+
// Explicitly destroy the WebMediaPlayer to allow verification of tear down.
m_webMediaPlayer.clear();
+#else
+ // Check that detach has been called. It should always have been done
+ // with Oilpan not enabled.
+ ASSERT(!m_webMediaPlayer);
+#endif
+}
+void WebMediaPlayerClientImpl::detach()
+{
+ // FIXME: This ad-hoc notification call to the supplement is out
+ // here to avoid a core/html/ -> modules/encryptedmedia/
+ // dependency. Consider adding a detach notification from the
+ // media element supplementable to its supplements instead to
+ // avoid involving this client object as a third party.
HTMLMediaElementEncryptedMedia::playerDestroyed(mediaElement());
+
+ // For Oilpan, the client object is on the heap and cannot be safely
+ // accessed by WebMediaPlayerClientImpl when it later is GCed.
+ // Clear out the client reference early.
+ //
+ // FIXME: the browser-side WebMediaPlayer implementation performs a
+ // twisty callback to clear the WebLayer (i.e., calls setWebLayer(0))
+ // when being destructed. This resetting is now handled by the
+ // media element directly when detaching. Follow up and support
+ // clearing of the WebMediaPlayer's client reference, so as to
+ // avoid that callback.
+ m_client.clear();
+
+ // Explicitly destroy the WebMediaPlayer to allow verification of tear down.
+ m_webMediaPlayer.clear();
+}
+
+void WebMediaPlayerClientImpl::trace(Visitor* visitor)
+{
+ visitor->trace(m_client);
+ MediaPlayer::trace(visitor);
}
void WebMediaPlayerClientImpl::networkStateChanged()
@@ -143,6 +185,13 @@ void WebMediaPlayerClientImpl::keyNeeded(const WebString& contentType, const uns
void WebMediaPlayerClientImpl::setWebLayer(blink::WebLayer* layer)
{
+ if (!m_client) {
+ // Only expect the WebLayer to be cleared when finalizing.
+ // FIXME: switch to assume a non-zero layer argument; see
+ // detach() comment.
+ ASSERT(!layer);
+ return;
+ }
m_client->mediaPlayerSetWebLayer(layer);
}
@@ -367,9 +416,9 @@ AudioSourceProvider* WebMediaPlayerClientImpl::audioSourceProvider()
}
#endif
-PassOwnPtr<MediaPlayer> WebMediaPlayerClientImpl::create(MediaPlayerClient* client)
+PassOwnPtrWillBeRawPtr<MediaPlayer> WebMediaPlayerClientImpl::create(MediaPlayerClient* client)
{
- return adoptPtr(new WebMediaPlayerClientImpl(client));
+ return adoptPtrWillBeNoop(new WebMediaPlayerClientImpl(client));
}
#if OS(ANDROID)
@@ -422,7 +471,7 @@ WebMediaPlayerClientImpl::WebMediaPlayerClientImpl(MediaPlayerClient* client)
WebCore::HTMLMediaElement& WebMediaPlayerClientImpl::mediaElement() const
{
- return *static_cast<HTMLMediaElement*>(m_client);
+ return *static_cast<HTMLMediaElement*>(m_client.get());
}
#if ENABLE(WEB_AUDIO)
@@ -447,8 +496,11 @@ void WebMediaPlayerClientImpl::AudioSourceProviderImpl::setClient(AudioSourcePro
else
m_client.clear();
- if (m_webAudioSourceProvider)
+ if (m_webAudioSourceProvider) {
m_webAudioSourceProvider->setClient(m_client.get());
+ if (!m_client)
+ m_webAudioSourceProvider = 0;
haraken 2014/06/03 09:19:47 Having this code looks harmless, but I still don't
sof 2014/06/03 16:35:31 m_webAudioSourceProvider is something m_webMediaPl
+ }
}
void WebMediaPlayerClientImpl::AudioSourceProviderImpl::provideInput(AudioBus* bus, size_t framesToProcess)
« Source/platform/graphics/media/MediaPlayer.h ('K') | « Source/web/WebMediaPlayerClientImpl.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698