Fold media::MessageLoopFactoryImpl into media::MessageLoopFactory.

media/base/message_loop_factory.cc

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/base/message_loop_factory.h"
 
+#include "base/threading/thread.h"
+
 namespace media {
 
-MessageLoopFactory::~MessageLoopFactory() {}
+MessageLoopFactory::MessageLoopFactory() {}
+
+MessageLoopFactory::~MessageLoopFactory() {
+  base::AutoLock auto_lock(lock_);

[Ami GONE FROM CHROMIUM, 2012/03/05 23:20:15]

Locking in the dtor is a pretty good indicator that things are borked; if
another method invocation races with this, then merely holding it off until the
dtor completes will result in that method blowing up (attempting to use a
just-deleted |*this|).

[scherkus (not reviewing), 2012/03/05 23:29:35]

I don't think things are borked rather coded defensively (but acolwell can chime
in seeing as he wrote the initial CL!)

[acolwell GONE FROM CHROMIUM, 2012/03/05 23:53:20]

This was an attempt at defensive coding. Nothing should be calling GetThread()
when the destructor gets called. I agree that things will go BOOM if that does
happen though. The original intent was to just protect all thread_map_ accesses
with the lock.

On 2012/03/05 23:29:35, scherkus wrote:

+
+  for (ThreadMap::iterator iter = thread_map_.begin();
+       iter != thread_map_.end();
+       ++iter) {
+    base::Thread* thread = (*iter).second;
+
+    if (thread) {
+      thread->Stop();
+      delete thread;
+    }
+  }
+  thread_map_.clear();
+}
+
+MessageLoop* MessageLoopFactory::GetMessageLoop(const std::string& name) {
+  base::Thread* thread = GetThread(name);
+  return thread ? thread->message_loop() : NULL;
+}
+
+scoped_refptr<base::MessageLoopProxy>
+MessageLoopFactory::GetMessageLoopProxy(const std::string& name) {
+  base::Thread* thread = GetThread(name);
+  return thread ? thread->message_loop_proxy() : NULL;
+}
+
+base::Thread* MessageLoopFactory::GetThread(const std::string& name) {
+  if (name.empty())
+    return NULL;

[Ami GONE FROM CHROMIUM, 2012/03/05 23:20:15]

This isn't part of the doco'd interface of the class; DCHECK_NE(name, "") looks
better to me (and reduces the error checking you need to do in
GetMessageLoop{,Proxy}() above).

[scherkus (not reviewing), 2012/03/05 23:29:35]

"NULL is returned if |name| is an empty string" ??

although I'm not sure when/why we'd do that

[acolwell GONE FROM CHROMIUM, 2012/03/05 23:53:20]

DCHECK_NE() seems good to me. I think this was one of my earliest CLs so I was
much less DCHECK happy.

On 2012/03/05 23:29:35, scherkus wrote:

+
+  base::AutoLock auto_lock(lock_);
+
+  ThreadMap::iterator it = thread_map_.find(name);

[Ami GONE FROM CHROMIUM, 2012/03/05 23:20:15]

We don't actually make use of this functionality anywhere, so IMO this whole
class should be dropped in favor of creating threads at GetThreads() call-sites
and passing their ownership to the objects that live on them.

[scherkus (not reviewing), 2012/03/05 23:29:35]

yup but ideally going further by not creating additional threads per object
(that was the original intent of MLF)

[acolwell GONE FROM CHROMIUM, 2012/03/05 23:53:20]

So the original intent was to share threads across multiple audio & video
decoders. Obviously that functionality hasn't been implemented over the last
year. I'm open to replacing this with something better. 

On 2012/03/05 23:29:35, scherkus wrote:

+  if (it != thread_map_.end())
+    return (*it).second;
+
+  base::Thread* thread = new base::Thread(name.c_str());
+  CHECK(thread->Start()) << "Failed to start thread: " << name;
+  thread_map_[name] = thread;
+  return thread;
+}
 
 }  // namespace media