Track scroll event handlers in nested documents

Source/core/dom/EventHandlerRegistry.cpp

Index: Source/core/dom/EventHandlerRegistry.cpp
diff --git a/Source/core/dom/EventHandlerRegistry.cpp b/Source/core/dom/EventHandlerRegistry.cpp
index b72b465ffaadc0a31187c14559227570e38b81e7..30f06dfbdcdc7b8a756feb45a6f7060e95d97004 100644
--- a/Source/core/dom/EventHandlerRegistry.cpp
+++ b/Source/core/dom/EventHandlerRegistry.cpp
@@ -17,6 +17,26 @@
 
 namespace WebCore {
 
+EventHandlerRegistry::DocumentObserver::DocumentObserver(Document& document)
+    : ActiveDOMObject(&document)
+{
+    suspendIfNeeded();

[abarth-chromium, 2014/04/22 16:18:23]

As a general pattern, we don't call suspendIfNeeded() inside constructors
because it can call back into virtual functions on |this| that might be
overridden in subclasses.  I understand that won't happen for this class because
it's FINAL, but it's a pattern we avoid generally in the code base to simplify
our lives.  Instead, we use a static |create| function that calls
suspendIfNeeded() after we're done constructing the object.

[Sami, 2014/04/22 18:05:18]

Okay, it sounds like I can move the call to suspendIfNeeded to the registry's
constructor instead.

+}
+
+EventHandlerRegistry::DocumentObserver::~DocumentObserver()
+{
+}
+
+void EventHandlerRegistry::DocumentObserver::stop()
+{
+    Document* document = static_cast<Document*>(lifecycleContext());
+    Document* parentDocument = document->parentDocument();
+    if (!parentDocument)
+        return;
+    EventHandlerRegistry* parentRegistry = EventHandlerRegistry::from(*parentDocument);
+    parentRegistry->didRemoveAllEventHandlers(*document);
+}
+
 EventHandlerRegistry::HandlerState::HandlerState()
 {
 }
@@ -27,6 +47,7 @@ EventHandlerRegistry::HandlerState::~HandlerState()
 
 EventHandlerRegistry::EventHandlerRegistry(Document& document)
     : m_document(document)
+    , m_documentObserver(DocumentObserver(document))
 {

[abarth-chromium, 2014/04/22 16:18:23]

What's the point of having a separate DocumentObserver class?  Instead,
EventHandlerRegistry could inherit from DocumentLifecycleObserver and listen for
documentWasDetached.

[Sami, 2014/04/22 18:05:18]

First, it better isolates the implementation from the clients of
EventHandlerRegistry; an "EventHandlerRegistry is-a DocumentLifecycleObserver"
relation does not make any sense for them. Second, in another patch I'm planning
to add a window observer as a separated nested class. I think having code for
observing documents and DOM windows in separate classes makes things clearer.

 }
 
@@ -75,8 +96,12 @@ bool EventHandlerRegistry::updateEventHandlerTargets(ChangeOperation op, EventHa
     EventTargetSet* targets = m_eventHandlers[handlerClass].targets.get();
     if (op == Add) {
 #if ASSERT_ENABLED
-        if (Node* node = target->toNode())
-            ASSERT(&node->document() == &m_document);
+        if (Node* node = target->toNode()) {
+            // The node should either be in the document, or be the Document node of a child
+            // of the document.
+            ASSERT(&node->document() == &m_document
+                || (node->isDocumentNode() && toDocument(node)->parentDocument() == &m_document));
+        }
 #endif // ASSERT_ENABLED
 
         if (!targets) {
@@ -86,6 +111,11 @@ bool EventHandlerRegistry::updateEventHandlerTargets(ChangeOperation op, EventHa
 
         if (!targets->add(target).isNewEntry) {
             // Just incremented refcount, no real change.
+#if ASSERT_ENABLED
+            // If this is a child document node, then the count should never go above 1.
+            if (Node* node = target->toNode())
+                ASSERT(!node->isDocumentNode() || &node->document() == &m_document);
+#endif // ASSERT_ENABLED
             return false;
         }
     } else {
@@ -123,9 +153,14 @@ void EventHandlerRegistry::updateEventHandlerInternal(ChangeOperation op, EventH
 
     // Notify the parent document's registry if we added the first or removed
     // the last handler.
-    if (hadHandlers != hasHandlers && !m_document.parentDocument()) {
-        // This is the root registry; notify clients accordingly.
-        notifyHasHandlersChanged(handlerClass, hasHandlers);
+    if (hadHandlers != hasHandlers) {
+        if (Document* parent = m_document.parentDocument()) {
+            // Report change to parent with our Document as the target.
+            EventHandlerRegistry::from(*parent)->updateEventHandlerInternal(op, handlerClass, &m_document);
+        } else {
+            // This is the root registry; notify clients accordingly.
+            notifyHasHandlersChanged(handlerClass, hasHandlers);
+        }

[abarth-chromium, 2014/04/22 16:18:23]

Can we implement this algorithm iteratively instead of recursively?

[Sami, 2014/04/22 18:05:18]

Sure, done. I'm having trouble deciding which way is better, but PTAL.

     }
 }