Lazily initialize MessageLoop for faster thread startup

base/message_loop/message_loop.h

Index: base/message_loop/message_loop.h
diff --git a/base/message_loop/message_loop.h b/base/message_loop/message_loop.h
index fd7596a79204ed89291f32b2f9135108a73b2882..b20baf03559dd8c7094d6f6bab6b65b5226c4ab7 100644
--- a/base/message_loop/message_loop.h
+++ b/base/message_loop/message_loop.h
@@ -109,12 +109,44 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate {
 #endif  // defined(OS_ANDROID)
   };
 
+  // Init options.
+  struct BASE_EXPORT InitOptions {
+    using MessagePumpFactory = Callback<scoped_ptr<MessagePump>()>;

[danakj, 2015/04/24 20:54:39]

nit: no () for the type name

[kinuko, 2015/04/27 16:36:04]

It's for Callback typedef, it seems we need it.

+    InitOptions();
+    ~InitOptions();
+
+    // Specify the type of message loop.

[danakj, 2015/04/24 20:54:39]

Comments for "why" not for "what", this doesn't say anything that l-120 doesn't
already say.

+    // This is ignored if message_pump_factory.is_null() is false.

[danakj, 2015/04/24 20:54:39]

"This is only used if message_pump_factory is null" is easier to parse IMO.

Can you put it below message_pump_factory, since it depends on it?

+    MessageLoop::Type message_loop_type;
+
+    // Specify timer slack for the message loop.

[danakj, 2015/04/24 20:54:39]

Can you say what timer slack is, instead of just repeating the name in the
comment?

+    TimerSlack timer_slack;
+
+    // Used to create the MessagePump for the MessageLoop.
+    // If message_pump_factory.is_null() is true, then a MessagePump appropriate

[danakj, 2015/04/24 20:54:39]

If message_pump_factory is null, then

+    // for |message_loop_type| is created. Setting this forces the
+    // MessageLoop::Type to TYPE_CUSTOM.
+    MessagePumpFactory message_pump_factory;
+  };
+
+  // Creates a MessageLoop with |options|. It is valid to create a new
+  // message loop on a different thread from the final 'current' thread,

[Nico, 2015/04/24 21:31:16]

This sounds confusing to me. Maybe

"It is valid to create a new message loop on one thread, and then pass it to the
thread where the message loop actually runs. The message loop's Init() method
must be called on the thread the message loop runs on, before calling Run()"?

Maybe also say that it's ok to post tasks before Init() is called?

Maybe Init() isn't the best name as it's possible to use the object before it's
called. Maybe BindToCurrentThread()?

(…why doesn't Run() just call Init()?)

[kinuko, 2015/04/27 16:36:04]

Done.

+  // and then pass it to the final thread where the message loop actually
+  // runs. The caller must call Init method on the final 'current'
+  // thread before calling Run to start running tasks on the message loop.
+  //
   // Normally, it is not necessary to instantiate a MessageLoop.  Instead, it
   // is typical to make use of the current thread's MessageLoop instance.
+  explicit MessageLoop(const InitOptions& options);

[danakj, 2015/04/24 20:54:39]

I'm a little surprised to see us using a struct here for just 3 arguments. Is
there a reason you went that route?

[kinuko, 2015/04/27 16:36:04]

I wanted to keep them together and wanted to make it clear that they're
internally passed to Init.  I removed this struct anyway as I refactored the
ctor a bit.

+
+  // Creates a MessageLoop of |type| for the current thread. Usually this
+  // constructor is used only for testing. No need to call Init if the

[danakj, 2015/04/24 20:54:39]

Do we need to keep these test-only constructors? Or could we add a test-only
subclass (in test code) and provide helper constructors there instead?

[kinuko, 2015/04/27 16:36:05]

We need these for now, while I agree that having these test-only ctors here is a
bit undesirable. I'd like to try cleaning up these in a follow-up CL as it needs
to touch really large # of files.

[danakj, 2015/04/27 16:46:40]

Can you leave TODOs (pointing to a bug) for things you will do in the future?

+  // message loop is constructed this way.
   explicit MessageLoop(Type type = TYPE_DEFAULT);
-  // Creates a TYPE_CUSTOM MessageLoop with the supplied MessagePump, which must
-  // be non-NULL.
-  explicit MessageLoop(scoped_ptr<base::MessagePump> pump);
+  // Creates a TYPE_CUSTOM MessageLoop for the current thread with the
+  // supplied MessagePump, which must be non-NULL. Usually used only for
+  // testing. No need to call Init if the message loop is constructed this way.
+  explicit MessageLoop(scoped_ptr<MessagePump> pump);
   ~MessageLoop() override;
 
   // Returns the MessageLoop object for the current thread, or null if none.
@@ -147,6 +179,9 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate {
     virtual ~DestructionObserver();
   };
 
+  // Configure various members and bind this message loop to the current thread.
+  void Init();

[danakj, 2015/04/24 20:54:40]

BindToCurrentThread() ?

[kinuko, 2015/04/27 16:36:04]

Done.

+
   // Add a DestructionObserver, which will start receiving notifications
   // immediately.
   void AddDestructionObserver(DestructionObserver* destruction_observer);
@@ -394,10 +429,6 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate {
   // Returns true if the message loop is "idle". Provided for testing.
   bool IsIdleForTesting();
 
-  // Wakes up the message pump. Can be called on any thread. The caller is
-  // responsible for synchronizing ScheduleWork() calls.
-  void ScheduleWork();
-
   // Returns the TaskAnnotator which is used to add debug information to posted
   // tasks.
   debug::TaskAnnotator* task_annotator() { return &task_annotator_; }
@@ -411,9 +442,12 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate {
 
  private:
   friend class RunLoop;
+  friend class internal::IncomingTaskQueue;
+  friend class ScheduleWorkTest;
 
-  // Configures various members for the two constructors.
-  void Init();
+  // Wakes up the message pump. Can be called on any thread. The caller is
+  // responsible for synchronizing ScheduleWork() calls.
+  void ScheduleWork();
 
   // Invokes the actual run loop using the message pump.
   void RunHandler();
@@ -490,6 +524,8 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate {
   bool os_modal_loop_;
 #endif
 
+  scoped_ptr<InitOptions> init_options_;

[danakj, 2015/04/24 20:54:39]

why scoped_ptr and not just a member? i'd prefer a bool initialized_ or
something instead. you could also guard it by DCHECK_IS_ON() if that helps.

[kinuko, 2015/04/27 16:36:04]

Removed this field.

+
   std::string thread_name_;
   // A profiling histogram showing the counts of various messages and events.
   HistogramBase* message_histogram_;