Lazily initialize MessageLoop for faster thread startup

base/message_loop/message_loop.h

 // Copyright 2013 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.
 
 #ifndef BASE_MESSAGE_LOOP_MESSAGE_LOOP_H_
 #define BASE_MESSAGE_LOOP_MESSAGE_LOOP_H_
 
 #include <queue>
 #include <string>
 
@@ skipping 91 matching lines @@
   enum Type {
     TYPE_DEFAULT,
     TYPE_UI,
     TYPE_CUSTOM,
     TYPE_IO,
 #if defined(OS_ANDROID)
     TYPE_JAVA,
 #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.
   static MessageLoop* current();
 
   static void EnableHistogrammer(bool enable_histogrammer);
 
   typedef scoped_ptr<MessagePump> (MessagePumpFactory)();
   // Uses the given base::MessagePumpForUIFactory to override the default
   // MessagePump implementation for 'TYPE_UI'. Returns true if the factory
@@ skipping 12 matching lines @@
   // not be run.  Instead, they will be deleted.
   //
   class BASE_EXPORT DestructionObserver {
    public:
     virtual void WillDestroyCurrentMessageLoop() = 0;
 
    protected:
     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);
 
   // Remove a DestructionObserver.  It is safe to call this method while a
   // DestructionObserver is receiving a notification callback.
   void RemoveDestructionObserver(DestructionObserver* destruction_observer);
 
   // NOTE: Deprecated; prefer task_runner() and the TaskRunner interfaces.
   // TODO(skyostil): Remove these functions (crbug.com/465354).
@@ skipping 227 matching lines @@
   // Can only be called from the thread that owns the MessageLoop.
   bool is_running() const;
 
   // Returns true if the message loop has high resolution timers enabled.
   // Provided for testing.
   bool HasHighResolutionTasks();
 
   // 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_; }
 
   // Runs the specified PendingTask.
   void RunTask(const PendingTask& pending_task);
 
   //----------------------------------------------------------------------------
  protected:
   scoped_ptr<MessagePump> pump_;
 
  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();
 
   // Called to process any delayed non-nestable tasks.
   bool ProcessNextDelayedNonNestableTask();
 
   // Calls RunTask or queues the pending_task on the deferred task list if it
   // cannot be run right now.  Returns true if the task was run.
   bool DeferOrRunPendingTask(const PendingTask& pending_task);
@@ skipping 56 matching lines @@
   // A recursion block that prevents accidentally running additional tasks when
   // insider a (accidentally induced?) nested message pump.
   bool nestable_tasks_allowed_;
 
 #if defined(OS_WIN)
   // Should be set to true before calling Windows APIs like TrackPopupMenu, etc
   // which enter a modal message loop.
   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_;
 
   RunLoop* run_loop_;
 
   ObserverList<TaskObserver> task_observers_;
 
   debug::TaskAnnotator task_annotator_;
 
@@ skipping 151 matching lines @@
 
 // Do not add any member variables to MessageLoopForIO!  This is important b/c
 // MessageLoopForIO is often allocated via MessageLoop(TYPE_IO).  Any extra
 // data that you need should be stored on the MessageLoop's pump_ instance.
 COMPILE_ASSERT(sizeof(MessageLoop) == sizeof(MessageLoopForIO),
                MessageLoopForIO_should_not_have_extra_member_variables);
 
 }  // namespace base
 
 #endif  // BASE_MESSAGE_LOOP_MESSAGE_LOOP_H_