Move HistoryItem handling to DocumentLoader

third_party/WebKit/Source/web/WebLocalFrameImpl.cpp

Index: third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
diff --git a/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp b/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
index ebda68aa738d402ad920e1bc0c4419b893699343..953faa2d1e72085005d42babf594b219cdc7d498 100644
--- a/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
+++ b/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
@@ -1678,10 +1678,9 @@ LocalFrame* WebLocalFrameImpl::createChildFrame(
   FrameLoadRequest newRequest = request;
   FrameLoadType loadType = FrameLoadTypeStandard;
   if (childItem) {
-    newRequest = FrameLoadRequest(
-        request.originDocument(),
-        FrameLoader::resourceRequestFromHistoryItem(
-            childItem, WebCachePolicy::UseProtocolCachePolicy));
+    newRequest = FrameLoadRequest(request.originDocument(),
+                                  childItem->generateResourceRequest(
+                                      WebCachePolicy::UseProtocolCachePolicy));
     loadType = FrameLoadTypeInitialHistoryLoad;
   }
   webframeChild->frame()->loader().load(newRequest, loadType, childItem);
@@ -1995,9 +1994,8 @@ WebURLRequest WebLocalFrameImpl::requestFromHistoryItem(
     const WebHistoryItem& item,
     WebCachePolicy cachePolicy) const {
   HistoryItem* historyItem = item;
-  ResourceRequest request =
-      FrameLoader::resourceRequestFromHistoryItem(historyItem, cachePolicy);
-  return WrappedResourceRequest(request);
+  return WrappedResourceRequest(
+      historyItem->generateResourceRequest(cachePolicy));
 }
 
 WebURLRequest WebLocalFrameImpl::requestForReload(
@@ -2052,9 +2050,16 @@ void WebLocalFrameImpl::loadData(const WebData& data,
   // unreachableURL informs FrameLoader::reload to load unreachableURL
   // instead of the currently loaded URL.
   ResourceRequest request;
-  if (replace && !unreachableURL.isEmpty() &&
-      frame()->loader().provisionalDocumentLoader())
-    request = frame()->loader().provisionalDocumentLoader()->originalRequest();
+  HistoryItem* historyItem = item;
+  DocumentLoader* provisionalDocumentLoader =
+      frame()->loader().provisionalDocumentLoader();
+  if (replace && !unreachableURL.isEmpty() && provisionalDocumentLoader) {
+    request = provisionalDocumentLoader->originalRequest();
+    if (provisionalDocumentLoader->loadType() == FrameLoadTypeBackForward) {
+      historyItem = provisionalDocumentLoader->historyItem();

[yhirano, 2017/03/29 09:38:13]

Where is this line (i.e., L2059) from?

[Nate Chapin, 2017/03/29 18:23:06]

Yeah, I should have mentioned this, sorry. This line is why I felt I had to move
this logic out of determineFrameLoadType.

This block handles the case where a back/forward navigation fails and we want to
replace it with an error page. Previously, the history state portion of that
logic worked magically: FrameLoader::m_provisionalItem would be set by the
back/forward navgation, and it would *not* be cleared before the error page
navigation started. So the error page would just pick up the existing
m_provisionalItem and use it when it committed.

That doesn't work with this change, because the provisional HistoryItem lives on
the provisional DocumentLoader, and the error page navigation will replace the
provsional DocumentLoader. Therefore, we need to grab the provisional
DocumentLoader's HistoryItem before we create the error page's provisional
DocumentLoader. determineFrameLoadType() could grab it, but would need a special
case to return it to the caller. Since some of the error page replacement logic
was here already, I thought it best to move it all here, especially since we're
passing a HistoryItem here anyway.

[kinuko, 2017/03/30 05:05:00]

Could we note some of this as code comment, e.g. 'this is for error page
handling and we need the history item before provisional docloader is replaced'
?

[Nate Chapin, 2017/03/30 20:13:56]

Done.

+      webFrameLoadType = WebFrameLoadType::BackForward;
+    }
+  }
   request.setURL(baseURL);
   request.setCheckForBrowserSideNavigation(false);
 
@@ -2065,7 +2070,6 @@ void WebLocalFrameImpl::loadData(const WebData& data,
   if (isClientRedirect)
     frameRequest.setClientRedirect(ClientRedirectPolicy::ClientRedirect);
 
-  HistoryItem* historyItem = item;
   frame()->loader().load(
       frameRequest, static_cast<FrameLoadType>(webFrameLoadType), historyItem,
       static_cast<HistoryLoadType>(webHistoryLoadType));
@@ -2078,9 +2082,8 @@ bool WebLocalFrameImpl::maybeRenderFallbackContent(
   if (!frame()->owner() || !frame()->owner()->canRenderFallbackContent())
     return false;
 
-  FrameLoader& frameloader = frame()->loader();
-  DCHECK(frameloader.provisionalDocumentLoader());
-  frameloader.loadFailed(frameloader.provisionalDocumentLoader(), error);
+  DCHECK(frame()->loader().provisionalDocumentLoader());
+  frame()->loader().provisionalDocumentLoader()->loadFailed(error);
   return true;
 }