Expand comments on ContextLifecycleObserver and PausableObject.

These are really common but their proper usage isn't necessarily clear.

Change-Id: I4db2e78edddee81f563f5a35f2425a92675d0949
Reviewed-on: https://chromium-review.googlesource.com/1101247
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567720}

third_party/blink/renderer/core/dom/context_lifecycle_observer.h

@@ -37,17 +37,35 @@ class LocalDOMWindow;
 class LocalFrame;
 
 // ContextClient and ContextLifecycleObserver are helpers to associate an
-// object with an ExecutionContext.
+// object with an ExecutionContext. It is unsafe to access an associated GC
+// object, *including via GetExecutionContext()*, in the destructor of a GC
+// object.
 //
-// - getExecutionContext() returns null after the context is detached.
-// - frame() is a syntax sugar for getExecutionContext()->frame(). It returns
-//   null after the context is detached or the context is not a Document.
+// To discourage incorrect usage, these objects both start returning null
+// once the execution context shuts down. For a document, this occurs when
+// the frame navigates to another document or is detached. For a worker global
+// scope, this occurs when it shuts down.
 //
-// Both can safely be used up until destruction; i.e., unsafe to
-// call upon in a destructor.
+// * If an object only needs to refer to a valid ExecutionContext but does not
+//   need to stop or suspend any activity, it should be a ContextClient.
+// * If an object associated with an ExecutionContext has shutdown logic to
+//   perform, such as halting activity or disconnecting from longer-lived
+//   objects, it should be a ContextLifecycleObserver.
+// * If an object additionally must suspend its activity during pause (see
+//   pausable_object.h), it should be a PausableObject (and thus, transitively,
+//   also a ContextLifecycleObserver).
+
+
+// ContextClient provides access to the associated execution context until it is
+// shut down (e.g. for a document, at navigation or frame detach).
 class CORE_EXPORT ContextClient : public GarbageCollectedMixin {
  public:
+  // Returns the execution context until it is detached.
+  // From then on, returns null instead.
   ExecutionContext* GetExecutionContext() const;
+
+  // If associated with a live document, returns the associated frame.
+  // Returns null otherwise.
   LocalFrame* GetFrame() const;
 
   void Trace(blink::Visitor*) override;
@@ -60,15 +78,31 @@ class CORE_EXPORT ContextClient : public GarbageCollectedMixin {
   WeakMember<ExecutionContext> execution_context_;
 };
 
-// ContextLifecycleObserver provides an additional contextDestroyed() hook
-// to execute cleanup code when a context is destroyed. Prefer the simpler
-// ContextClient when possible.
+// ContextLifecycleObserver provides an additional ContextDestroyed() hook
+// to execute cleanup code when a context is shut down (e.g. for a document,
+// at navigation or frame detach -- not when its destructor runs).
+//
+// Execution context associated objects which have ongoing activity,
+// registration with objects which outlive the context, or resources which
+// should be promptly released, should consider deriving from
+// ContextLifecycleObserver. As a rule of thumb: if the destructor contains
+// non-trivial logic, that logic may belong in ContextDestroyed() instead.
+//
+// If there is ongoing activity associated with the object, consider whether it
+// needs to be paused when execution is suspended (see PausableObject).
+//
+// If none of the above applies, prefer the simpler ContextClient.
 class CORE_EXPORT ContextLifecycleObserver
     : public LifecycleObserver<ExecutionContext, ContextLifecycleObserver> {
  public:
   virtual void ContextDestroyed(ExecutionContext*) {}
 
+  // Returns the execution context until it is detached.
+  // From then on, returns null instead.
   ExecutionContext* GetExecutionContext() const { return LifecycleContext(); }
+
+  // If associated with a live document, returns the associated frame.
+  // Returns null otherwise.
   LocalFrame* GetFrame() const;
 
   enum Type {

third_party/blink/renderer/core/dom/pausable_object.h

@@ -33,6 +33,24 @@
 
 namespace blink {
 
+// A PausableObject responds to situations where Blink is running a nested event
+// loop. At such times, the page should be responsive, but only in a limited
+// sense: the browser should redraw the page as necessary, but should not run
+// timer callbacks, finish promise resolution, fire events, or perform other
+// activity, especially activity which may run script.
+//
+// Pausing can happen in cases such as:
+// - modal dialogs during script execution (e.g. window.alert, window.print)
+// - script execution stopped at a debugger breakpoint
+//
+// The scheduler will automatically suspend certain task queues for the duration
+// that the page is paused.
+//
+// Objects with asynchronous activity, especially activity that may have an
+// observable effect on web-visible state, on should suspend that activity while
+// the page is paused by overriding Pause() and Unpause().
+//
+// https://html.spec.whatwg.org/multipage/webappapis.html#pause
 class CORE_EXPORT PausableObject : public ContextLifecycleObserver {
  public:
   explicit PausableObject(ExecutionContext*);