Simplify FrameLoader::Clear(), remove FrameLoader::check_timer_

third_party/WebKit/Source/core/loader/FrameLoader.cpp

Index: third_party/WebKit/Source/core/loader/FrameLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index 1b3632431e4b013f2f6ec9ae8f49fac18325b5a0..d4e6a0a8071a0dcab727bff54a67d0322e293fcf 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -239,9 +239,6 @@ FrameLoader::FrameLoader(LocalFrame* frame)
     : frame_(frame),
       progress_tracker_(ProgressTracker::Create(frame)),
       in_stop_all_loaders_(false),
-      check_timer_(TaskRunnerHelper::Get(TaskType::kNetworking, frame),
-                   this,
-                   &FrameLoader::CheckTimerFired),
       forced_sandbox_flags_(kSandboxNone),
       dispatching_did_clear_window_object_in_main_world_(false),
       protect_provisional_loader_(false),
@@ -303,10 +300,8 @@ void FrameLoader::SetDefersLoading(bool defers) {
       document->ResumeScheduledTasks();
   }
 
-  if (!defers) {
+  if (!defers)
     frame_->GetNavigationScheduler().StartTimer();
-    ScheduleCheckCompleted();

[Nate Chapin, 2017/05/04 19:53:12]

This dates back to
https://chromium.googlesource.com/chromium/src/+/8ec6d2367142e649bb9ef374c259...,
which claims it "might have been fired and ignored while defersLoading was
true." I'm pretty sure that's not true anymore.

Also, I can't think of a situation in which we would meet the last requirement
to complete during deferral. All the things that block completion should be
stopped during deferral.

-  }
 }
 
 void FrameLoader::SaveScrollState() {
@@ -374,17 +369,10 @@ void FrameLoader::Clear() {
     return;
 
   frame_->GetEditor().Clear();
-  frame_->GetDocument()->RemoveFocusedElementOfSubtree(frame_->GetDocument());

[Nate Chapin, 2017/05/04 19:53:12]

Documnet::Shutdown() clears the focused element unconditionally, I don't think
this is necessary.

   frame_->GetEventHandler().Clear();
   if (frame_->View())
     frame_->View()->Clear();
 
-  frame_->GetScriptController().EnableEval();

[Nate Chapin, 2017/05/04 19:53:12]

Moved to ScriptController::UpdateDocument, which should be called shortly after
Clear() (except in the frame-detach case, in which case it's irrelevant).

-
-  frame_->GetNavigationScheduler().Cancel();

[Nate Chapin, 2017/05/04 19:53:12]

In the frame-detach case, StopAllLoaders() will cover this. In the
standard-commit case, CommitProvisionalLoad() cover this. In the javascript: url
case, we check that a location change isn't pending before replacing the
document. Ergo, I think this is vestigial.

-
-  check_timer_.Stop();
-
   if (state_machine_.IsDisplayingInitialEmptyDocument())
     state_machine_.AdvanceTo(FrameLoaderStateMachine::kCommittedFirstRealLoad);
 
@@ -490,19 +478,6 @@ void FrameLoader::DidFinishNavigation() {
     ToLocalFrame(parent)->GetDocument()->CheckCompleted();
 }
 
-void FrameLoader::CheckTimerFired(TimerBase*) {
-  if (Page* page = frame_->GetPage()) {
-    if (page->Suspended())
-      return;
-  }
-  frame_->GetDocument()->CheckCompleted();
-}
-
-void FrameLoader::ScheduleCheckCompleted() {
-  if (!check_timer_.IsActive())
-    check_timer_.StartOneShot(0, BLINK_FROM_HERE);
-}
-
 Frame* FrameLoader::Opener() {
   return Client() ? Client()->Opener() : 0;
 }
@@ -981,7 +956,6 @@ void FrameLoader::StopAllLoaders() {
   if (!protect_provisional_loader_)
     DetachDocumentLoader(provisional_document_loader_);
 
-  check_timer_.Stop();
   frame_->GetNavigationScheduler().Cancel();
 
   // It's possible that the above actions won't have stopped loading if load
@@ -1209,9 +1183,6 @@ void FrameLoader::Detach() {
   DetachDocumentLoader(document_loader_);
   DetachDocumentLoader(provisional_document_loader_);
 
-  Frame* parent = frame_->Tree().Parent();
-  if (parent && parent->IsLocalFrame())
-    ToLocalFrame(parent)->Loader().ScheduleCheckCompleted();

[Nate Chapin, 2017/05/04 19:53:12]

Instead of scheduling a CheckCompleted() here, have HTMLFrameOwnerElement
directly call CheckCompleted() when its ContentFrame() is cleared, as that is
one of the very last steps of frame detach.

[dcheng, 2017/05/05 06:12:31]

It's unclear to me if there are edge cases that this might miss. For example, if
we have main frame A - child B - grandchild A, then detaching grandchild A
wouldn't call CheckCompleted(), because grandchild A's owner is not a
HTMLFrameOwnerElement.

I guess maybe this is already an existing problem though?

   if (progress_tracker_) {
     progress_tracker_->Dispose();
     progress_tracker_.Clear();
@@ -1490,7 +1461,6 @@ void FrameLoader::StartLoad(FrameLoadRequest& frame_load_request,
   if (!had_placeholder_client_document_loader ||
       navigation_policy == kNavigationPolicyHandledByClient) {
     frame_->GetNavigationScheduler().Cancel();
-    check_timer_.Stop();
   }
 
   if (frame_load_request.Form())