Use enum instead of string in MessageLoopFactory::GetMessageLoop*

media/base/message_loop_factory.cc

Index: media/base/message_loop_factory.cc
diff --git a/media/base/message_loop_factory.cc b/media/base/message_loop_factory.cc
index 22ec96c9a21cc166adf2a405d7c39201a18fea3f..49a9d64c87578fb69f801196a68a7ecc4d560ad4 100644
--- a/media/base/message_loop_factory.cc
+++ b/media/base/message_loop_factory.cc
@@ -8,6 +8,17 @@
 
 namespace media {
 
+struct MessageLoopTypeNamePair {
+  MessageLoopFactory::Type type;
+  const char* name;
+};
+
+static const MessageLoopTypeNamePair kMessageLoopTypeToNameMapping[] = {
+  { MessageLoopFactory::kAudioDecoder, "AudioDecoderThread" },
+  { MessageLoopFactory::kVideoDecoder, "VideoDecoderThread" },
+  { MessageLoopFactory::kPipeline, "PipelineThread" }
+};
+
 MessageLoopFactory::MessageLoopFactory() {}
 
 MessageLoopFactory::~MessageLoopFactory() {
@@ -20,27 +31,28 @@ MessageLoopFactory::~MessageLoopFactory() {
   threads_.clear();
 }
 
-MessageLoop* MessageLoopFactory::GetMessageLoop(const std::string& name) {
-  return GetThread(name)->message_loop();
+scoped_refptr<base::MessageLoopProxy> MessageLoopFactory::GetMessageLoop(
+    Type type) {
+  return GetThread(type)->message_loop_proxy();
 }
 
-scoped_refptr<base::MessageLoopProxy>
-MessageLoopFactory::GetMessageLoopProxy(const std::string& name) {
-  return GetThread(name)->message_loop_proxy();
-}
-
-base::Thread* MessageLoopFactory::GetThread(const std::string& name) {
-  DCHECK(!name.empty());
-
+base::Thread* MessageLoopFactory::GetThread(Type type) {
   base::AutoLock auto_lock(lock_);
   for (ThreadList::iterator it = threads_.begin(); it != threads_.end(); ++it) {
-    if (it->first == name)
+    if (it->first == type)
       return it->second;
   }
 
+  std::string name;
+  for (size_t i = 0; i < arraysize(kMessageLoopTypeToNameMapping); ++i) {
+    if (kMessageLoopTypeToNameMapping[i].type == type)
+      name = kMessageLoopTypeToNameMapping[i].name;
+  }
+  DCHECK(!name.empty());
+
   base::Thread* thread = new base::Thread(name.c_str());

[Ami GONE FROM CHROMIUM, 2012/08/10 04:38:06]

I think you're working too hard on this :)
l.16-21 and l45-57 can be replaced with:

const char* name = NULL;
switch (type) {
 case kAudioDecoder: 
  name = "AudioDecoderThread"; 
  break;
 case kVideoDecoder: 
  name = "VideoDecoderThread"; 
  break;
 case kPipeline:
  name = "PipelineThread"; 
  break;
}
base::Thread* thread = new base::Thread(name);
CHECK(thread->Start()) << "Failed to start thread: " << name;
threads_.push_back(std::make_pair(type, thread));
return thread;


(the benefits are:
1) compiler checks all the types are handled
2) the literal names are present exactly where they are used
3) it's simpler/shorter)

[scherkus (not reviewing), 2012/08/10 17:00:09]

+1

[xhwang, 2012/08/10 19:33:33]

Done.

   CHECK(thread->Start()) << "Failed to start thread: " << name;
-  threads_.push_back(std::make_pair(name, thread));
+  threads_.push_back(std::make_pair(type, thread));
   return thread;
 }