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..5f8c87d06d6b817f9b13f37741d6745417a75f84 100644
--- a/Source/core/dom/EventHandlerRegistry.cpp
+++ b/Source/core/dom/EventHandlerRegistry.cpp
@@ -26,7 +26,8 @@ EventHandlerRegistry::HandlerState::~HandlerState()
 }
 
 EventHandlerRegistry::EventHandlerRegistry(Document& document)
-    : m_document(document)
+    : ActiveDOMObject(&document)
+    , m_document(document)

[abarth-chromium, 2014/04/23 15:23:28]

We should be able to remove m_document now.  ActiveDOMObject already keeps a
pointer to the document.

[Sami, 2014/04/23 16:44:09]

Ah, good idea, done.

 {
 }
 
@@ -44,6 +45,7 @@ EventHandlerRegistry* EventHandlerRegistry::from(Document& document)
     EventHandlerRegistry* registry = static_cast<EventHandlerRegistry*>(DocumentSupplement::from(document, supplementName()));
     if (!registry) {
         registry = new EventHandlerRegistry(document);
+        registry->suspendIfNeeded();

[abarth-chromium, 2014/04/23 15:23:28]

Please add a static create function that does this work and make the constructor
private.

[Sami, 2014/04/23 16:44:09]

Done.

         DocumentSupplement::provideTo(document, supplementName(), adoptPtr(registry));
     }
     return registry;
@@ -75,8 +77,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 +92,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 {
@@ -112,20 +123,28 @@ bool EventHandlerRegistry::updateEventHandlerTargets(ChangeOperation op, EventHa
 
 void EventHandlerRegistry::updateEventHandlerInternal(ChangeOperation op, EventHandlerClass handlerClass, EventTarget* target)
 {
-    // After the document has stopped, all updates become no-ops.
-    if (!m_document.isActive()) {
-        return;
-    }
+    // Notify our parent registry if we added the first or removed the last handler.
+    for (Document* document = &m_document; document; document = document->parentDocument()) {
+        // After the document has stopped, all updates become no-ops.
+        if (!document->isActive()) {
+            return;
+        }

[abarth-chromium, 2014/04/23 15:23:28]

Is it possible for an active document to be contained in an inactive document? 
That seems impossible.

[Sami, 2014/04/23 16:44:09]

Right, we can just check this at the leaf.

+        EventHandlerRegistry* registry = EventHandlerRegistry::from(*document);
 
-    bool hadHandlers = hasEventHandlers(handlerClass);
-    updateEventHandlerTargets(op, handlerClass, target);
-    bool hasHandlers = hasEventHandlers(handlerClass);
+        bool hadHandlers = registry->hasEventHandlers(handlerClass);
+        registry->updateEventHandlerTargets(op, handlerClass, target);

[abarth-chromium, 2014/04/23 15:23:28]

So, each registry knows about all the event listeners on all descendant
documents?  Why do we need to keep this information redundantly at every level
of the tree?  It seems like each event listener should be represented exactly
once (e.g., either associated with its document or maybe with the top-level
document).

[Sami, 2014/04/23 16:44:09]

No, each registry knows two things: 1) the set of handlers directly in its
associated document, and 2) for any direct descendant document whether there are
handlers anywhere in that subtree or not. That is, the full set of handlers is
not redundantly propagated up the tree. Note that this up-propagation also only
happens if it gives some new information to the parent (i.e., the first handler
was added or the last handler was removed in this subtree).

I don't think we want to register all handlers just to the top-level registry,
because that set will quickly grow very large.

+        bool hasHandlers = registry->hasEventHandlers(handlerClass);
 
-    // 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) {
+            break;
+        }
+        if (!document->parentDocument()) {
+            // This is the root registry; notify clients accordingly.
+            registry->notifyHasHandlersChanged(handlerClass, hasHandlers);
+        } else {
+            // Report change to parent with our Document as the target.
+            target = document;
+        }
     }
 }
 
@@ -199,4 +218,13 @@ void EventHandlerRegistry::notifyHasHandlersChanged(EventHandlerClass handlerCla
     }
 }
 
+void EventHandlerRegistry::stop()
+{
+    Document* parentDocument = m_document.parentDocument();
+    if (!parentDocument)
+        return;
+    EventHandlerRegistry* parentRegistry = EventHandlerRegistry::from(*parentDocument);
+    parentRegistry->didRemoveAllEventHandlers(m_document);
+}
+
 } // namespace WebCore