Track event handlers on windows

Source/core/frame/DOMWindow.cpp

Index: Source/core/frame/DOMWindow.cpp
diff --git a/Source/core/frame/DOMWindow.cpp b/Source/core/frame/DOMWindow.cpp
index 58eb5f81e01198d54b8ca2702ff2913171ed1e1e..bf86c6029e217a21c3ff4e4865980f89cb20ece2 100644
--- a/Source/core/frame/DOMWindow.cpp
+++ b/Source/core/frame/DOMWindow.cpp
@@ -59,6 +59,7 @@
 #include "core/frame/Console.h"
 #include "core/frame/DOMPoint.h"
 #include "core/frame/DOMWindowLifecycleNotifier.h"
+#include "core/frame/EventHandlerRegistry.h"
 #include "core/frame/FrameConsole.h"
 #include "core/frame/FrameHost.h"
 #include "core/frame/FrameView.h"
@@ -329,6 +330,7 @@ DOMWindow::DOMWindow(LocalFrame& frame)
 #endif
 {
     ScriptWrappable::init(this);
+    m_frame->host()->eventHandlerRegistry().didMoveIntoFrameHost(*this);

[Rick Byers, 2014/05/06 20:54:39]

Please add a comment describing why this is necessary.  Naively I don't see how
there can be any event handlers to attach to the registry at this point when the
window is just being created now.  Ah, I see now - it's to ensure the observer
is created.

I think separate windowCreated/windowDestroyed entrypoints into the registry
might be clearer.  Then you could ASSERT in in windowCreated that there are no
event handlers on the window (and maybe even assert in windowDestroyed that
there aren't any handlers left in the registry if we can get deterministic
cleanup).  WDYT?

Overall it seems like this might be a lot of extra bookkeeping in order to
robustly support the DOMWindowLifecycleObserver abstraction.  Since every
DOMWindow should always have exactly one FrameHost, perhaps this would get a lot
simpler (and more robust) if we just hard-coded calls into EventHandlerRegistry
in DOMWindow::*eventListener (as the touch event handlers are tracked today)?

[Sami, 2014/05/07 10:36:41]

Yeah, I was thinking along the same lines -- it would be much simpler just to
add the few hooks that we need to DOMWindow. I've now done that and I think the
result is much cleaner. WDYT?

[Rick Byers, 2014/05/07 13:28:21]

Yeah this seems a lot cleaner.  The observer abstraction is a nice idea, but in
this case I don't think it's worth the extra lifetime management complexity.

 }
 
 void DOMWindow::clearDocument()
@@ -484,6 +486,9 @@ DOMWindow::~DOMWindow()
     ASSERT(m_hasBeenReset);
     reset();
 
+    if (m_frame && m_frame->host())
+        m_frame->host()->eventHandlerRegistry().didMoveOutOfFrameHost(*this);
+
     removeAllEventListeners();
 
     ASSERT(m_document->isStopped());
@@ -523,6 +528,7 @@ void DOMWindow::frameDestroyed()
 
 void DOMWindow::willDetachFrameHost()

[Rick Byers, 2014/05/06 20:54:39]

Does this only happen when a window is being destroyed?  Is there any way we can
verify that event handlers aren't added after this point?

[Sami, 2014/05/07 10:36:41]

I don't think we need to worry about that. If handlers are attached after
FrameHost goes away, they shouldn't appear in the registry anyway. There's also
no need to import such handlers later, since as far as I can tell DOMWindow
can't attach to a different FrameHost.

The critical thing is to make sure we unregister all existing handlers before
the FrameHost goes away. The asserts in EventHandlerRegistry::checkConsistency()
check for that.

[Rick Byers, 2014/05/07 13:28:21]

Thanks, sounds reasonable.

 {
+    m_frame->host()->eventHandlerRegistry().didMoveOutOfFrameHost(*this);
     InspectorInstrumentation::frameWindowDiscarded(m_frame, this);
 }