Streamline frame detach

Source/core/frame/LocalFrame.cpp

Index: Source/core/frame/LocalFrame.cpp
diff --git a/Source/core/frame/LocalFrame.cpp b/Source/core/frame/LocalFrame.cpp
index 9c32d1e80efeb8b446a097cd40902fbc0a818a0c..3b8887d367f44b7b63d5f9d894f5026eb59becfd 100644
--- a/Source/core/frame/LocalFrame.cpp
+++ b/Source/core/frame/LocalFrame.cpp
@@ -162,14 +162,22 @@ void LocalFrame::detach()
     // A lot of the following steps can result in the current frame being
     // detached, so protect a reference to it.
     RefPtrWillBeRawPtr<LocalFrame> protect(this);
-    m_loader.stopAllLoaders();
-    m_loader.closeURL();
     detachChildren();
     // stopAllLoaders() needs to be called after detachChildren(), because detachChildren()
     // will trigger the unload event handlers of any child frames, and those event
     // handlers might start a new subresource load in this frame.
+    m_loader.closeURL();

[dcheng, 2014/09/25 23:35:43]

Out of curiosity, is there a reason you chose to reorder this? This results in a
web-visible change, doesn't it? I think unload events in parents are supposed to
fire before unload events in children:
https://html.spec.whatwg.org/multipage/browsers.html#unloading-documents ("When
a user agent is to unload a document, it must run the following steps.")

[Nate Chapin, 2014/09/25 23:57:50]

So it appears. I was grumpy about the 2 stopAllLoaders() calls during detach,
and didn't think to check the spec before trying to optimize one of them away :(

     m_loader.stopAllLoaders();
-    m_loader.detachFromParent();
+    if (!client())
+        return;
+    m_loader.detach();
+    setView(nullptr);
+    willDetachFrameHost();
+    // Notify ScriptController that the frame is closing, since its cleanup ends up calling
+    // back to FrameLoaderClient via WindowProxy.
+    script().clearForClose();
+    InspectorInstrumentation::frameDetachedFromParent(this);
+    Frame::detach();
 }
 
 bool LocalFrame::inScope(TreeScope* scope) const
@@ -314,12 +322,6 @@ void LocalFrame::removeDestructionObserver(FrameDestructionObserver* observer)
 
 void LocalFrame::willDetachFrameHost()
 {
-    // We should never be detatching the page during a Layout.
-    RELEASE_ASSERT(!m_view || !m_view->isInPerformLayout());
-
-    Frame* parent = tree().parent();
-    if (parent && parent->isLocalFrame())
-        toLocalFrame(parent)->loader().checkLoadComplete();
 
     WillBeHeapHashSet<RawPtrWillBeWeakMember<FrameDestructionObserver> >::iterator stop = m_destructionObservers.end();
     for (WillBeHeapHashSet<RawPtrWillBeWeakMember<FrameDestructionObserver> >::iterator it = m_destructionObservers.begin(); it != stop; ++it)
@@ -336,13 +338,6 @@ void LocalFrame::willDetachFrameHost()
         page()->scrollingCoordinator()->willDestroyScrollableArea(m_view.get());
 }
 
-void LocalFrame::detachFromFrameHost()
-{
-    // We should never be detaching the page during a Layout.
-    RELEASE_ASSERT(!m_view || !m_view->isInPerformLayout());
-    m_host = nullptr;
-}
-
 String LocalFrame::documentTypeString() const
 {
     if (DocumentType* doctype = document()->doctype())
@@ -579,7 +574,7 @@ bool LocalFrame::isURLAllowed(const KURL& url) const
 {
     // We allow one level of self-reference because some sites depend on that,
     // but we don't allow more than one.
-    if (page()->subframeCount() >= Page::maxNumberOfFrames)
+    if (host()->frameCount() >= FrameHost::maxNumberOfFrames)
         return false;
     bool foundSelfReference = false;
     for (const Frame* frame = this; frame; frame = frame->tree().parent()) {