Reland "Move most of FrameLoader::CheckCompleted() to Document"

third_party/WebKit/Source/web/WebFrame.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "public/web/WebFrame.h"
 
+#include <algorithm>
 #include "bindings/core/v8/WindowProxyManager.h"
 #include "core/HTMLNames.h"
+#include "core/dom/IncrementLoadEventDelayCount.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/RemoteFrame.h"
 #include "core/html/HTMLFrameElementBase.h"
 #include "core/html/HTMLFrameOwnerElement.h"
 #include "core/page/Page.h"
 #include "platform/UserGestureIndicator.h"
 #include "platform/heap/Handle.h"
 #include "platform/instrumentation/tracing/TraceEvent.h"
 #include "public/web/WebElement.h"
 #include "public/web/WebFrameOwnerProperties.h"
 #include "public/web/WebSandboxFlags.h"
 #include "web/OpenedFrameTracker.h"
 #include "web/RemoteFrameOwner.h"
 #include "web/WebLocalFrameImpl.h"
 #include "web/WebRemoteFrameImpl.h"
-#include <algorithm>
 
 namespace blink {
 
 bool WebFrame::Swap(WebFrame* frame) {
   using std::swap;
   Frame* old_frame = ToImplBase()->GetFrame();
   if (!old_frame->IsAttached())
     return false;
 
   // Unload the current Document in this frame: this calls unload handlers,
   // detaches child frames, etc. Since this runs script, make sure this frame
   // wasn't detached before continuing with the swap.
   // FIXME: There is no unit test for this condition, so one needs to be
   // written.
   if (!old_frame->PrepareForCommit())
     return false;
 
+  // If there is a local parent, it might incorrectly declare itself complete
+  // during the detach phase of this swap. Suppress its completion until swap is
+  // over, at which point its completion will be correctly dependent on its
+  // newly swapped-in child.
+  std::unique_ptr<IncrementLoadEventDelayCount> delay_parent_load =

[lfg, 2017/04/27 17:08:25]

This looks good, but do you mind explaining why this wasn't needed before? It
seems that we are now calling CheckCompleted where we weren't before.

Also, by the time the detach happens, we already swapped the old frame from the
FrameTree, so why is the new frame allowed to declare itself complete?

+      parent_ && parent_->IsWebLocalFrame()
+          ? IncrementLoadEventDelayCount::Create(
+                *ToWebLocalFrameImpl(parent_)->GetFrame()->GetDocument())
+          : nullptr;
+
   if (parent_) {
     if (parent_->first_child_ == this)
       parent_->first_child_ = frame;
     if (parent_->last_child_ == this)
       parent_->last_child_ = frame;
     // FIXME: This is due to the fact that the |frame| may be a provisional
     // local frame, because we don't know if the navigation will result in
     // an actual page or something else, like a download. The PlzNavigate
     // project will remove the need for provisional local frames.
     frame->parent_ = parent_;
@@ skipping 267 matching lines @@
   for (WebFrame* child = frame->FirstChild(); child;
        child = child->NextSibling())
     TraceFrame(visitor, child);
 }
 
 void WebFrame::Close() {
   opened_frame_tracker_->Dispose();
 }
 
 }  // namespace blink