[ServiceWorker] waitUntil() should wait until all promises got resolved/rejected.

third_party/WebKit/Source/modules/serviceworkers/WaitUntilObserver.h

 // Copyright 2014 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 WaitUntilObserver_h
 #define WaitUntilObserver_h
 
 #include "modules/ModulesExport.h"
 #include "modules/serviceworkers/ServiceWorkerGlobalScopeClient.h"
 #include "platform/Timer.h"
 #include "platform/wtf/Forward.h"
 
 namespace blink {
 
 class ExceptionState;
 class ExecutionContext;
 class ScriptPromise;
 class ScriptState;
-class ScriptValue;
 
 // Created for each ExtendableEvent instance.
 class MODULES_EXPORT WaitUntilObserver final
     : public GarbageCollectedFinalized<WaitUntilObserver> {
  public:
   enum EventType {
     kActivate,
     kFetch,
     kInstall,
     kMessage,
     kNotificationClick,
     kNotificationClose,
     kPaymentRequest,
     kPush,
     kSync,
     kBackgroundFetchAbort,
     kBackgroundFetchClick,
     kBackgroundFetchFail,
     kBackgroundFetched
   };
 
   static WaitUntilObserver* Create(ExecutionContext*, EventType, int event_id);
 
-  // Must be called before and after dispatching the event.
+  // Must be called before dispatching the event.
   void WillDispatchEvent();
-  void DidDispatchEvent(bool error_occurred);
+  // Must be called after dispatching the event. If |event_dispatch_failed|,
+  // this WaitUntilObserver will soon report to ServiceWorkerGlobalScopeClient

[falken, 2017/05/10 05:25:12]

my bad, I didn't realize the DidDispatchEvent will immediately report to the
SWGSC in this case. So "If |event_dispatch_failed| is true, then
DidDispatchEvent() immediately reports to ServiceWorkerGlobalScopeClient that
the event finished, without waiting for all waitUntil promises to settle."

[leonhsl(Using Gerrit), 2017/05/10 07:07:52]

Done.

+  // that the event finished, without waiting for all waitUntil promises to
+  // settle.
+  void DidDispatchEvent(bool event_dispatch_failed);
 
   // Observes the promise and delays calling the continuation until
   // the given promise is resolved or rejected.
   void WaitUntil(ScriptState*, ScriptPromise, ExceptionState&);
 
   // These methods can be called when the lifecycle of ExtendableEvent
   // observed by this WaitUntilObserver should be extended by other reason
   // than ExtendableEvent.waitUntil.
   // Note: There is no need to call decrementPendingActivity() after the context
   // is being destroyed.
   void IncrementPendingActivity();
   void DecrementPendingActivity();
 
   DECLARE_VIRTUAL_TRACE();
 
  private:
   friend class InternalsServiceWorker;
   class ThenFunction;
 
   WaitUntilObserver(ExecutionContext*, EventType, int event_id);
 
-  void ReportError(const ScriptValue&);
+  // Called when a promise passed to a waitUntil() call that is associated with
+  // this observer has completed. |rejected| indicates whether the promise is
+  // rejected.
+  void OnPromiseCompleted(bool rejected);
 
   void ConsumeWindowInteraction(TimerBase*);
 
+  enum EventDispatchState { kInitial, kCompleted, kFailed };

[shimazu, 2017/05/10 05:20:14]

"enum class" is better here:
https://www.chromium.org/blink/coding-style#TOC-Right:12

[falken, 2017/05/10 05:25:12]

Can you document:

// Event dispatch has not yet finished.
kInitial,
// Event dispatch completed. There may still be outstanding waitUntil promises
that must settle before notifying ServiceWorkerGlobalScopeClient that the event
finished.
kCompleted,
// Event dispatch failed. Any outstanding waitUntil promises are ignored.
kFailed

[leonhsl(Using Gerrit), 2017/05/10 07:07:52]

Done.

[leonhsl(Using Gerrit), 2017/05/10 07:07:52]

Done.

+  EventDispatchState event_dispatch_state_ = kInitial;

[shimazu, 2017/05/10 05:20:14]

It would be better to initialize this at the constructor to align with other
members.

[falken, 2017/05/10 05:26:52]

I thought so too but pending_activity_ and has_rejected_promise_ are initialized
here already... I don't know whether one is better than the other anymore.

Maybe type_ and event_id_ should just be assigned to something here too?

[leonhsl(Using Gerrit), 2017/05/10 07:07:52]

We set class member's default value in .h file, so I set event_dispatch_state_
together with pending_activity_ and event_dispatch_time_ etc. While type_ and
event_id_ seems have no default value, they're expected to be set explicitly by
constructor.

+
   Member<ExecutionContext> execution_context_;
   EventType type_;
   int event_id_;
   int pending_activity_ = 0;
-  bool has_error_ = false;
-  bool event_dispatched_ = false;
+  bool has_rejected_promise_ = false;
   double event_dispatch_time_ = 0;
   TaskRunnerTimer<WaitUntilObserver> consume_window_interaction_timer_;
 };
 
 }  // namespace blink
 
 #endif  // WaitUntilObserver_h