Refactored MediaStreamImpl to be a RenderViewObserver as the other delegates in RenderViewImpl.

Reenabled the MediaStreamImpl unit tests.

This supersedes https://chromiumcodereview.appspot.com/9368022/

BUG=112408 
TEST=unit test

Review URL: http://codereview.chromium.org/10108016

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@133417 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 66 insertions, 72 deletions

diff --git a/content/renderer/media/media_stream_impl.cc b/content/renderer/media/media_stream_impl.cc
index fceeb40..ee4af5d 100644
--- a/content/renderer/media/media_stream_impl.cc
+++ b/content/renderer/media/media_stream_impl.cc
@@ -74,11 +74,13 @@ static std::string ExtractManagerStreamLabel(
 int MediaStreamImpl::next_request_id_ = 0;
 
 MediaStreamImpl::MediaStreamImpl(
+    content::RenderView* render_view,
     MediaStreamDispatcher* media_stream_dispatcher,
     content::P2PSocketDispatcher* p2p_socket_dispatcher,
     VideoCaptureImplManager* vc_manager,
     MediaStreamDependencyFactory* dependency_factory)
-    : dependency_factory_(dependency_factory),
+    : content::RenderViewObserver(render_view),
+      dependency_factory_(dependency_factory),
       media_stream_dispatcher_(media_stream_dispatcher),
       p2p_socket_dispatcher_(p2p_socket_dispatcher),
       network_manager_(NULL),
@@ -410,7 +412,7 @@ bool MediaStreamImpl::EnsurePeerConnectionFactory() {
     base::WaitableEvent event(true, false);
     chrome_worker_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
         &MediaStreamImpl::InitializeWorkerThread,
-        this,
+        base::Unretained(this),
         &worker_thread_,
         &event));
     event.Wait();
@@ -421,7 +423,7 @@ bool MediaStreamImpl::EnsurePeerConnectionFactory() {
     base::WaitableEvent event(true, false);
     chrome_worker_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
           &MediaStreamImpl::CreateIpcNetworkManagerOnWorkerThread,
-          this,
+          base::Unretained(this),
           &event));
     event.Wait();
   }
diff --git a/content/renderer/media/media_stream_impl.h b/content/renderer/media/media_stream_impl.h
index edd0564..c949a64 100644
--- a/content/renderer/media/media_stream_impl.h
+++ b/content/renderer/media/media_stream_impl.h
@@ -20,6 +20,7 @@
 #include "base/threading/non_thread_safe.h"
 #include "base/threading/thread.h"
 #include "content/common/content_export.h"
+#include "content/public/renderer/render_view_observer.h"
 #include "content/renderer/media/media_stream_dispatcher_eventhandler.h"
 #include "content/renderer/media/rtc_video_decoder.h"
 #include "third_party/libjingle/source/talk/app/webrtc/mediastream.h"
@@ -60,15 +61,19 @@ class RTCVideoDecoder;
 // MediaStreamManager (via MediaStreamDispatcher and MediaStreamDispatcherHost)
 // in the browser process. It must be created, called and destroyed on the
 // render thread.
+// MediaStreamImpl have weak pointers to a P2PSocketDispatcher and a
+// MediaStreamDispatcher. These objects are also RenderViewObservers.
+// MediaStreamImpl must be deleted before the P2PSocketDispatcher.
 class CONTENT_EXPORT MediaStreamImpl
-    : NON_EXPORTED_BASE(public WebKit::WebUserMediaClient),
+    : public content::RenderViewObserver,
+      NON_EXPORTED_BASE(public WebKit::WebUserMediaClient),
       NON_EXPORTED_BASE(public webkit_media::MediaStreamClient),
       public MediaStreamDispatcherEventHandler,
