Make the embedder responsible for creating the WebFrame

Source/web/WebFrameImpl.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 32 matching lines @@
 //
 // FrameLoader and Frame are formerly one object that was split apart because
 // it got too big. They basically have the same lifetime, hence the double line.
 //
 // WebFrame is refcounted and has one ref on behalf of the FrameLoader/Frame.
 // This is not a normal reference counted pointer because that would require
 // changing WebKit code that we don't control. Instead, it is created with this
 // ref initially and it is removed when the FrameLoader is getting destroyed.
 //
 // WebFrames are created in two places, first in WebViewImpl when the root
-// frame is created, and second in WebFrame::CreateChildFrame when sub-frames
+// frame is created, and second in WebFrame::createChildFrame when sub-frames
 // are created. WebKit will hook up this object to the FrameLoader/Frame
 // and the refcount will be correct.
 //
 // How frames are destroyed
 // ------------------------
 //
 // The main frame is never destroyed and is re-used. The FrameLoader is re-used
 // and a reference to the main frame is kept by the Page.
 //
 // When frame content is replaced, all subframes are destroyed. This happens
 // in FrameLoader::detachFromParent for each subframe.
 //
 // Frame going away causes the FrameLoader to get deleted. In FrameLoader's
-// destructor, it notifies its client with frameLoaderDestroyed. This calls
-// WebFrame::Closing and then derefs the WebFrame and will cause it to be
-// deleted (unless an external someone is also holding a reference).
+// destructor, it notifies its client with frameLoaderDestroyed. This derefs
+// the WebFrame and will cause it to be deleted (unless an external someone
+// is also holding a reference).
+//
+// Thie client is expected to be set whenever the WebFrameImpl is attached to
+// the DOM.
 
 #include "config.h"
 #include "WebFrameImpl.h"
 
 #include "AssociatedURLLoader.h"
 #include "DOMUtilitiesPrivate.h"
 #include "EventListenerWrapper.h"
 #include "FindInPageCoordinates.h"
 #include "HTMLNames.h"
 #include "PageOverlay.h"
@@ skipping 179 matching lines @@
         if (output.length() >= maxChars - frameSeparatorLength)
             return;
 
         output.append(frameSeparator, frameSeparatorLength);
         frameContentAsPlainText(maxChars, curChild, output);
         if (output.length() >= maxChars)
             return; // Filled up the buffer.
     }
 }
 
-static long long generateFrameIdentifier()
-{
-    static long long next = 0;
-    return ++next;
-}
-
 WebPluginContainerImpl* WebFrameImpl::pluginContainerFromFrame(Frame* frame)
 {
     if (!frame)
         return 0;
     if (!frame->document() || !frame->document()->isPluginDocument())
         return 0;
     PluginDocument* pluginDocument = static_cast<PluginDocument*>(frame->document());
     return toPluginContainerImpl(pluginDocument->pluginWidget());
 }
 
@@ skipping 244 matching lines @@
 WebFrame* WebFrame::frameForContext(v8::Handle<v8::Context> context)
 {
    return WebFrameImpl::fromFrame(toFrameIfNotDetached(context));
 }
 
 WebFrame* WebFrame::fromFrameOwnerElement(const WebElement& element)
 {
     return WebFrameImpl::fromFrameOwnerElement(PassRefPtr<Element>(element).get());
 }
 
+void WebFrameImpl::close()

[darin (slow to review), 2013/09/13 04:02:47]

not that you should clean this up now, but WebFrameClient has a strange method
named willClose() that has no relation to this new method or
WebFrameClient::frameDetached(). it actually corresponds to the document for the
frame changing to another document. willClose means "the old document is being
closed."

+{
+    m_client = 0;
+    deref();  // Balances ref() acquired in WebFrame::create

[darin (slow to review), 2013/09/13 04:02:47]

nit: WebFrameImpl::create

+}

[darin (slow to review), 2013/09/13 04:02:47]

you might want to add a comment here explaining what this function is doing. you
might explain what it means for the embedder to signal that it has closed the
WebFrame, etc.

