Streamline frame detach

Source/core/frame/LocalFrame.cpp

 /*
  * Copyright (C) 1998, 1999 Torben Weis <weis@kde.org>
  *                     1999 Lars Knoll <knoll@kde.org>
  *                     1999 Antti Koivisto <koivisto@kde.org>
  *                     2000 Simon Hausmann <hausmann@kde.org>
  *                     2000 Stefan Schimanski <1Stein@gmx.de>
  *                     2001 George Staikos <staikos@kde.org>
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2005 Alexey Proskuryakov <ap@nypop.com>
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
@@ skipping 144 matching lines @@
     HeapSupplementable<LocalFrame>::trace(visitor);
 #endif
     Frame::trace(visitor);
 }
 
 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
 {
     ASSERT(scope);
     Document* doc = document();
     if (!doc)
         return false;
     // FIXME: This check is broken in for OOPI.
     HTMLFrameOwnerElement* owner = doc->ownerElement();
@@ skipping 124 matching lines @@
     m_destructionObservers.add(observer);
 }
 
 void LocalFrame::removeDestructionObserver(FrameDestructionObserver* observer)
 {
     m_destructionObservers.remove(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)
         (*it)->willDetachFrameHost();
 
     // FIXME: Page should take care of updating focus/scrolling instead of Frame.
     // FIXME: It's unclear as to why this is called more than once, but it is,
     // so page() could be null.
     if (page() && page()->focusController().focusedFrame() == this)
         page()->focusController().setFocusedFrame(nullptr);
     script().clearScriptObjects();
 
     if (page() && page()->scrollingCoordinator() && m_view)
         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())
         return createMarkup(doctype);
 
     return String();
 }
 
 String LocalFrame::selectedText() const
 {
@@ skipping 216 matching lines @@
     for (RefPtrWillBeRawPtr<Frame> child = tree().firstChild(); child; child = child->tree().nextSibling()) {
         if (child->isLocalFrame())
             toLocalFrame(child.get())->deviceOrPageScaleFactorChanged();
     }
 }
 
 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()) {
         if (!frame->isLocalFrame())
             continue;
         if (equalIgnoringFragmentIdentifier(toLocalFrame(frame)->document()->url(), url)) {
             if (foundSelfReference)
                 return false;
             foundSelfReference = true;
         }
@@ skipping 137 matching lines @@
 
     return curFrame;
 }
 
 void LocalFrame::setPagePopupOwner(Element& owner)
 {
     m_pagePopupOwner = &owner;
 }
 
 } // namespace blink