Cleanup blink-side PlzNavigate logic

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 c727b4ebb19cd9c57a410e7dfd4972325e2bf91d..5bf1db2a5d46f1934e5160969805aa67c39e592f 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -189,8 +189,7 @@ FrameLoader::FrameLoader(LocalFrame* frame)
                    &FrameLoader::checkTimerFired),
       m_forcedSandboxFlags(SandboxNone),
       m_dispatchingDidClearWindowObjectInMainWorld(false),
-      m_protectProvisionalLoader(false),
-      m_isNavigationHandledByClient(false) {
+      m_protectProvisionalLoader(false) {
   DCHECK(m_frame);
   TRACE_EVENT_OBJECT_CREATED_WITH_ID("loading", "FrameLoader", this);
   takeObjectSnapshot();
@@ -703,8 +702,7 @@ static bool shouldSendFinishNotification(LocalFrame* frame) {
   return true;
 }
 
-static bool shouldSendCompleteNotification(LocalFrame* frame,
-                                           bool isNavigationHandledByClient) {
+static bool shouldSendCompleteNotification(LocalFrame* frame) {
   // FIXME: We might have already sent stop notifications and be re-completing.
   if (!frame->isLoading())
     return false;
@@ -744,7 +742,7 @@ void FrameLoader::checkCompleted() {
       return;
   }
 
-  if (shouldSendCompleteNotification(m_frame, m_isNavigationHandledByClient)) {
+  if (shouldSendCompleteNotification(m_frame)) {
     m_progressTracker->progressCompleted();
     // Retry restoring scroll offset since finishing loading disables content
     // size clamping.
@@ -865,9 +863,6 @@ void FrameLoader::loadInSameDocument(
   // scroll should cancel it.
   detachDocumentLoader(m_provisionalDocumentLoader);
 
-  // PlzNavigate: A fragment scroll should clear ongoing client navigations.
-  clearNavigationHandledByClient();
-
   if (!m_frame->host())
     return;
   saveScrollState();
@@ -1245,13 +1240,6 @@ void FrameLoader::stopAllLoaders() {
 
   m_inStopAllLoaders = true;
 
-  if (m_isNavigationHandledByClient) {
-    client()->dispatchDidFailProvisionalLoad(
-        ResourceError::cancelledError(String()), StandardCommit);
-  }
-
-  clearNavigationHandledByClient();
-
   for (Frame* child = m_frame->tree().firstChild(); child;
        child = child->tree().nextSibling()) {
     if (child->isLocalFrame())
@@ -1383,10 +1371,7 @@ void FrameLoader::commitProvisionalLoad() {
 
   client()->transitionToCommittedForNewPage();
 
-  // PlzNavigate: We need to ensure that script initiated navigations are
-  // honored.
-  if (!m_isNavigationHandledByClient)
-    m_frame->navigationScheduler().cancel();
+  m_frame->navigationScheduler().cancel();
 
   m_frame->editor().clearLastEditCommand();
 
@@ -1515,6 +1500,8 @@ void FrameLoader::loadFailed(DocumentLoader* loader,
   HistoryCommitType historyCommitType =
       loadTypeToCommitType(loader->loadType());
   if (loader == m_provisionalDocumentLoader) {
+    if (!m_provisionalDocumentLoader->didStart())
+      InspectorInstrumentation::frameClearedScheduledClientNavigation(m_frame);
     m_provisionalDocumentLoader->setSentDidFinishLoad();
     client()->dispatchDidFailProvisionalLoad(error, historyCommitType);
     if (loader != m_provisionalDocumentLoader)
@@ -1626,7 +1613,7 @@ bool FrameLoader::shouldClose(bool isReload) {
   return shouldClose;
 }
 
-bool FrameLoader::shouldContinueForNavigationPolicy(
+NavigationPolicy FrameLoader::shouldContinueForNavigationPolicy(
     const ResourceRequest& request,
     const SubstituteData& substituteData,
     DocumentLoader* loader,
@@ -1638,7 +1625,7 @@ bool FrameLoader::shouldContinueForNavigationPolicy(
     HTMLFormElement* form) {
   // Don't ask if we are loading an empty URL.
   if (request.url().isEmpty() || substituteData.isValid())
-    return true;
+    return NavigationPolicyCurrentTab;
 
   // If we're loading content into |m_frame| (NavigationPolicyCurrentTab), check
   // against the parent's Content Security Policy and kill the load if that
@@ -1656,7 +1643,7 @@ bool FrameLoader::shouldContinueForNavigationPolicy(
         // page load.
         m_frame->document()->enforceSandboxFlags(SandboxOrigin);
         m_frame->owner()->dispatchLoad();
-        return false;
+        return NavigationPolicyIgnore;
       }
     }
   }
@@ -1666,48 +1653,36 @@ bool FrameLoader::shouldContinueForNavigationPolicy(
   if (isFormSubmission &&
       !m_frame->document()->contentSecurityPolicy()->allowFormAction(
           request.url()))
-    return false;
+    return NavigationPolicyIgnore;
 
   bool replacesCurrentHistoryItem =
       frameLoadType == FrameLoadTypeReplaceCurrentItem;
   policy = client()->decidePolicyForNavigation(request, loader, type, policy,
                                                replacesCurrentHistoryItem,
                                                isClientRedirect, form);
-  if (policy == NavigationPolicyCurrentTab)
-    return true;
-  if (policy == NavigationPolicyIgnore)
-    return false;
-  if (policy == NavigationPolicyHandledByClient) {
-    setNavigationHandledByClient();
-    // Mark the frame as loading since the embedder is handling the navigation.
-    m_progressTracker->progressStarted(frameLoadType);
-
-    m_frame->navigationScheduler().cancel();
-
-    // If this is a form submit, dispatch that a form is being submitted
-    // since the embedder is handling the navigation.
-    if (form)
-      client()->dispatchWillSubmitForm(form);
-
-    m_frame->document()->cancelParsing();
-
-    return false;
+  if (policy == NavigationPolicyCurrentTab ||
+      policy == NavigationPolicyIgnore ||
+      policy == NavigationPolicyHandledByClient ||
+      policy == NavigationPolicyHandledByClientForInitialHistory) {
+    return policy;
   }
+
   if (!LocalDOMWindow::allowPopUp(*m_frame) &&
       !UserGestureIndicator::utilizeUserGesture())
-    return false;
+    return NavigationPolicyIgnore;
   client()->loadURLExternally(request, policy, String(),
                               replacesCurrentHistoryItem);
-  return false;
+  return NavigationPolicyIgnore;
 }
 
-bool FrameLoader::checkLoadCanStart(FrameLoadRequest& frameLoadRequest,
-                                    FrameLoadType type,
-                                    NavigationPolicy navigationPolicy,
-                                    NavigationType navigationType) {
+NavigationPolicy FrameLoader::checkLoadCanStart(
+    FrameLoadRequest& frameLoadRequest,
+    FrameLoadType type,
+    NavigationPolicy navigationPolicy,
+    NavigationType navigationType) {
   if (m_frame->document()->pageDismissalEventBeingDispatched() !=
       Document::NoDismissal) {
-    return false;
+    return NavigationPolicyIgnore;
   }
 
   // Record the latest requiredCSP value that will be used when sending this
@@ -1716,25 +1691,12 @@ bool FrameLoader::checkLoadCanStart(FrameLoadRequest& frameLoadRequest,
   recordLatestRequiredCSP();
   modifyRequestForCSP(resourceRequest, nullptr);
 
-  if (!shouldContinueForNavigationPolicy(
-          resourceRequest, frameLoadRequest.substituteData(), nullptr,
-          frameLoadRequest.shouldCheckMainWorldContentSecurityPolicy(),
-          navigationType, navigationPolicy, type,
-          frameLoadRequest.clientRedirect() ==
-              ClientRedirectPolicy::ClientRedirect,
-          frameLoadRequest.form())) {
-    return false;
-  }
-
-  m_frame->document()->cancelParsing();
-  detachDocumentLoader(m_provisionalDocumentLoader);
-
-  // beforeunload fired above, and detaching a DocumentLoader can fire events,
-  // which can detach this frame.
-  if (!m_frame->host())
-    return false;
-
-  return true;
+  return shouldContinueForNavigationPolicy(
+      resourceRequest, frameLoadRequest.substituteData(), nullptr,
+      frameLoadRequest.shouldCheckMainWorldContentSecurityPolicy(),
+      navigationType, navigationPolicy, type,
+      frameLoadRequest.clientRedirect() == ClientRedirectPolicy::ClientRedirect,
+      frameLoadRequest.form());
 }
 
 void FrameLoader::startLoad(FrameLoadRequest& frameLoadRequest,
@@ -1751,50 +1713,46 @@ void FrameLoader::startLoad(FrameLoadRequest& frameLoadRequest,
                                    ? WebURLRequest::FrameTypeTopLevel
                                    : WebURLRequest::FrameTypeNested);
 
-  if (!checkLoadCanStart(frameLoadRequest, type, navigationPolicy,
-                         navigationType)) {
-    if (m_isNavigationHandledByClient) {
-      // PlzNavigate: if the navigation is a commit of a client-handled
-      // navigation, record that there is no longer a navigation handled by the
-      // client.
-      if (!frameLoadRequest.resourceRequest().checkForBrowserSideNavigation()) {
-        clearNavigationHandledByClient();
-      } else {
-        DocumentLoader* loader = createDocumentLoader(
-            resourceRequest, frameLoadRequest, type, navigationType);
-        // PlzNavigate: If the navigation is handled by the client, then the
-        // didFinishDocumentLoad() event occurs before the
-        // didStartProvisionalLoad() notification which occurs after the
-        // navigation
-        // is committed. This causes a number of layout tests to fail. We
-        // workaround this by invoking the didStartProvisionalLoad()
-        // notification
-        // here. Consumers of the didStartProvisionalLoad() notification rely on
-        // the provisional loader and save navigation state in it. We want to
-        // avoid
-        // this dependency on the provisional loader. For now we create a
-        // temporary
-        // loader and pass it to the didStartProvisionalLoad() function.
-        // TODO(ananta)
-        // We should get rid of the dependency on the DocumentLoader in
-        // consumers
-        // of
-        // the didStartProvisionalLoad() notification.
-        client()->dispatchDidStartProvisionalLoad(loader);
-        DCHECK(loader);
-        loader->setSentDidFinishLoad();
-        loader->detachFromFrame();
-      }
+  bool hadPlaceholderClientDocumentLoader =
+      m_provisionalDocumentLoader && !m_provisionalDocumentLoader->didStart();
+  navigationPolicy = checkLoadCanStart(frameLoadRequest, type, navigationPolicy,
+                                       navigationType);
+  if (navigationPolicy == NavigationPolicyIgnore) {
+    if (hadPlaceholderClientDocumentLoader &&
+        !resourceRequest.checkForBrowserSideNavigation()) {
+      detachDocumentLoader(m_provisionalDocumentLoader);
     }
     return;
   }
 
+  // For PlzNavigate placeholer DocumentLoaders, don't send failure callbacks

[ananta, 2017/03/01 23:13:17]

typo placeholder

[Nate Chapin, 2017/03/01 23:37:46]

Done.

+  // for a placeholder simply being replaced with a new DocumentLoader.
+  if (hadPlaceholderClientDocumentLoader)
+    m_provisionalDocumentLoader->setSentDidFinishLoad();

[ananta, 2017/03/01 23:15:23]

Where do we call client()->dispatchDidStartProvisionalLoad(loader) for
PlzNavigate?. We had this to get some layout tests to pass as they expected
willStartRequest to happen after didStartProvisionalLoad()

[Nate Chapin, 2017/03/01 23:37:46]

With this patch, we'll call it in the same spot as for non-PlzNavigate, in
FrameLoader::startLoad(), see comment below.

+  m_frame->document()->cancelParsing();
+  detachDocumentLoader(m_provisionalDocumentLoader);
+
+  // beforeunload fired above, and detaching a DocumentLoader can fire events,
+  // which can detach this frame.
+  if (!m_frame->host())
+    return;
+
+  m_progressTracker->progressStarted(type);
+  // TODO(japhet): This case wants to flag the frame as loading and do nothing
+  // else. It'd be nice if it could go through the placeholder DocumentLoader
+  // path, too.
+  if (navigationPolicy == NavigationPolicyHandledByClientForInitialHistory)
+    return;
+  DCHECK(navigationPolicy == NavigationPolicyCurrentTab ||
+         navigationPolicy == NavigationPolicyHandledByClient);
+
   m_provisionalDocumentLoader = createDocumentLoader(
       resourceRequest, frameLoadRequest, type, navigationType);
 
   // PlzNavigate: We need to ensure that script initiated navigations are
   // honored.
-  if (!m_isNavigationHandledByClient) {
+  if (!hadPlaceholderClientDocumentLoader ||
+      navigationPolicy == NavigationPolicyHandledByClient) {
     m_frame->navigationScheduler().cancel();
     m_checkTimer.stop();
   }
@@ -1802,29 +1760,23 @@ void FrameLoader::startLoad(FrameLoadRequest& frameLoadRequest,
   if (frameLoadRequest.form())
     client()->dispatchWillSubmitForm(frameLoadRequest.form());
 
-  bool isNavigationHandledByClient = m_isNavigationHandledByClient;
-  // If the loader wasn't waiting for the client to handle a navigation, update
-  // the progress tracker. Otherwise don't, as it was already notified before
-  // sending the navigation to teh client.
-  if (!m_isNavigationHandledByClient)
-    m_progressTracker->progressStarted(type);
-  else
-    m_isNavigationHandledByClient = false;
-
   m_provisionalDocumentLoader->appendRedirect(
       m_provisionalDocumentLoader->getRequest().url());
-  // TODO(ananta)
+  // TODO(ananta):
   // We should get rid of the dependency on the DocumentLoader in consumers of
   // the didStartProvisionalLoad() notification.
   client()->dispatchDidStartProvisionalLoad(m_provisionalDocumentLoader);

[Nate Chapin, 2017/03/01 23:37:46]

As mentioned above, this will be called for both PlzNavigate and legacy
navigation codepaths.

   DCHECK(m_provisionalDocumentLoader);
 
-  m_provisionalDocumentLoader->startLoadingMainResource();
-
-  // This should happen after the request is sent, so we don't use
-  // clearNavigationHandledByClient() above.
-  if (isNavigationHandledByClient)
+  if (navigationPolicy == NavigationPolicyCurrentTab) {

[Nate Chapin, 2017/03/01 23:37:46]

This if() clause is where the PlzNavigate and non-PlzNavigate paths diverge.
non-PlzNavigate will go through the if{} clause and call
startLoadingMainResource(), where PlzNavigate will go through the else{} and
just note that a navigation was scheduled.

+    m_provisionalDocumentLoader->startLoadingMainResource();
+    // This should happen after the request is sent, so that the state
+    // the inspector stored in the matching frameScheduledClientNavigation()
+    // is available while sending the request.
     InspectorInstrumentation::frameClearedScheduledClientNavigation(m_frame);
+  } else {
+    InspectorInstrumentation::frameScheduledClientNavigation(m_frame);
+  }
 
   takeObjectSnapshot();
 }
@@ -2032,14 +1984,4 @@ DocumentLoader* FrameLoader::createDocumentLoader(
   return loader;
 }
 
-void FrameLoader::setNavigationHandledByClient() {
-  m_isNavigationHandledByClient = true;
-  InspectorInstrumentation::frameScheduledClientNavigation(m_frame);
-}
-
-void FrameLoader::clearNavigationHandledByClient() {
-  m_isNavigationHandledByClient = false;
-  InspectorInstrumentation::frameClearedScheduledClientNavigation(m_frame);
-}
-
 }  // namespace blink