+
 WebString WebFrameImpl::uniqueName() const
 {
     return frame()->tree()->uniqueName();
 }
 
 WebString WebFrameImpl::assignedName() const
 {
     return frame()->tree()->name();
 }
 
@@ skipping 1517 matching lines @@
 WebString WebFrameImpl::layerTreeAsText(bool showDebugInfo) const
 {
     if (!frame())
         return WebString();
 
     return WebString(frame()->layerTreeAsText(showDebugInfo ? LayerTreeIncludesDebugInfo : LayerTreeNormal));
 }
 
 // WebFrameImpl public ---------------------------------------------------------
 
-PassRefPtr<WebFrameImpl> WebFrameImpl::create(WebFrameClient* client)
+WebFrame* WebFrame::create(WebFrameClient* client)
 {
-    return adoptRef(new WebFrameImpl(client));
+    return WebFrameImpl::create(client, generateFrameIdentifier());
 }
 
-WebFrameImpl::WebFrameImpl(WebFrameClient* client)
+WebFrame* WebFrame::create(WebFrameClient* client, long long identifier)
+{
+    return WebFrameImpl::create(client, identifier);
+}
+
+long long WebFrame::generateFrameIdentifier()
+{
+    static long long next = 0;
+    // Assume that 64-bit will not wrap to -1.
+    return ++next;
+}
+
+WebFrameImpl* WebFrameImpl::create(WebFrameClient* client, long long identifier)
+{
+    return adoptRef(new WebFrameImpl(client, identifier)).leakRef();

[darin (slow to review), 2013/09/13 04:02:47]

nit: add a comment indicating that this leak gets cleaned up in
WebFrameImpl::close()

+}
+
+WebFrameImpl::WebFrameImpl(WebFrameClient* client, long long identifier)
     : FrameDestructionObserver(0)
     , m_frameLoaderClient(this)
     , m_client(client)
     , m_currentActiveMatchFrame(0)
     , m_activeMatchIndexInCurrentFrame(-1)
     , m_locatingActiveRect(false)
     , m_resumeScopingFromRange(0)
     , m_lastMatchCount(-1)
     , m_totalMatchCount(-1)
     , m_framesScopingCount(-1)
     , m_findRequestIdentifier(-1)
     , m_scopingInProgress(false)
     , m_lastFindRequestCompletedWithNoMatches(false)
     , m_nextInvalidateAfter(0)
     , m_findMatchMarkersVersion(0)
     , m_findMatchRectsAreValid(false)
-    , m_identifier(generateFrameIdentifier())
+    , m_identifier(identifier)

[darin (slow to review), 2013/09/13 04:02:47]

We should just move the concept of WebFrame identifier to Chromium. it doesn't
need to be part of Blink at all.  it was a mistake that it was ever put here.
this identifier can be a property of the content::RenderFrameImpl on the
Chromium side.

[awong, 2013/09/13 04:25:02]

I did this initially, but didn't know how to handle WebSharedWorkerImpl.cpp and
WebHelperPluginImpl.cpp. Both these create a WebFrameImpl outside the purview of
RenderFrameImpl.

WebHelplerPluginImpl in particular uses the mainframe's render_frame_impl as the
client.  I have no idea whether or not WebHelperPluginImpl executes resource
load requests.  If it does, then I think it needs to have a unique WebFrame
identifier which means I can't move the WebFrame identifier into render_view
just yet.

Do you know whether it'd be okay for WebHelperPluginImpl to not have a sensible
WebFrame identifier?

