Remove the HistoryState property of WebRequest.

In the new WebKit API, it seems best if WebRequest is just a wrapper for
WebCore::ResourceRequest since in most contexts that's what we need it to
be.

The solution here is to introduce a LoadHistoryState method on WebFrame that
can be used to navigate to a session history item.

BUG=10038
TEST=covered by existing back/forward navigation tests.
R=brettw

Review URL: http://codereview.chromium.org/113758

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@16747 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 84 insertions, 78 deletions

diff --git a/chrome/renderer/render_view.cc b/chrome/renderer/render_view.cc
index 79ddf45..9e669fd 100644
--- a/chrome/renderer/render_view.cc
+++ b/chrome/renderer/render_view.cc
@@ -794,18 +794,6 @@ void RenderView::OnNavigate(const ViewMsg_Navigate_Params& params) {
     is_reload = false;
   }
 
-  WebRequestCachePolicy cache_policy;
-  if (is_reload) {
-    cache_policy = WebRequestReloadIgnoringCacheData;
-  } else if (params.page_id != -1 || main_frame->GetInViewSourceMode()) {
-    cache_policy = WebRequestReturnCacheDataElseLoad;
-  } else {
-    cache_policy = WebRequestUseProtocolCachePolicy;
-  }
-
-  scoped_ptr<WebRequest> request(WebRequest::Create(params.url));
-  request->SetCachePolicy(cache_policy);
-
   // A navigation resulting from loading a javascript URL should not be treated
   // as a browser initiated event.  Instead, we want it to look as if the page
   // initiated any load resulting from JS execution.
@@ -814,17 +802,39 @@ void RenderView::OnNavigate(const ViewMsg_Navigate_Params& params) {
         params.page_id, params.transition, params.request_time));
   }
 
-  // If we are reloading, then WebKit will use the state of the current page.
-  // Otherwise, we give it the state to navigate to.
-  if (!is_reload)
-    request->SetHistoryState(params.state);
+  // If we are reloading, then WebKit will use the history state of the current
+  // page, so we should just ignore any given history state.  Otherwise, if we
+  // have history state, then we need to navigate to it, which corresponds to a
+  // back/forward navigation event.
+  if (!is_reload && !params.state.empty()) {
+    // We must know the page ID of the page we are navigating back to.
+    DCHECK(params.page_id != -1);
+    main_frame->LoadHistoryState(params.state);
+  } else {
+    // Navigate to the given URL.
+    scoped_ptr<WebRequest> request(WebRequest::Create(params.url));
 
-  if (params.referrer.is_valid()) {
-    request->SetHttpHeaderValue("Referer",
-                                params.referrer.spec());
-  }
+    // TODO(darin): WebFrame should just have a Reload method.
 
-  main_frame->LoadRequest(request.get());
+    WebRequestCachePolicy cache_policy;
+    if (is_reload) {
+      cache_policy = WebRequestReloadIgnoringCacheData;
+    } else {
+      // A session history navigation should have been accompanied by state.
+      DCHECK(params.page_id == -1);
+      if (main_frame->GetInViewSourceMode()) {
+        cache_policy = WebRequestReturnCacheDataElseLoad;
+      } else {
+        cache_policy = WebRequestUseProtocolCachePolicy;
+      }
+    }
+    request->SetCachePolicy(cache_policy);
+
+    if (params.referrer.is_valid())
+      request->SetHttpHeaderValue("Referer", params.referrer.spec());
+
+    main_frame->LoadRequest(request.get());
+  }
 
   // In case LoadRequest failed before DidCreateDataSource was called.
   pending_navigation_state_.reset();
