[base] Migrate MessagePumpGlib to DoSomeWork()

This CL is intended to be a no-op logic wise (aside from merging
DoWork()/DoDelayedWork() into a single call).

As a follow-up, it might be possible to take better advantage of
NextWorkInfo::recent_now but it wasn't trivial to do so in this pump
since the sleep is not invoked directly from Run() but rather implicit
from HandlePrepare(). We could also potentially reuse |recent_now|
between HandlePrepare() and HandleCheck(). For now we stick to invoking
TimeTicks::Now() as much as the previous impl did.

Bug: 885371
Change-Id: I4f9d6642c44dbad82cb3588cf3f5323ddf9c19ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1594695
Commit-Queue: Gabriel Charette <gab@chromium.org>
Commit-Queue: Mark Mentovai <mark@chromium.org>
Auto-Submit: Gabriel Charette <gab@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#656415}

base/message_loop/message_pump_glib.cc

@@ -11,6 +11,7 @@
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/platform_thread.h"
@@ -19,20 +20,19 @@ namespace base {
 
 namespace {
 
-// Return a timeout suitable for the glib loop, -1 to block forever,
-// 0 to return right away, or a timeout in milliseconds from now.
-int GetTimeIntervalMilliseconds(const TimeTicks& from) {
-  if (from.is_null())
+// Return a timeout suitable for the glib loop according to |next_task_time|, -1
+// to block forever, 0 to return right away, or a timeout in milliseconds from
+// now.
+int GetTimeIntervalMilliseconds(TimeTicks next_task_time) {
+  if (next_task_time.is_null())
+    return 0;
+  else if (next_task_time.is_max())
     return -1;
 
-  // Be careful here.  TimeDelta has a precision of microseconds, but we want a
-  // value in milliseconds.  If there are 5.5ms left, should the delay be 5 or
-  // 6?  It should be 6 to avoid executing delayed work too early.
-  int delay = static_cast<int>(
-      ceil((from - TimeTicks::Now()).InMillisecondsF()));
+  auto timeout_ms =
+      (next_task_time - TimeTicks::Now()).InMillisecondsRoundedUp();
 
-  // If this value is negative, then we need to run delayed work soon.
-  return delay < 0 ? 0 : delay;
+  return timeout_ms < 0 ? 0 : saturated_cast<int>(timeout_ms);
 }
 
 // A brief refresher on GLib:
@@ -68,8 +68,8 @@ int GetTimeIntervalMilliseconds(const TimeTicks& from) {
 // - Return true if any of prepare() or check() returned true.
 //
 // gtk_main_iteration just calls g_main_context_iteration, which does the whole
-// thing, respecting the timeout for the poll (and block, although it is
-// expected not to if gtk_events_pending returned true), and call dispatch.
+// thing, respecting the timeout for the poll (and block, although it is to if
+// gtk_events_pending returned true), and call dispatch.
 //
 // Thus it is important to only return true from prepare or check if we
 // actually have events or work to do. We also need to make sure we keep
@@ -79,10 +79,9 @@ int GetTimeIntervalMilliseconds(const TimeTicks& from) {
 //
 // For the GLib pump we try to follow the Windows UI pump model:
 // - Whenever we receive a wakeup event or the timer for delayed work expires,
-// we run DoWork and/or DoDelayedWork. That part will also run in the other
-// event pumps.
-// - We also run DoWork, DoDelayedWork, and possibly DoIdleWork in the main
-// loop, around event handling.
+// we run DoSomeWork. That part will also run in the other event pumps.
+// - We also run DoSomeWork, and possibly DoIdleWork, in the main loop,
+// around event handling.
 
 struct WorkSource : public GSource {
   MessagePumpGlib* pump;
@@ -169,10 +168,10 @@ struct MessagePumpGlib::RunState {
   // Used to count how many Run() invocations are on the stack.
   int run_depth;
 
-  // This keeps the state of whether the pump got signaled that there was new
-  // work to be done. Since we eat the message on the wake up pipe as soon as
-  // we get it, we keep that state here to stay consistent.
-  bool has_work;
+  // The information of the next task available at this run-level. Stored in
+  // RunState because different set of tasks can be accessible at various
+  // run-levels (e.g. non-nestable tasks).
+  Delegate::NextWorkInfo next_work_info;
 };
 
 MessagePumpGlib::MessagePumpGlib()
@@ -212,15 +211,11 @@ MessagePumpGlib::~MessagePumpGlib() {
 
 // Return the timeout we want passed to poll.
 int MessagePumpGlib::HandlePrepare() {
-  // We know we have work, but we haven't called HandleDispatch yet. Don't let
-  // the pump block so that we can do some processing.
-  if (state_ &&  // state_ may be null during tests.
-      state_->has_work)
+  // |state_| may be null during tests.
+  if (!state_)
     return 0;
 
-  // We don't think we have work to do, but make sure not to block
-  // longer than the next time we need to run delayed work.
-  return GetTimeIntervalMilliseconds(delayed_work_time_);
+  return GetTimeIntervalMilliseconds(state_->next_work_info.delayed_run_time);
 }
 
 bool MessagePumpGlib::HandleCheck() {
@@ -240,18 +235,17 @@ bool MessagePumpGlib::HandleCheck() {
     }
     DCHECK((num_bytes == 1 && msg[0] == '!') ||
            (num_bytes == 2 && msg[0] == '!' && msg[1] == '!'));
-    // Since we ate the message, we need to record that we have more work,
+    // Since we ate the message, we need to record that we have immediate work,
     // because HandleCheck() may be called without HandleDispatch being called
     // afterwards.
-    state_->has_work = true;
-  }
-
-  if (state_->has_work)
+    state_->next_work_info = {TimeTicks()};
     return true;
+  }
 
-  if (GetTimeIntervalMilliseconds(delayed_work_time_) == 0) {
-    // The timer has expired. That condition will stay true until we process
-    // that delayed work, so we don't need to record this differently.
+  // As described in the summary at the top : Check is a second-chance to
+  // Prepare, verify whether we have work ready again.
+  if (GetTimeIntervalMilliseconds(state_->next_work_info.delayed_run_time) ==
+      0) {
     return true;
   }
 
@@ -259,19 +253,7 @@ bool MessagePumpGlib::HandleCheck() {
 }
 
 void MessagePumpGlib::HandleDispatch() {
-  state_->has_work = false;
-  if (state_->delegate->DoWork()) {
-    // NOTE: on Windows at this point we would call ScheduleWork (see
-    // MessagePumpGlib::HandleWorkMessage in message_pump_win.cc). But here,
-    // instead of posting a message on the wakeup pipe, we can avoid the
-    // syscalls and just signal that we have more work.
-    state_->has_work = true;
-  }
-
-  if (state_->should_quit)
-    return;
-
-  state_->delegate->DoDelayedWork(&delayed_work_time_);
+  state_->next_work_info = state_->delegate->DoSomeWork();
 }
 
 void MessagePumpGlib::Run(Delegate* delegate) {
@@ -283,7 +265,6 @@ void MessagePumpGlib::Run(Delegate* delegate) {
   state.delegate = delegate;
   state.should_quit = false;
   state.run_depth = state_ ? state_->run_depth + 1 : 1;
-  state.has_work = false;
 
   RunState* previous_state = state_;
   state_ = &state;
@@ -306,12 +287,8 @@ void MessagePumpGlib::Run(Delegate* delegate) {
     if (state_->should_quit)
       break;
 
-    more_work_is_plausible |= state_->delegate->DoWork();
-    if (state_->should_quit)
-      break;
-
-    more_work_is_plausible |=
-        state_->delegate->DoDelayedWork(&delayed_work_time_);
+    state_->next_work_info = state_->delegate->DoSomeWork();
+    more_work_is_plausible |= state_->next_work_info.is_immediate();
     if (state_->should_quit)
       break;
 
@@ -347,7 +324,6 @@ void MessagePumpGlib::ScheduleWork() {
 void MessagePumpGlib::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
   // We need to wake up the loop in case the poll timeout needs to be
   // adjusted.  This will cause us to try to do work, but that's OK.
-  delayed_work_time_ = delayed_work_time;
   ScheduleWork();
 }
 

base/message_loop/message_pump_glib.h

@@ -56,9 +56,6 @@ class BASE_EXPORT MessagePumpGlib : public MessagePump {
   // dispatched.
   GMainContext* context_;
 
-  // This is the time when we need to do delayed work.
-  TimeTicks delayed_work_time_;
-
   // The work source.  It is shared by all calls to Run and destroyed when
   // the message pump is destroyed.
   GSource* work_source_;

base/message_loop/message_pump_unittest.cc

@@ -44,7 +44,7 @@ bool PumpTypeUsesDoSomeWork(MessageLoop::Type type) {
       // iOS uses a MessagePumpDefault for UI in unit tests, ref.
       // test_support_ios.mm::CreateMessagePumpForUIForTests().
       return true;
-#elif defined(OS_WIN) || defined(OS_ANDROID)
+#elif defined(OS_WIN) || defined(OS_ANDROID) || defined(USE_GLIB)
       return true;
 #elif defined(OS_POSIX) && !defined(OS_NACL_SFI)
       // MessagePumpLibevent was migrated (ref. message_pump_for_ui.h and