Further more, since the per-frame RenderFrameHostImpl creation is sitting behind
a flag, I can't make this transition yet.  Otherwise, I think I would break
resource loads on trunk. :'(

     , m_inSameDocumentHistoryLoad(false)
 {
     WebKit::Platform::current()->incrementStatsCounter(webFrameActiveCount);
     frameCount++;
 }
 
 WebFrameImpl::~WebFrameImpl()
 {
     WebKit::Platform::current()->decrementStatsCounter(webFrameActiveCount);
     frameCount--;
 
     cancelPendingScopingEffort();
 }
 
 void WebFrameImpl::setWebCoreFrame(WebCore::Frame* frame)
 {
     ASSERT(frame);
     observeFrame(frame);
 }
 
 void WebFrameImpl::initializeAsMainFrame(WebCore::Page* page)
 {
     RefPtr<Frame> mainFrame = Frame::create(page, 0, &m_frameLoaderClient);
     setWebCoreFrame(mainFrame.get());
 
-    // Add reference on behalf of FrameLoader.  See comments in
+    // Add reference on behalf of FrameLoader. See comments in
     // WebFrameLoaderClient::frameLoaderDestroyed for more info.
     ref();
 
     // We must call init() after m_frame is assigned because it is referenced
     // during init().
     frame()->init();
 }
 
 PassRefPtr<Frame> WebFrameImpl::createChildFrame(const FrameLoadRequest& request, HTMLFrameOwnerElement* ownerElement)
 {
-    RefPtr<WebFrameImpl> webframe(adoptRef(new WebFrameImpl(m_client)));
+    ASSERT(m_client);
+    WebFrameImpl* webframe = static_cast<WebFrameImpl*>(m_client->createChildFrame(this, request.frameName()));
 
     // Add an extra ref on behalf of the Frame/FrameLoader, which references the
     // WebFrame via the FrameLoaderClient interface. See the comment at the top
     // of this file for more info.
     webframe->ref();
 
     RefPtr<Frame> childFrame = Frame::create(frame()->page(), ownerElement, &webframe->m_frameLoaderClient);
     webframe->setWebCoreFrame(childFrame.get());
 
     childFrame->tree()->setName(request.frameName());
 
     frame()->tree()->appendChild(childFrame);
 
     // Frame::init() can trigger onload event in the parent frame,
     // which may detach this frame and trigger a null-pointer access
     // in FrameTree::removeChild. Move init() after appendChild call
     // so that webframe->mFrame is in the tree before triggering
     // onload event handler.
     // Because the event handler may set webframe->mFrame to null,
     // it is necessary to check the value after calling init() and
     // return without loading URL.
     // (b:791612)
     childFrame->init(); // create an empty document
-    if (!childFrame->tree()->parent())
+    if (!childFrame->tree()->parent()) {
+        if (m_client)
+            m_client->frameDetached(webframe);

[darin (slow to review), 2013/09/13 04:02:47]

there have been bugs where it appeared from the chromium side that frameDetached
was not called when it should have been. i wonder if you found the culprit to
those bugs.

         return 0;
+    }
 
     HistoryItem* parentItem = frame()->loader()->history()->currentItem();
     HistoryItem* childItem = 0;
     // If we're moving in the back/forward list, we might want to replace the content
     // of this child frame with whatever was there at that point.
     if (parentItem && parentItem->children().size() && isBackForwardLoadType(frame()->loader()->loadType()) && !frame()->document()->loadEventFinished())
         childItem = parentItem->childItemWithTarget(childFrame->tree()->uniqueName());
 
     if (childItem)
         childFrame->loader()->loadHistoryItem(childItem);
     else
         childFrame->loader()->load(FrameLoadRequest(0, request.resourceRequest(), "_self"));
 
     // A synchronous navigation (about:blank) would have already processed
     // onload, so it is possible for the frame to have already been destroyed by
     // script in the page.
-    if (!childFrame->tree()->parent())
+    if (!childFrame->tree()->parent()) {
+        if (m_client)
+            m_client->frameDetached(webframe);
         return 0;
+    }
 
-    if (m_client)
-        m_client->didCreateFrame(this, webframe.get());
 
     return childFrame.release();
 }
 
 void WebFrameImpl::didChangeContentsSize(const IntSize& size)
 {
     // This is only possible on the main frame.
     if (m_totalMatchCount > 0) {
         ASSERT(!parent());
         ++m_findMatchMarkersVersion;
@@ skipping 284 matching lines @@
 
         // There is a possibility that the frame being detached was the only
         // pending one. We need to make sure final replies can be sent.
         flushCurrentScopingEffort(m_findRequestIdentifier);
 
         cancelPendingScopingEffort();
     }
 }
 
 } // namespace WebKit