diff --git a/webkit/glue/webframe.h b/webkit/glue/webframe.h
index 77d3b27..8271974 100644
--- a/webkit/glue/webframe.h
+++ b/webkit/glue/webframe.h
@@ -76,6 +76,10 @@ class WebFrame {
   // Loads the given WebRequest.
   virtual void LoadRequest(WebRequest* request) = 0;
 
+  // Loads the given history state.  This corresponds to a back/forward
+  // navigation.
+  virtual void LoadHistoryState(const std::string& history_state) = 0;
+
   // This method is short-hand for calling LoadAlternateHTMLString with a dummy
   // request for the given base_url.
   virtual void LoadHTMLString(const std::string& html_text,
diff --git a/webkit/glue/webframe_impl.cc b/webkit/glue/webframe_impl.cc
index 5843d07..9284936 100644
--- a/webkit/glue/webframe_impl.cc
+++ b/webkit/glue/webframe_impl.cc
@@ -408,12 +408,21 @@ void WebFrameImpl::InitMainFrame(WebViewImpl* webview_impl) {
 }
 
 void WebFrameImpl::LoadRequest(WebRequest* request) {
-  SubstituteData data;
-  InternalLoadRequest(request, data, false);
+  InternalLoadRequest(request, SubstituteData(), NULL, false);
+}
+
+void WebFrameImpl::LoadHistoryState(const std::string& history_state) {
+  RefPtr<HistoryItem> history_item =
+      webkit_glue::HistoryItemFromString(history_state);
+  DCHECK(history_item.get());
+
+  WebRequestImpl dummy_request;
+  InternalLoadRequest(&dummy_request, SubstituteData(), history_item, false);
 }
 
 void WebFrameImpl::InternalLoadRequest(const WebRequest* request,
                                        const SubstituteData& data,
+                                       PassRefPtr<HistoryItem> history_item,
                                        bool replace) {
   const WebRequestImpl* request_impl =
       static_cast<const WebRequestImpl*>(request);
@@ -460,7 +469,7 @@ void WebFrameImpl::InternalLoadRequest(const WebRequest* request,
       // loaded page.
       frame_->loader()->setReplacing();
     }
-  } else if (request_impl->history_item()) {
+  } else if (history_item.get()) {
     // Use the history item if we have one, otherwise fall back to standard
     // load.
     RefPtr<HistoryItem> current_item = frame_->loader()->currentHistoryItem();
@@ -476,7 +485,7 @@ void WebFrameImpl::InternalLoadRequest(const WebRequest* request,
       GetWebViewImpl()->SetCurrentHistoryItem(current_item.get());
     }
 
-    frame_->loader()->goToItem(request_impl->history_item().get(),
+    frame_->loader()->goToItem(history_item.get(),
                                WebCore::FrameLoadTypeIndexedBackForward);
   } else if (resource_request.cachePolicy() == ReloadIgnoringCacheData) {
     frame_->loader()->reload();
@@ -503,7 +512,7 @@ void WebFrameImpl::LoadAlternateHTMLString(const WebRequest* request,
       webkit_glue::GURLToKURL(display_url));
   DCHECK(subst_data.isValid());
 
-  InternalLoadRequest(request, subst_data, replace);
+  InternalLoadRequest(request, subst_data, NULL, replace);
 }
 
 GURL WebFrameImpl::GetURL() const {
diff --git a/webkit/glue/webframe_impl.h b/webkit/glue/webframe_impl.h
index 8ecd64c..3ec3f02 100644
--- a/webkit/glue/webframe_impl.h
+++ b/webkit/glue/webframe_impl.h
@@ -79,6 +79,7 @@ class WebFrameImpl : public WebFrame, public base::RefCounted<WebFrameImpl> {
 
   // WebFrame
   virtual void LoadRequest(WebRequest* request);
+  virtual void LoadHistoryState(const std::string& history_state);
   virtual void LoadHTMLString(const std::string& html_text,
                               const GURL& base_url);
   virtual void LoadAlternateHTMLString(const WebRequest* request,
@@ -392,6 +393,7 @@ class WebFrameImpl : public WebFrame, public base::RefCounted<WebFrameImpl> {
 
   void InternalLoadRequest(const WebRequest* request,
                            const WebCore::SubstituteData& data,
+                           PassRefPtr<WebCore::HistoryItem> history_item,
                            bool replace);
 
   // Clears the map of password listeners.
diff --git a/webkit/glue/weburlrequest.h b/webkit/glue/weburlrequest.h
index d4c3596..6443c7c 100644
--- a/webkit/glue/weburlrequest.h
+++ b/webkit/glue/weburlrequest.h
@@ -77,10 +77,6 @@ class WebRequest {
   //
   virtual std::string GetHttpReferrer() const = 0;
 
-  // Get/set the opaque history state (used for back/forward navigations).
-  virtual std::string GetHistoryState() const = 0;
-  virtual void SetHistoryState(const std::string& state) = 0;
-
   // Get/set an opaque value containing the security info (including SSL
   // connection state) that should be reported as used in the response for that
   // request, or an empty string if no security info should be reported.  This
diff --git a/webkit/glue/weburlrequest_impl.cc b/webkit/glue/weburlrequest_impl.cc
index f8ff805..cdbdcab 100644
--- a/webkit/glue/weburlrequest_impl.cc
+++ b/webkit/glue/weburlrequest_impl.cc
@@ -127,16 +127,6 @@ std::string WebRequestImpl::GetHttpReferrer() const {
       request_.resourceRequest().httpReferrer());
 }
 
-std::string WebRequestImpl::GetHistoryState() const {
-  std::string value;
-  webkit_glue::HistoryItemToString(history_item_, &value);
-  return value;
-}
-
-void WebRequestImpl::SetHistoryState(const std::string& value) {
-  history_item_ = webkit_glue::HistoryItemFromString(value);
-}
-
 std::string WebRequestImpl::GetSecurityInfo() const {
   return webkit_glue::CStringToStdString(
       request_.resourceRequest().securityInfo());
diff --git a/webkit/glue/weburlrequest_impl.h b/webkit/glue/weburlrequest_impl.h
index 5a613b3..aa4524a 100644
--- a/webkit/glue/weburlrequest_impl.h
+++ b/webkit/glue/weburlrequest_impl.h
@@ -37,8 +37,6 @@ class WebRequestImpl : public WebRequest {
   virtual void GetHttpHeaders(HeaderMap* headers) const;
   virtual void SetHttpHeaders(const HeaderMap& headers);
   virtual std::string GetHttpReferrer() const;
-  virtual std::string GetHistoryState() const;
-  virtual void SetHistoryState(const std::string& value);
   virtual std::string GetSecurityInfo() const;
   virtual void SetSecurityInfo(const std::string& value);
   virtual bool HasUploadData() const;
@@ -54,13 +52,8 @@ class WebRequestImpl : public WebRequest {
     request_ = request;
   }
 
-  PassRefPtr<WebCore::HistoryItem> history_item() const {
-    return history_item_;
-  }
-
  protected:
   WebCore::FrameLoadRequest request_;
-  RefPtr<WebCore::HistoryItem> history_item_;
 };
 
 #endif  // #ifndef WEBKIT_GLUE_WEBURLREQUEST_IMPL_H_
diff --git a/webkit/tools/test_shell/test_shell.cc b/webkit/tools/test_shell/test_shell.cc
index f2d5b0c..6c94b1d 100644
--- a/webkit/tools/test_shell/test_shell.cc
+++ b/webkit/tools/test_shell/test_shell.cc
@@ -494,51 +494,53 @@ void TestShell::LoadURL(const wchar_t* url) {
 }
 
 bool TestShell::Navigate(const TestNavigationEntry& entry, bool reload) {
+  // Get the right target frame for the entry.
+  WebFrame* frame = webView()->GetMainFrame();
+  if (!entry.GetTargetFrame().empty())
+      frame = webView()->GetFrameWithName(entry.GetTargetFrame());
+  // TODO(mpcomplete): should we clear the target frame, or should
+  // back/forward navigations maintain the target frame?
+
+  // A navigation resulting from loading a javascript URL should not be
+  // treated as a browser initiated event.  Instead, we want it to look as if
+  // the page initiated any load resulting from JS execution.
+  if (!entry.GetURL().SchemeIs("javascript")) {
+    delegate_->set_pending_extra_data(
+        new TestShellExtraData(entry.GetPageID()));
+  }
+
+  // If we are reloading, then WebKit will use the state of the current page.
+  // Otherwise, we give it the state to navigate to.
+  if (!reload && !entry.GetContentState().empty()) {
+    DCHECK(entry.GetPageID() != -1);
+    frame->LoadHistoryState(entry.GetContentState());
+  } else {
     WebRequestCachePolicy cache_policy;
     if (reload) {
       cache_policy = WebRequestReloadIgnoringCacheData;
-    } else if (entry.GetPageID() != -1) {
-      cache_policy = WebRequestReturnCacheDataElseLoad;
     } else {
+      DCHECK(entry.GetPageID() == -1);
       cache_policy = WebRequestUseProtocolCachePolicy;
     }
 
     scoped_ptr<WebRequest> request(WebRequest::Create(entry.GetURL()));
     request->SetCachePolicy(cache_policy);
-    // If we are reloading, then WebKit will use the state of the current page.
-    // Otherwise, we give it the state to navigate to.
-    if (!reload)
-      request->SetHistoryState(entry.GetContentState());
-
-    // A navigation resulting from loading a javascript URL should not be
-    // treated as a browser initiated event.  Instead, we want it to look as if
-    // the page initiated any load resulting from JS execution.
-    if (!entry.GetURL().SchemeIs("javascript")) {
-      delegate_->set_pending_extra_data(
-          new TestShellExtraData(entry.GetPageID()));
-    }
-
-    // Get the right target frame for the entry.
-    WebFrame* frame = webView()->GetMainFrame();
-    if (!entry.GetTargetFrame().empty())
-        frame = webView()->GetFrameWithName(entry.GetTargetFrame());
-    // TODO(mpcomplete): should we clear the target frame, or should
-    // back/forward navigations maintain the target frame?
 
     frame->LoadRequest(request.get());
+  }
 
-    // In case LoadRequest failed before DidCreateDataSource was called.
-    delegate_->set_pending_extra_data(NULL);
+  // In case LoadRequest failed before DidCreateDataSource was called.
+  delegate_->set_pending_extra_data(NULL);
 
-    // Restore focus to the main frame prior to loading new request.
-    // This makes sure that we don't have a focused iframe. Otherwise, that
-    // iframe would keep focus when the SetFocus called immediately after
-    // LoadRequest, thus making some tests fail (see http://b/issue?id=845337
-    // for more details).
-    webView()->SetFocusedFrame(frame);
-    SetFocus(webViewHost(), true);
+  // Restore focus to the main frame prior to loading new request.
+  // This makes sure that we don't have a focused iframe. Otherwise, that
+  // iframe would keep focus when the SetFocus called immediately after
+  // LoadRequest, thus making some tests fail (see http://b/issue?id=845337
+  // for more details).
+  webView()->SetFocusedFrame(frame);
+  SetFocus(webViewHost(), true);
 
-    return true;
+  return true;
 }
 
 void TestShell::GoBackOrForward(int offset) {