-      NON_EXPORTED_BASE(public base::NonThreadSafe),
-      public base::RefCountedThreadSafe<MediaStreamImpl>,
-      public base::SupportsWeakPtr<MediaStreamImpl> {
+      public base::SupportsWeakPtr<MediaStreamImpl>,
+      NON_EXPORTED_BASE(public base::NonThreadSafe) {
  public:
   MediaStreamImpl(
+      content::RenderView* render_view,
       MediaStreamDispatcher* media_stream_dispatcher,
       content::P2PSocketDispatcher* p2p_socket_dispatcher,
       VideoCaptureImplManager* vc_manager,
@@ -171,6 +176,7 @@ class CONTENT_EXPORT MediaStreamImpl
   content::P2PSocketDispatcher* p2p_socket_dispatcher_;
 
   // We own network_manager_, must be deleted on the worker thread.
+  // The network manager uses |p2p_socket_dispatcher_|.
   content::IpcNetworkManager* network_manager_;
 
   scoped_ptr<content::IpcPacketSocketFactory> socket_factory_;
diff --git a/content/renderer/media/media_stream_impl_unittest.cc b/content/renderer/media/media_stream_impl_unittest.cc
index f00aa04..a821d14 100644
--- a/content/renderer/media/media_stream_impl_unittest.cc
+++ b/content/renderer/media/media_stream_impl_unittest.cc
@@ -17,92 +17,80 @@
 #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebPeerConnection00Handler.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/platform/WebPeerConnectionHandler.h"
 
-// Disabled due to http://crbug.com/112408 .
-TEST(MediaStreamImplTest, DISABLED_Basic) {
-  MessageLoop loop;
+class MediaStreamImplTest : public ::testing::Test {
+ public:
+  void SetUp() {
+    // Create our test object.
+    ms_dispatcher_.reset(new MockMediaStreamDispatcher());
+    p2p_socket_dispatcher_.reset(new content::P2PSocketDispatcher(NULL));
+    scoped_refptr<VideoCaptureImplManager> vc_manager(
+        new VideoCaptureImplManager());
+    MockMediaStreamDependencyFactory* dependency_factory =
+        new MockMediaStreamDependencyFactory();
+    ms_impl_.reset(new MediaStreamImpl(NULL,
+                                       ms_dispatcher_.get(),
+                                       p2p_socket_dispatcher_.get(),
+                                       vc_manager.get(),
+                                       dependency_factory));
+  }
 
-  // Create our test object.
-  scoped_ptr<MockMediaStreamDispatcher> ms_dispatcher(
-      new MockMediaStreamDispatcher());
-  scoped_ptr<content::P2PSocketDispatcher> p2p_socket_dispatcher(
-      new content::P2PSocketDispatcher(NULL));
-  scoped_refptr<VideoCaptureImplManager> vc_manager(
-      new VideoCaptureImplManager());
-  MockMediaStreamDependencyFactory* dependency_factory =
-      new MockMediaStreamDependencyFactory();
-  scoped_refptr<MediaStreamImpl> ms_impl(new MediaStreamImpl(
-      ms_dispatcher.get(),
-      p2p_socket_dispatcher.get(),
-      vc_manager.get(),
-      dependency_factory));
+ protected:
+  MessageLoop loop_;
+  scoped_ptr<MockMediaStreamDispatcher> ms_dispatcher_;
+  scoped_ptr<content::P2PSocketDispatcher> p2p_socket_dispatcher_;
+  scoped_ptr<MediaStreamImpl> ms_impl_;
+};
 
+TEST_F(MediaStreamImplTest, Basic) {
   // TODO(grunell): Add tests for WebKit::WebUserMediaClient and
   // MediaStreamDispatcherEventHandler implementations.
 
   WebKit::MockWebPeerConnectionHandlerClient client;
   WebKit::WebPeerConnectionHandler* pc_handler =
-      ms_impl->CreatePeerConnectionHandler(&client);
-  EXPECT_EQ(1u, ms_impl->peer_connection_handlers_.size());
+      ms_impl_->CreatePeerConnectionHandler(&client);
+  EXPECT_EQ(1u, ms_impl_->peer_connection_handlers_.size());
 
   // Delete PC handler explicitly after closing to mimic WebKit behavior.
-  ms_impl->ClosePeerConnection(
+  ms_impl_->ClosePeerConnection(
       static_cast<PeerConnectionHandler*>(pc_handler));
-  EXPECT_TRUE(ms_impl->peer_connection_handlers_.empty());
+  EXPECT_TRUE(ms_impl_->peer_connection_handlers_.empty());
   delete pc_handler;
 
   WebKit::MockWebPeerConnection00HandlerClient client_jsep;
   WebKit::WebPeerConnection00Handler* pc_handler_jsep =
-      ms_impl->CreatePeerConnectionHandlerJsep(&client_jsep);
-  EXPECT_EQ(1u, ms_impl->peer_connection_handlers_.size());
+      ms_impl_->CreatePeerConnectionHandlerJsep(&client_jsep);
+  EXPECT_EQ(1u, ms_impl_->peer_connection_handlers_.size());
 
   // Delete PC handler explicitly after closing to mimic WebKit behavior.
-  ms_impl->ClosePeerConnection(
+  ms_impl_->ClosePeerConnection(
       static_cast<PeerConnectionHandlerJsep*>(pc_handler_jsep));
-  EXPECT_TRUE(ms_impl->peer_connection_handlers_.empty());
+  EXPECT_TRUE(ms_impl_->peer_connection_handlers_.empty());
   delete pc_handler_jsep;
 }
 
-// Disabled due to http://crbug.com/112408 .
-TEST(MediaStreamImplTest, DISABLED_MultiplePeerConnections) {
-  MessageLoop loop;
-
-  // Create our test object.
-  scoped_ptr<MockMediaStreamDispatcher> ms_dispatcher(
-      new MockMediaStreamDispatcher());
-  scoped_ptr<content::P2PSocketDispatcher> p2p_socket_dispatcher(
-      new content::P2PSocketDispatcher(NULL));
-  scoped_refptr<VideoCaptureImplManager> vc_manager(
-      new VideoCaptureImplManager());
-  MockMediaStreamDependencyFactory* dependency_factory =
-      new MockMediaStreamDependencyFactory();
-  scoped_refptr<MediaStreamImpl> ms_impl(new MediaStreamImpl(
-      ms_dispatcher.get(),
-      p2p_socket_dispatcher.get(),
-      vc_manager.get(),
-      dependency_factory));
-
+TEST_F(MediaStreamImplTest, MultiplePeerConnections) {
   // TODO(grunell): Add tests for WebKit::WebUserMediaClient and
   // MediaStreamDispatcherEventHandler implementations.
 
   WebKit::MockWebPeerConnectionHandlerClient client;
   WebKit::WebPeerConnectionHandler* pc_handler =
-      ms_impl->CreatePeerConnectionHandler(&client);
-  EXPECT_EQ(1u, ms_impl->peer_connection_handlers_.size());
+      ms_impl_->CreatePeerConnectionHandler(&client);
+  EXPECT_EQ(1u, ms_impl_->peer_connection_handlers_.size());
 
   WebKit::MockWebPeerConnection00HandlerClient client_jsep;
   WebKit::WebPeerConnection00Handler* pc_handler_jsep =
-      ms_impl->CreatePeerConnectionHandlerJsep(&client_jsep);
-  EXPECT_EQ(2u, ms_impl->peer_connection_handlers_.size());
+      ms_impl_->CreatePeerConnectionHandlerJsep(&client_jsep);
+  EXPECT_EQ(2u, ms_impl_->peer_connection_handlers_.size());
 
   // Delete PC handler explicitly after closing to mimic WebKit behavior.
-  ms_impl->ClosePeerConnection(
+  ms_impl_->ClosePeerConnection(
       static_cast<PeerConnectionHandler*>(pc_handler));
-  EXPECT_EQ(1u, ms_impl->peer_connection_handlers_.size());
+  EXPECT_EQ(1u, ms_impl_->peer_connection_handlers_.size());
   delete pc_handler;
 
   // Delete PC handler explicitly after closing to mimic WebKit behavior.
-  ms_impl->ClosePeerConnection(
+  ms_impl_->ClosePeerConnection(
       static_cast<PeerConnectionHandlerJsep*>(pc_handler_jsep));
-  EXPECT_TRUE(ms_impl->peer_connection_handlers_.empty());
+  EXPECT_TRUE(ms_impl_->peer_connection_handlers_.empty());
   delete pc_handler_jsep;
 }
diff --git a/content/renderer/media/mock_media_stream_impl.cc b/content/renderer/media/mock_media_stream_impl.cc
index 8ce04de..7066a1d 100644
--- a/content/renderer/media/mock_media_stream_impl.cc
+++ b/content/renderer/media/mock_media_stream_impl.cc
@@ -9,7 +9,7 @@
 #include "content/renderer/media/rtc_video_decoder.h"
 
 MockMediaStreamImpl::MockMediaStreamImpl()
-    : MediaStreamImpl(NULL, NULL, NULL, NULL) {
+    : MediaStreamImpl(NULL, NULL, NULL, NULL, NULL) {
 }
 
 MockMediaStreamImpl::~MockMediaStreamImpl() {}
diff --git a/content/renderer/media/peer_connection_handler_jsep_unittest.cc b/content/renderer/media/peer_connection_handler_jsep_unittest.cc
index 63a0f26..3f35680 100644
--- a/content/renderer/media/peer_connection_handler_jsep_unittest.cc
+++ b/content/renderer/media/peer_connection_handler_jsep_unittest.cc
@@ -41,7 +41,7 @@ TEST(PeerConnectionHandlerJsepTest, Basic) {
 
   scoped_ptr<WebKit::MockWebPeerConnection00HandlerClient> mock_client(
       new WebKit::MockWebPeerConnection00HandlerClient());
-  scoped_refptr<MockMediaStreamImpl> mock_ms_impl(new MockMediaStreamImpl());
+  scoped_ptr<MockMediaStreamImpl> mock_ms_impl(new MockMediaStreamImpl());
   scoped_ptr<MockMediaStreamDependencyFactory> mock_dependency_factory(
       new MockMediaStreamDependencyFactory());
   mock_dependency_factory->CreatePeerConnectionFactory(NULL,
diff --git a/content/renderer/media/peer_connection_handler_unittest.cc b/content/renderer/media/peer_connection_handler_unittest.cc
index 535877f..7703e34 100644
--- a/content/renderer/media/peer_connection_handler_unittest.cc
+++ b/content/renderer/media/peer_connection_handler_unittest.cc
@@ -58,7 +58,7 @@ TEST(PeerConnectionHandlerTest, Basic) {
 
   scoped_ptr<WebKit::MockWebPeerConnectionHandlerClient> mock_client(
       new WebKit::MockWebPeerConnectionHandlerClient());
-  scoped_refptr<MockMediaStreamImpl> mock_ms_impl(new MockMediaStreamImpl());
+  scoped_ptr<MockMediaStreamImpl> mock_ms_impl(new MockMediaStreamImpl());
   scoped_ptr<MockMediaStreamDependencyFactory> mock_dependency_factory(
       new MockMediaStreamDependencyFactory());
   mock_dependency_factory->CreatePeerConnectionFactory(NULL,
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
index bcd3e8d..0d6467d 100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -465,6 +465,7 @@ RenderViewImpl::RenderViewImpl(
       input_tag_speech_dispatcher_(NULL),
       device_orientation_dispatcher_(NULL),
       media_stream_dispatcher_(NULL),
+      media_stream_impl_(NULL),
       p2p_socket_dispatcher_(NULL),
       devtools_agent_(NULL),
       renderer_accessibility_(NULL),
@@ -593,10 +594,6 @@ RenderViewImpl::~RenderViewImpl() {
     DCHECK_NE(this, it->second) << "Failed to call Close?";
 #endif
 
-  // MediaStreamImpl holds weak references to RenderViewObserver objects,
-  // ensure it's deleted before the observers.
-  media_stream_impl_ = NULL;
-
   FOR_EACH_OBSERVER(RenderViewObserver, observers_, RenderViewGone());
   FOR_EACH_OBSERVER(RenderViewObserver, observers_, OnDestruct());
 }
@@ -658,7 +655,7 @@ RenderViewImpl* RenderViewImpl::Create(
 WebPeerConnectionHandler* RenderViewImpl::CreatePeerConnectionHandler(
     WebPeerConnectionHandlerClient* client) {
   EnsureMediaStreamImpl();
-  if (!media_stream_impl_.get())
+  if (!media_stream_impl_)
     return NULL;
   return media_stream_impl_->CreatePeerConnectionHandler(client);
 }
@@ -666,7 +663,7 @@ WebPeerConnectionHandler* RenderViewImpl::CreatePeerConnectionHandler(
 WebPeerConnection00Handler* RenderViewImpl::CreatePeerConnectionHandlerJsep(
     WebPeerConnection00HandlerClient* client) {
   EnsureMediaStreamImpl();
-  if (!media_stream_impl_.get())
+  if (!media_stream_impl_)
     return NULL;
   return media_stream_impl_->CreatePeerConnectionHandlerJsep(client);
 }
@@ -2218,7 +2215,7 @@ WebMediaPlayer* RenderViewImpl::createMediaPlayer(
   webkit_media::WebMediaPlayerImpl* media_player =
       content::GetContentClient()->renderer()->OverrideCreateWebMediaPlayer(
           this, frame, client, AsWeakPtr(), collection, audio_source_provider,
-          message_loop_factory, media_stream_impl_.get(), render_media_log);
+          message_loop_factory, media_stream_impl_, render_media_log);
 #if defined(OS_ANDROID)
   // TODO(qinmin): Implement for android.
   // http://crbug.com/113218
@@ -2226,7 +2223,7 @@ WebMediaPlayer* RenderViewImpl::createMediaPlayer(
   if (!media_player) {
     media_player = new webkit_media::WebMediaPlayerImpl(
         frame, client, AsWeakPtr(), collection, audio_source_provider,
-        message_loop_factory, media_stream_impl_.get(), render_media_log);
+        message_loop_factory, media_stream_impl_, render_media_log);
   }
 #endif
   return media_player;
@@ -3300,9 +3297,10 @@ void RenderViewImpl::EnsureMediaStreamImpl() {
   if (!media_stream_dispatcher_)
     media_stream_dispatcher_ = new MediaStreamDispatcher(this);
 
-  if (!media_stream_impl_.get()) {
+  if (!media_stream_impl_) {
     MediaStreamDependencyFactory* factory = new MediaStreamDependencyFactory();
     media_stream_impl_ = new MediaStreamImpl(
+        this,
         media_stream_dispatcher_,
         p2p_socket_dispatcher_,
         RenderThreadImpl::current()->video_capture_impl_manager(),
diff --git a/content/renderer/render_view_impl.h b/content/renderer/render_view_impl.h
index d6128a2..9be49ba 100644
--- a/content/renderer/render_view_impl.h
+++ b/content/renderer/render_view_impl.h
@@ -1223,7 +1223,7 @@ class RenderViewImpl : public RenderWidget,
   MediaStreamDispatcher* media_stream_dispatcher_;
 
   // MediaStreamImpl attached to this view; lazily initialized.
-  scoped_refptr<MediaStreamImpl> media_stream_impl_;
+  MediaStreamImpl* media_stream_impl_;
 
   // Dispatches all P2P socket used by the renderer.
   content::P2PSocketDispatcher* p2p_socket_dispatcher_;