Extract simpler classes for observing context lifecycle and observe Page lifecycle inNavigatorVibra…

Source/core/dom/SimpleLifecycleNotifier.h

Index: Source/core/dom/SimpleLifecycleNotifier.h
diff --git a/Source/core/dom/ContextLifecycleObserver.h b/Source/core/dom/SimpleLifecycleNotifier.h
similarity index 51%
copy from Source/core/dom/ContextLifecycleObserver.h
copy to Source/core/dom/SimpleLifecycleNotifier.h
index 574455e861a357cf4f33514367aaf69b3657d5a9..2b7221da556a7c81cac545e5a1e69587565e6742 100644
--- a/Source/core/dom/ContextLifecycleObserver.h
+++ b/Source/core/dom/SimpleLifecycleNotifier.h
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2008 Apple Inc. All Rights Reserved.
+ * Copyright (C) 2013 Google Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -23,34 +24,55 @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  *
  */
+#ifndef SimpleLifecycleNotifier_h
+#define SimpleLifecycleNotifier_h
 
-#ifndef ContextLifecycleObserver_h
-#define ContextLifecycleObserver_h
+#include "core/dom/SimpleLifecycleObserver.h"
+#include "wtf/HashSet.h"
+#include "wtf/PassOwnPtr.h"
 
 namespace WebCore {
 
-class ScriptExecutionContext;
+class SimpleLifecycleContext;
+class SimpleLifecycleObserver;
 
-class ContextLifecycleObserver {
+class SimpleLifecycleNotifier {
 public:
-    enum Type {
-        ActiveDOMObjectType,
-        DocumentLifecycleObserverType,
-        GenericType
-    };
+    static PassOwnPtr<SimpleLifecycleNotifier> create(SimpleLifecycleContext*);
+
+    virtual ~SimpleLifecycleNotifier();
 
-    explicit ContextLifecycleObserver(ScriptExecutionContext*, Type = GenericType);
-    virtual void contextDestroyed();
+    typedef HashSet<SimpleLifecycleObserver*> SimpleObserverSet;
 
-    ScriptExecutionContext* scriptExecutionContext() const { return m_scriptExecutionContext; }
+    virtual void addObserver(SimpleLifecycleObserver*, SimpleLifecycleObserver::Type);
+    virtual void removeObserver(SimpleLifecycleObserver*, SimpleLifecycleObserver::Type);
 
 protected:
-    virtual ~ContextLifecycleObserver();
-    void observeContext(ScriptExecutionContext*, Type);
+    explicit SimpleLifecycleNotifier(SimpleLifecycleContext*);
 
-    ScriptExecutionContext* m_scriptExecutionContext;
+    SimpleLifecycleContext* context();
+
+    enum IterationType {
+        IteratingNone,
+        IteratingOverActiveDOMObjects,
+        IteratingOverContextObservers,
+        IteratingOverDocumentObservers,
+        IteratingOverPageObservers

[abarth-chromium, 2013/07/08 21:25:24]

I'm not sure it's worth abstracting the SimpleLifecycleNotifier base class.  I
would just create a PageLifecycleNotifier and not worry too much about the five
duplicated lines of code.

The downside to the approach in this CL is that you have the enum that requires
this class to know about all of its subclasses.  That introduces some non-sense
states like a DocumentLifecycleNotifier be in the IteratingOverPageObservers
state (at least according to the type system).

I can see how a SimpleLifecycleNotifier would make sense if it didn't need to
know about all of its subclasses...  Even then, though, it looks like you've
made addObserver and removeObserver virtual, which seems like more unnecessarily
generality....

[Michael van Ouwerkerk, 2013/07/09 17:09:12]

The file branch detection may be confusing here - the system thinks the original
file was ContextLifecycleObserver.h. The real original is
ContextLifecycleNotifier.h and it contains a lot of code for managing
ActiveDOMObjects. I don't think that's needed in the 'simple' use case, and I
didn't want to make that functionality and its dependencies part of the Page
class.

The enum that knows about all subclasses seems like an anti pattern. However,
the enum was already there in ContextLifecycleNotifier, I didn't add that to
this system:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The addObserver and removeObserver methods were already virtual, I didn't add
that generality:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Really, in this patch I'm trying to avoid pulling too much complexity into the
Page class. The alternative would be to not extract these simple base classes
and just make Page inherit from ScriptExecutionContext, which is quite large and
does much more than some basic lifecycle observation.

On 2013/07/08 21:25:24, abarth wrote:

+    };
+
+    IterationType m_iterating;
+
+private:
+    SimpleObserverSet m_observers;
+    SimpleLifecycleContext* m_context;
+    bool m_inDestructor;
 };
 
+inline PassOwnPtr<SimpleLifecycleNotifier> SimpleLifecycleNotifier::create(SimpleLifecycleContext* context)
+{
+    return adoptPtr(new SimpleLifecycleNotifier(context));
+}
+
 } // namespace WebCore
 
-#endif // ContextLifecycleObserver_h
+#endif // SimpleLifecycleNotifier_h