ServiceWorkerEventQueue: Prefer separate functions to one PushTask(Task) function

Follow-up of the review comment:
https://crrev.com/c/1906452/6/third_party/blink/renderer/modules/service_worker/service_worker_timeout_timer.h#109

Prefer separate functions, EnqueueNormal(...) and EnqueuePending(...),
to just one PushTask(Task) function, so that callers don't have to
create a Task on caller sides.

This CL also renames ServiceWorkerEventQueue::Task to
ServiceWorkerEventQueue::Event, and does necessary changes accordingly.
We no longer use a word of *task* in ServiceWorkerEventQueue because using
both tasks and events in ServiceWorkerEventQueue is slightly confusing.
It would be better to use Event consistently because its meaning is clear
in this context.

Bug: 965802

Change-Id: I18b133429e4636eef69cfc4e79c33b13be978ef7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1948368
Commit-Queue: Hayato Ito <hayato@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#721885}

third_party/blink/renderer/modules/service_worker/service_worker_event_queue.cc

@@ -80,61 +80,73 @@ void ServiceWorkerEventQueue::Start() {
                                   WTF::Unretained(this)));
 }
 
-void ServiceWorkerEventQueue::PushTask(std::unique_ptr<Task> task) {
-  DCHECK(task->type != Task::Type::Pending || did_idle_timeout());
-  bool can_start_processing_tasks =
-      !processing_tasks_ && task->type != Task::Type::Pending;
-  task_queue_.emplace_back(std::move(task));
-  if (!can_start_processing_tasks)
+void ServiceWorkerEventQueue::EnqueueNormal(
+    StartCallback start_callback,
+    AbortCallback abort_callback,
+    base::Optional<base::TimeDelta> custom_timeout) {
+  EnqueueEvent(std::make_unique<Event>(
+      Event::Type::Normal, std::move(start_callback), std::move(abort_callback),
+      std::move(custom_timeout)));
+}
+
+void ServiceWorkerEventQueue::EnqueuePending(
+    StartCallback start_callback,
+    AbortCallback abort_callback,
+    base::Optional<base::TimeDelta> custom_timeout) {
+  EnqueueEvent(std::make_unique<Event>(
+      Event::Type::Pending, std::move(start_callback),
+      std::move(abort_callback), std::move(custom_timeout)));
+}
+
+void ServiceWorkerEventQueue::EnqueueEvent(std::unique_ptr<Event> event) {
+  DCHECK(event->type != Event::Type::Pending || did_idle_timeout());
+  bool can_start_processing_events =
+      !processing_events_ && event->type != Event::Type::Pending;
+  queue_.emplace_back(std::move(event));
+  if (!can_start_processing_events)
     return;
   if (did_idle_timeout()) {
     idle_time_ = base::TimeTicks();
     did_idle_timeout_ = false;
   }
-  ProcessTasks();
+  ProcessEvents();
 }
 
-void ServiceWorkerEventQueue::ProcessTasks() {
-  DCHECK(!processing_tasks_);
-  processing_tasks_ = true;
-  while (!task_queue_.IsEmpty()) {
-    StartTask(task_queue_.TakeFirst());
+void ServiceWorkerEventQueue::ProcessEvents() {
+  DCHECK(!processing_events_);
+  processing_events_ = true;
+  while (!queue_.IsEmpty()) {
+    StartEvent(queue_.TakeFirst());
   }
-  processing_tasks_ = false;
+  processing_events_ = false;
 
   // We have to check HasInflightEvent() and may trigger
-  // OnNoInflightEvent() here because StartTask() can call EndEvent()
+  // OnNoInflightEvent() here because StartEvent() can call EndEvent()
   // synchronously, and EndEvent() never triggers OnNoInflightEvent()
-  // while ProcessTasks() is running.
+  // while ProcessEvents() is running.
   if (!HasInflightEvent())
     OnNoInflightEvent();
 }
 
-void ServiceWorkerEventQueue::StartTask(std::unique_ptr<Task> task) {
-  int event_id = StartEvent(std::move(task->abort_callback),
-                            task->custom_timeout.value_or(kEventTimeout));
-  std::move(task->start_callback).Run(event_id);
-}
-
-int ServiceWorkerEventQueue::StartEvent(AbortCallback abort_callback,
-                                        base::TimeDelta timeout) {
+void ServiceWorkerEventQueue::StartEvent(std::unique_ptr<Event> event) {
   idle_time_ = base::TimeTicks();
   const int event_id = NextEventId();
-  auto add_result = id_event_map_.insert(
+  DCHECK(!HasEvent(event_id));
+  id_event_map_.insert(
       event_id, std::make_unique<EventInfo>(
-                    tick_clock_->NowTicks() + timeout,
-                    WTF::Bind(std::move(abort_callback), event_id)));
-  DCHECK(add_result.is_new_entry);
-  return event_id;
+                    tick_clock_->NowTicks() +
+                        event->custom_timeout.value_or(kEventTimeout),
+                    WTF::Bind(std::move(event->abort_callback), event_id)));
+  std::move(event->start_callback).Run(event_id);
 }
 
 void ServiceWorkerEventQueue::EndEvent(int event_id) {
   DCHECK(HasEvent(event_id));
   id_event_map_.erase(event_id);
-  // Check |processing_tasks_| here because EndEvent() can be called
-  // synchronously in StartTask(). We don't want to trigger
-  // OnNoInflightEvent() while ProcessTasks() is running.
-  if (!processing_tasks_ && !HasInflightEvent())
+  // Check |processing_events_| here because EndEvent() can be called
+  // synchronously in StartEvent(). We don't want to trigger
+  // OnNoInflightEvent() while ProcessEvents() is running.
+  if (!processing_events_ && !HasInflightEvent())
     OnNoInflightEvent();
 }
 
@@ -207,8 +219,8 @@ bool ServiceWorkerEventQueue::HasInflightEvent() const {
   return !id_event_map_.IsEmpty() || num_of_stay_awake_tokens_ > 0;
 }
 
-ServiceWorkerEventQueue::Task::Task(
-    ServiceWorkerEventQueue::Task::Type type,
+ServiceWorkerEventQueue::Event::Event(
+    ServiceWorkerEventQueue::Event::Type type,
     StartCallback start_callback,
     AbortCallback abort_callback,
     base::Optional<base::TimeDelta> custom_timeout)
@@ -217,7 +229,7 @@ ServiceWorkerEventQueue::Task::Task(
       abort_callback(std::move(abort_callback)),
       custom_timeout(custom_timeout) {}
 
-ServiceWorkerEventQueue::Task::~Task() = default;
+ServiceWorkerEventQueue::Event::~Event() = default;
 
 ServiceWorkerEventQueue::EventInfo::EventInfo(
     base::TimeTicks expiration_time,

third_party/blink/renderer/modules/service_worker/service_worker_event_queue.h

@@ -24,8 +24,11 @@ class TickClock;
 
 namespace blink {
 
-// ServiceWorkerEventQueue manages two types of timeouts: the long standing
-// event timeout and the idle timeout.
+// ServiceWorkerEventQueue manages events dispatched on ServiceWorkerGlobalScope
+// and their timeouts.
+//
+// There are two types of timeouts: the long standing event timeout
+// and the idle timeout.
 //
 // 1) Event timeout: when an event starts, StartEvent() records the expiration
 // time of the event (kEventTimeout). If EndEvent() has not been called within
@@ -70,45 +73,20 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
 
   using StartCallback = base::OnceCallback<void(int /* event_id */)>;
 
-  // Represents an event dispatch task, which can be queued into |task_queue_|.
-  struct MODULES_EXPORT Task {
-    enum class Type {
-      // A normal task is pushed to the end of the task queue and triggers
-      // processing of the queue.
-      Normal,
-      // A pending task, which does not start until a normal task is
-      // pushed. A pending task should be used if the idle timeout
-      // occurred (did_idle_timeout() returns true). In practice, the
-      // caller pushes pending tasks after the service worker
-      // requested the browser to terminate it due to idleness. These
-      // tasks run once PushTask() is called due to a new event from
-      // the browser, signalling that the browser decided not to
-      // terminate the worker.
-      Pending,
-    };
-
-    Task(Type type,
-         StartCallback start_callback,
-         AbortCallback abort_callback,
-         base::Optional<base::TimeDelta> custom_timeout);
-    ~Task();
-    Type type;
-    // Callback which is run when the event_queue starts this task. The
-    // callback receives |event_id|, which is the result of
-    // StartEvent(). When an event finishes, EndEvent() should be
-    // called with the given |event_id|.
-    StartCallback start_callback;
-    // Callback which is run when the event_queue aborts a started task.
-    AbortCallback abort_callback;
-    // The custom timeout value.
-    base::Optional<base::TimeDelta> custom_timeout;
-  };
-
+  // EndEvent() must be called when an event finishes, with |event_id|
+  // which StartCallback received.
   void EndEvent(int event_id);
 
-  // Push the task to the queue, and tasks in the queue can run synchronously.
-  // See also Task's comment.
-  void PushTask(std::unique_ptr<Task> task);
+  // Enqueues a Normal event. See ServiceWorkerEventQueue::Event to know the
+  // meaning of each parameter.
+  void EnqueueNormal(StartCallback start_callback,
+                     AbortCallback abort_callback,
+                     base::Optional<base::TimeDelta> custom_timeout);
+
+  // Similar to EnqueueNormal(), but enqueues a Pending event.
+  void EnqueuePending(StartCallback start_callback,
+                      AbortCallback abort_callback,
+                      base::Optional<base::TimeDelta> custom_timeout);
 
   // Returns true if |event_id| was started and hasn't ended.
   bool HasEvent(int event_id) const;
@@ -140,10 +118,45 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
       base::TimeDelta::FromSeconds(30);
 
  private:
-  // StartEvent() should be called at the beginning of an event. It returns an
-  // event id, which is unique among threads in the same process.
-  // The event id should be passed to EndEvent() when the event has finished.
-  int StartEvent(AbortCallback abort_callback, base::TimeDelta timeout);
+  // Represents an event dispatch, which can be queued into |queue_|.
+  struct Event {
+    enum class Type {
+      // A normal event is enqueued to the end of the event queue and
+      // triggers processing of the queue.
+      Normal,
+      // A pending event which does not start until a normal event is
+      // pushed. A pending event should be used if the idle timeout
+      // occurred (did_idle_timeout() returns true). In practice, the
+      // caller enqueues pending events after the service worker
+      // requested the browser to terminate it due to idleness. These
+      // events run when a new event comes from the browser,
+      // signalling that the browser decided not to terminate the
+      // worker.
+      Pending,
+    };
+
+    Event(Type type,
+          StartCallback start_callback,
+          AbortCallback abort_callback,
+          base::Optional<base::TimeDelta> custom_timeout);
+    ~Event();
+    Type type;
+    // Callback which is run when the event queue starts this event. The
+    // callback receives |event_id|. When an event finishes,
+    // EndEvent() should be called with the given |event_id|.
+    StartCallback start_callback;
+    // Callback which is run when a started event is aborted.
+    AbortCallback abort_callback;
+    // The custom timeout value.
+    base::Optional<base::TimeDelta> custom_timeout;
+  };
+
+  // Enqueues the event to |queue_|, and run events in the queue or sometimes
+  // later synchronously, depending on the type of events.
+  void EnqueueEvent(std::unique_ptr<Event> event);
+
+  // Starts a single event.
+  void StartEvent(std::unique_ptr<Event> event);
 
   // Updates the internal states and fires timeout callbacks if any.
   void UpdateStatus();
@@ -159,11 +172,8 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
   // Returns true if there are running events.
   bool HasInflightEvent() const;
 
-  // Processes all tasks in |task_queue_|.
-  void ProcessTasks();
-
-  // Start a single task.
-  void StartTask(std::unique_ptr<Task> task);
+  // Processes all events in |queue_|.
+  void ProcessEvents();
 
   struct EventInfo {
     EventInfo(base::TimeTicks expiration_time,
@@ -195,13 +205,13 @@ class MODULES_EXPORT ServiceWorkerEventQueue {
   // StartEvent() is called.
   bool did_idle_timeout_ = false;
 
-  // Tasks that are to be run in the next ProcessTasks() call.
-  Deque<std::unique_ptr<Task>> task_queue_;
+  // Event queue to where all events are enqueued.
+  Deque<std::unique_ptr<Event>> queue_;
 
-  // Set to true during running ProcessTasks(). This is used for avoiding to
-  // invoke |idle_callback_| or to re-enter ProcessTasks() when calling
-  // ProcessTasks().
-  bool processing_tasks_ = false;
+  // Set to true during running ProcessEvents(). This is used for avoiding to
+  // invoke |idle_callback_| or to re-enter ProcessEvents() when calling
+  // ProcessEvents().
+  bool processing_events_ = false;
 
   // The number of the living StayAwakeToken. See also class comments.
   int num_of_stay_awake_tokens_ = 0;

third_party/blink/renderer/modules/service_worker/service_worker_global_scope.h

@@ -458,7 +458,7 @@ class MODULES_EXPORT ServiceWorkerGlobalScope final
   void NoteRespondedToFetchEvent(const KURL& request_url);
 
   // Dispatches the event synchronously. Enqueued by Dispatch*Event methods to
-  // the timeout timer, and executed immediately or sometimes later.
+  // the event queue, and executed immediately or sometimes later.
   void StartFetchEvent(
       mojom::blink::DispatchFetchEventParamsPtr params,
       network::mojom::blink::CrossOriginEmbedderPolicy requestor_coep,
@@ -641,8 +641,8 @@ class MODULES_EXPORT ServiceWorkerGlobalScope final
   // optimizations in these cases.
   HashMap<KURL, int> unresponded_fetch_event_counts_;
 
-  // Timer triggered when the service worker considers it should be stopped or
-  // an event should be aborted.
+  // ServiceWorker event queue where all events are queued before
+  // they are dispatched.
   std::unique_ptr<ServiceWorkerEventQueue> event_queue_;
 
   // InitializeGlobalScope() pauses the top level script evaluation when this