PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a navigation in startLoad(

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights
  * reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  * (http://www.torchmobile.com/)
  * Copyright (C) 2008 Alp Toker <alp@atoker.com>
  * Copyright (C) Research In Motion Limited 2009. All rights reserved.
  * Copyright (C) 2011 Kris Jordan <krisjordan@gmail.com>
  * Copyright (C) 2011 Google Inc. All rights reserved.
@@ skipping 1715 matching lines @@
       determineRequestContextFromNavigationType(navigationType));
   resourceRequest.setFrameType(m_frame->isMainFrame()
                                    ? WebURLRequest::FrameTypeTopLevel
                                    : WebURLRequest::FrameTypeNested);
 
   if (!checkLoadCanStart(frameLoadRequest, type, navigationPolicy,
                          navigationType)) {
     // 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 (m_isNavigationHandledByClient &&

[Nate Chapin, 2017/02/03 19:35:22]

Nit: Can we make this if() block part of the new if
(m_isNavigationHandledByClient) {) block?

[ananta, 2017/02/03 20:42:00]

Done.

         !frameLoadRequest.resourceRequest().checkForBrowserSideNavigation()) {
       m_isNavigationHandledByClient = false;
     }
+    if (m_isNavigationHandledByClient) {
+      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();
+    }
     return;
   }
 
-  m_provisionalDocumentLoader = client()->createDocumentLoader(
-      m_frame, resourceRequest,
-      frameLoadRequest.substituteData().isValid()
-          ? frameLoadRequest.substituteData()
-          : defaultSubstituteDataForURL(resourceRequest.url()),
-      frameLoadRequest.clientRedirect());
-  m_provisionalDocumentLoader->setLoadType(type);
-  m_provisionalDocumentLoader->setNavigationType(navigationType);
-  m_provisionalDocumentLoader->setReplacesCurrentHistoryItem(
-      type == FrameLoadTypeReplaceCurrentItem);
+  m_provisionalDocumentLoader = createDocumentLoader(
+      resourceRequest, frameLoadRequest, type, navigationType);
 
   // PlzNavigate: We need to ensure that script initiated navigations are
   // honored.
   if (!m_isNavigationHandledByClient) {
     m_frame->navigationScheduler().cancel();
     m_checkTimer.stop();
   }
 
   if (frameLoadRequest.form())
     client()->dispatchWillSubmitForm(frameLoadRequest.form());
 
   // 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());
-  client()->dispatchDidStartProvisionalLoad();
+  // 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/02/03 19:35:22]

Why did this move back here? No objections, just curious :)

[ananta, 2017/02/03 20:42:00]

Sending the provisional load notification early causes a number of tests to
break in the non plznavigate case. Turns out that there are assumptions that the
current page unload handlers would be called before subsequent load
notifications etc.

   DCHECK(m_provisionalDocumentLoader);
+
   m_provisionalDocumentLoader->startLoadingMainResource();
 
   takeObjectSnapshot();
 }
 
 void FrameLoader::applyUserAgent(ResourceRequest& request) {
   String userAgent = this->userAgent();
   DCHECK(!userAgent.isNull());
   request.setHTTPUserAgent(AtomicString(userAgent));
 }
@@ skipping 170 matching lines @@
   tracedValue->setString("documentLoaderURL",
                          m_documentLoader ? m_documentLoader->url() : String());
   return tracedValue;
 }
 
 inline void FrameLoader::takeObjectSnapshot() const {
   TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID("loading", "FrameLoader", this,
                                       toTracedValue());
 }
 
+DocumentLoader* FrameLoader::createDocumentLoader(
+    const ResourceRequest& request,
+    const FrameLoadRequest& frameLoadRequest,
+    FrameLoadType loadType,
+    NavigationType navigationType) {
+  DocumentLoader* loader = client()->createDocumentLoader(
+      m_frame, request, frameLoadRequest.substituteData().isValid()
+                            ? frameLoadRequest.substituteData()
+                            : defaultSubstituteDataForURL(request.url()),
+      frameLoadRequest.clientRedirect());
+
+  loader->setLoadType(loadType);
+  loader->setNavigationType(navigationType);
+  loader->setReplacesCurrentHistoryItem(loadType ==
+                                        FrameLoadTypeReplaceCurrentItem);
+  return loader;
+}
+
 }  // namespace blink