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

Source/core/dom/SimpleLifecycleNotifier.h

 /*
  * 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
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
  *    documentation and/or other materials provided with the distribution.
  *
  * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
  * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
  * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
  * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
  * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
  * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
  * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
  * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
  * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * 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();
+
+    typedef HashSet<SimpleLifecycleObserver*> SimpleObserverSet;
+
+    virtual void addObserver(SimpleLifecycleObserver*, SimpleLifecycleObserver::Type);
+    virtual void removeObserver(SimpleLifecycleObserver*, SimpleLifecycleObserver::Type);
+
+protected:
+    explicit SimpleLifecycleNotifier(SimpleLifecycleContext*);
+
+    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:

     };
 
-    explicit ContextLifecycleObserver(ScriptExecutionContext*, Type = GenericType);
-    virtual void contextDestroyed();
+    IterationType m_iterating;
 
-    ScriptExecutionContext* scriptExecutionContext() const { return m_scriptExecutionContext; }
+private:
+    SimpleObserverSet m_observers;
+    SimpleLifecycleContext* m_context;
+    bool m_inDestructor;
+};
 
-protected:
-    virtual ~ContextLifecycleObserver();
-    void observeContext(ScriptExecutionContext*, Type);
-
-    ScriptExecutionContext* m_scriptExecutionContext;
-};
+inline PassOwnPtr<SimpleLifecycleNotifier> SimpleLifecycleNotifier::create(SimpleLifecycleContext* context)
+{
+    return adoptPtr(new SimpleLifecycleNotifier(context));
+}
 
 } // namespace WebCore
 
-#endif // ContextLifecycleObserver_h
+#endif // SimpleLifecycleNotifier_h