Refactor NavigationState to use NavigationParams

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 437e8c98be937398ffd4b3ca46cdfb4c5b964f16..1a210380f9a3edc59939055f2779a5c940e14b3c 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -84,6 +84,7 @@
 #include "content/renderer/media/webmediaplayer_ms.h"
 #include "content/renderer/memory_benchmarking_extension.h"
 #include "content/renderer/mojo/service_registry_js_wrapper.h"
+#include "content/renderer/navigation_state_impl.h"
 #include "content/renderer/notification_permission_dispatcher.h"
 #include "content/renderer/npapi/plugin_channel_host.h"
 #include "content/renderer/pepper/plugin_instance_throttler_impl.h"
@@ -2018,7 +2019,8 @@ void RenderFrameImpl::didAccessInitialDocument(blink::WebLocalFrame* frame) {
   if (!frame->parent()) {
     DocumentState* document_state =
         DocumentState::FromDataSource(frame->dataSource());
-    NavigationState* navigation_state = document_state->navigation_state();
+    NavigationStateImpl* navigation_state =
+        static_cast<NavigationStateImpl*>(document_state->navigation_state());
 
     if (!navigation_state->request_committed()) {
       Send(new FrameHostMsg_DidAccessInitialDocument(routing_id_));
@@ -2259,13 +2261,15 @@ void RenderFrameImpl::willSubmitForm(blink::WebLocalFrame* frame,
   DCHECK(!frame_ || frame_ == frame);
   DocumentState* document_state =
       DocumentState::FromDataSource(frame->provisionalDataSource());
-  NavigationState* navigation_state = document_state->navigation_state();
+  NavigationStateImpl* navigation_state =
+      static_cast<NavigationStateImpl*>(document_state->navigation_state());
   InternalDocumentStateData* internal_data =
       InternalDocumentStateData::FromDocumentState(document_state);
 
-  if (ui::PageTransitionCoreTypeIs(navigation_state->transition_type(),
+  if (ui::PageTransitionCoreTypeIs(navigation_state->common_params().transition,

[Charlie Reis, 2015/03/17 03:26:31]

Why not navigation_state->GetTransitionType()?

[clamy, 2015/03/17 16:08:18]

Done.

                                    ui::PAGE_TRANSITION_LINK)) {
-    navigation_state->set_transition_type(ui::PAGE_TRANSITION_FORM_SUBMIT);
+    navigation_state->common_params().transition =

[Charlie Reis, 2015/03/17 03:26:31]

Should use a setter for this.  Same below.

[clamy, 2015/03/17 16:08:18]

Done.

+        ui::PAGE_TRANSITION_FORM_SUBMIT;
   }
 
   // Save these to be processed when the ensuing navigation is committed.
@@ -2341,8 +2345,9 @@ void RenderFrameImpl::didStartProvisionalLoad(blink::WebLocalFrame* frame,
     // Subframe navigations that don't add session history items must be
     // marked with AUTO_SUBFRAME. See also didFailProvisionalLoad for how we
     // handle loading of error pages.
-    document_state->navigation_state()->set_transition_type(
-        ui::PAGE_TRANSITION_AUTO_SUBFRAME);
+    static_cast<NavigationStateImpl*>(document_state->navigation_state())
+        ->common_params()
+        .transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME;
   }
 
   FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers(),
@@ -2424,7 +2429,8 @@ void RenderFrameImpl::didFailProvisionalLoad(blink::WebLocalFrame* frame,
   frame->enableViewSourceMode(false);
 
   DocumentState* document_state = DocumentState::FromDataSource(ds);
-  NavigationState* navigation_state = document_state->navigation_state();
+  NavigationStateImpl* navigation_state =
+      static_cast<NavigationStateImpl*>(document_state->navigation_state());
 
   // If this is a failed back/forward/reload navigation, then we need to do a
   // 'replace' load.  This is necessary to avoid messing up session history.
@@ -2437,28 +2443,19 @@ void RenderFrameImpl::didFailProvisionalLoad(blink::WebLocalFrame* frame,
   // TODO(davidben): This should also take the failed navigation's replacement
   // state into account, if a location.replace() failed.
   bool replace =
-      navigation_state->pending_page_id() != -1 ||
-      ui::PageTransitionCoreTypeIs(navigation_state->transition_type(),
-                               ui::PAGE_TRANSITION_AUTO_SUBFRAME);
+      navigation_state->history_params().page_id != -1 ||
+      ui::PageTransitionCoreTypeIs(navigation_state->common_params().transition,
+                                   ui::PAGE_TRANSITION_AUTO_SUBFRAME);
 
   // If we failed on a browser initiated request, then make sure that our error
   // page load is regarded as the same browser initiated request.
-  if (!navigation_state->is_content_initiated()) {
+  if (!navigation_state->IsContentInitiated()) {
     render_view_->pending_navigation_params_.reset(new NavigationParams(
-        CommonNavigationParams(
-            error.unreachableURL, Referrer(),

[Charlie Reis, 2015/03/17 03:26:31]

I don't understand how the new code is equivalent in this case.  Where are you
handling the error.unreachableURL (etc)?

[clamy, 2015/03/17 16:08:18]

Well until
https://codereview.chromium.org/971653004/diff/130001/content/renderer/render...
we were resetting the params with a default FrameMsg_Navigate_Params without
setting the url. I thought that based on the comment resetting it with
error.unreachableURL would be closer to the intention of having the error page
load be regarded as the same browser-initiated request then using an empty URL.
And now that we keep what we initially received from the browser in
NavigationState we can reset it with exactly what we received, hence why I'm not
setting the url to error.unreachableURL.

[Charlie Reis, 2015/03/17 16:29:06]

Oh I see-- it's from a recent change.  Ok, I'll defer here since I don't have a
great understanding of the error page logic.

-            navigation_state->transition_type(), FrameMsg_Navigate_Type::NORMAL,
-            true, base::TimeTicks(),
-            FrameMsg_UILoadMetricsReportType::NO_REPORT, GURL(), GURL()),
-        StartNavigationParams(false, std::string(),
-                              std::vector<unsigned char>(), replace, -1, -1),
+        navigation_state->common_params(), navigation_state->start_params(),
         CommitNavigationParams(false, base::TimeTicks(), std::vector<GURL>(),
                                false, std::string(),
                                document_state->request_time()),
-        HistoryNavigationParams(
-            PageState(), navigation_state->pending_page_id(),
-            navigation_state->pending_history_list_offset(), -1, 0,
-            navigation_state->history_list_was_cleared())));
+        navigation_state->history_params()));
   }
 
   // Load an error page.
@@ -2475,7 +2472,8 @@ void RenderFrameImpl::didCommitProvisionalLoad(
   DCHECK(!frame_ || frame_ == frame);
   DocumentState* document_state =
       DocumentState::FromDataSource(frame->dataSource());
-  NavigationState* navigation_state = document_state->navigation_state();
+  NavigationStateImpl* navigation_state =
+      static_cast<NavigationStateImpl*>(document_state->navigation_state());
 
   if (proxy_routing_id_ != MSG_ROUTING_NONE) {
     RenderFrameProxy* proxy =
@@ -2490,8 +2488,8 @@ void RenderFrameImpl::didCommitProvisionalLoad(
   // before updating the HistoryController state.
   render_view_->UpdateSessionHistory(frame);
 
-  render_view_->history_controller()->UpdateForCommit(this, item, commit_type,
-      navigation_state->was_within_same_page());
+  render_view_->history_controller()->UpdateForCommit(
+      this, item, commit_type, navigation_state->GetWasWithinSamePage());
 
   InternalDocumentStateData* internal_data =
       InternalDocumentStateData::FromDocumentState(document_state);
@@ -2516,10 +2514,11 @@ void RenderFrameImpl::didCommitProvisionalLoad(
     // UpdateSessionHistory and update page_id_ even in this case, so that
     // the current entry gets a state update and so that we don't send a
     // state update to the wrong entry when we swap back in.
-    DCHECK_IMPLIES(navigation_state->should_replace_current_entry(),
-                   render_view_->history_list_length_ > 0);
+    DCHECK_IMPLIES(
+        navigation_state->start_params().should_replace_current_entry,
+        render_view_->history_list_length_ > 0);
     if (GetLoadingUrl() != GURL(kSwappedOutURL) &&
-        !navigation_state->should_replace_current_entry()) {
+        !navigation_state->start_params().should_replace_current_entry) {
       // Advance our offset in session history, applying the length limit.
       // There is now no forward history.
       render_view_->history_list_offset_++;
@@ -2539,14 +2538,14 @@ void RenderFrameImpl::didCommitProvisionalLoad(
     // Note that we need to check if the page ID changed. In the case of a
     // reload, the page ID doesn't change, and UpdateSessionHistory gets the
     // previous URL and the current page ID, which would be wrong.
-    if (navigation_state->pending_page_id() != -1 &&
-        navigation_state->pending_page_id() != render_view_->page_id_ &&
+    if (navigation_state->history_params().page_id != -1 &&
+        navigation_state->history_params().page_id != render_view_->page_id_ &&
         !navigation_state->request_committed()) {
       // This is a successful session history navigation!
-      render_view_->page_id_ = navigation_state->pending_page_id();
+      render_view_->page_id_ = navigation_state->history_params().page_id;
 
       render_view_->history_list_offset_ =
-          navigation_state->pending_history_list_offset();
+          navigation_state->history_params().pending_history_list_offset;
     }
   }
 
@@ -2556,10 +2555,10 @@ void RenderFrameImpl::didCommitProvisionalLoad(
 
   FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers_,
                     DidCommitProvisionalLoad(frame, is_new_navigation));
-  FOR_EACH_OBSERVER(RenderFrameObserver, observers_,
-                    DidCommitProvisionalLoad(
-                        is_new_navigation,
-                        navigation_state->was_within_same_page()));
+  FOR_EACH_OBSERVER(
+      RenderFrameObserver, observers_,
+      DidCommitProvisionalLoad(is_new_navigation,
+                               navigation_state->GetWasWithinSamePage()));
 
   if (!frame->parent()) {  // Only for top frames.
     RenderThreadImpl* render_thread_impl = RenderThreadImpl::current();
@@ -2773,8 +2772,8 @@ void RenderFrameImpl::didNavigateWithinPage(blink::WebLocalFrame* frame,
 
   DocumentState* document_state =
       DocumentState::FromDataSource(frame->dataSource());
-  NavigationState* new_state = document_state->navigation_state();
-  new_state->set_was_within_same_page(true);
+  static_cast<NavigationStateImpl*>(document_state->navigation_state())
+      ->set_was_within_same_page(true);
 
   didCommitProvisionalLoad(frame, item, commit_type);
 }
@@ -2990,8 +2989,10 @@ void RenderFrameImpl::willSendRequest(
   DCHECK(document_state);
   InternalDocumentStateData* internal_data =
       InternalDocumentStateData::FromDocumentState(document_state);
-  NavigationState* navigation_state = document_state->navigation_state();
-  ui::PageTransition transition_type = navigation_state->transition_type();
+  NavigationStateImpl* navigation_state =
+      static_cast<NavigationStateImpl*>(document_state->navigation_state());
+  ui::PageTransition transition_type =
+      navigation_state->common_params().transition;
   WebDataSource* frame_ds = frame->provisionalDataSource();
   if (frame_ds && frame_ds->isClientRedirect()) {
     transition_type = ui::PageTransitionFromInt(
@@ -3060,7 +3061,7 @@ void RenderFrameImpl::willSendRequest(
   // this navigation later require a request transfer, all state is preserved
   // when it is re-created in the new process.
   bool should_replace_current_entry = false;
-  if (navigation_state->is_content_initiated()) {
+  if (navigation_state->IsContentInitiated()) {
     should_replace_current_entry = data_source->replacesCurrentHistoryItem();
   } else {
     // If the navigation is browser-initiated, the NavigationState contains the
@@ -3069,7 +3070,7 @@ void RenderFrameImpl::willSendRequest(
     // TODO(davidben): Avoid this awkward duplication of state. See comment on
     // NavigationState::should_replace_current_entry().
     should_replace_current_entry =
-        navigation_state->should_replace_current_entry();
+        navigation_state->start_params().should_replace_current_entry;
   }
 
   int provider_id = kInvalidServiceWorkerProviderId;
@@ -3110,13 +3111,14 @@ void RenderFrameImpl::willSendRequest(
       GURL(frame->document().securityOrigin().toString()));
   extra_data->set_parent_is_main_frame(parent && !parent->parent());
   extra_data->set_parent_render_frame_id(parent_routing_id);
-  extra_data->set_allow_download(navigation_state->allow_download());
+  extra_data->set_allow_download(
+      navigation_state->common_params().allow_download);
   extra_data->set_transition_type(transition_type);
   extra_data->set_should_replace_current_entry(should_replace_current_entry);
   extra_data->set_transferred_request_child_id(
-      navigation_state->transferred_request_child_id());
+      navigation_state->start_params().transferred_request_child_id);
   extra_data->set_transferred_request_request_id(
-      navigation_state->transferred_request_request_id());
+      navigation_state->start_params().transferred_request_request_id);
   extra_data->set_service_worker_provider_id(provider_id);
   extra_data->set_stream_override(stream_override.Pass());
   request.setExtraData(extra_data);
@@ -3137,11 +3139,11 @@ void RenderFrameImpl::willSendRequest(
   request.setRequestorID(render_view_->GetRoutingID());
   request.setHasUserGesture(WebUserGestureIndicator::isProcessingUserGesture());
 
-  if (!navigation_state->extra_headers().empty()) {
+  if (!navigation_state->start_params().extra_headers.empty()) {
     for (net::HttpUtil::HeadersIterator i(
-        navigation_state->extra_headers().begin(),
-        navigation_state->extra_headers().end(), "\n");
-        i.GetNext(); ) {
+             navigation_state->start_params().extra_headers.begin(),
+             navigation_state->start_params().extra_headers.end(), "\n");
+         i.GetNext();) {
       if (LowerCaseEqualsASCII(i.name(), "referer")) {
         WebString referrer = WebSecurityPolicy::generateReferrerHeader(
             blink::WebReferrerPolicyDefault,
@@ -3696,7 +3698,8 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad(
   const WebURLResponse& response = ds->response();
 
   DocumentState* document_state = DocumentState::FromDataSource(ds);
-  NavigationState* navigation_state = document_state->navigation_state();
+  NavigationStateImpl* navigation_state =
+      static_cast<NavigationStateImpl*>(document_state->navigation_state());
   InternalDocumentStateData* internal_data =
       InternalDocumentStateData::FromDocumentState(document_state);
 
@@ -3719,7 +3722,7 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad(
   WebURLResponseExtraDataImpl* extra_data = GetExtraDataFromResponse(response);
   if (extra_data)
     params.was_fetched_via_proxy = extra_data->was_fetched_via_proxy();
-  params.was_within_same_page = navigation_state->was_within_same_page();
+  params.was_within_same_page = navigation_state->GetWasWithinSamePage();
   params.security_info = response.securityInfo();
 
   // Set the URL to be displayed in the browser UI to the user.
@@ -3791,7 +3794,7 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad(
     // Update contents MIME type for main frame.
     params.contents_mime_type = ds->response().mimeType().utf8();
 
-    params.transition = navigation_state->transition_type();
+    params.transition = navigation_state->common_params().transition;
     if (!ui::PageTransitionIsMainFrame(params.transition)) {
       // If the main frame does a load, it should not be reported as a subframe
       // navigation.  This can occur in the following case:
@@ -3834,7 +3837,7 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad(
     params.original_request_url = GetOriginalRequestURL(ds);
 
     params.history_list_was_cleared =
-        navigation_state->history_list_was_cleared();
+        navigation_state->history_params().should_clear_history_list;
 
     params.report_type = static_cast<FrameMsg_UILoadMetricsReportType::Value>(
         frame->dataSource()->request().inputPerfMetricReportPolicy());
@@ -3861,7 +3864,7 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad(
     else
       params.transition = ui::PAGE_TRANSITION_AUTO_SUBFRAME;
 
-    DCHECK(!navigation_state->history_list_was_cleared());
+    DCHECK(!navigation_state->history_params().should_clear_history_list);
     params.history_list_was_cleared = false;
     params.report_type = FrameMsg_UILoadMetricsReportType::NO_REPORT;
 
@@ -3872,7 +3875,7 @@ void RenderFrameImpl::SendDidCommitProvisionalLoad(
 
   // If we end up reusing this WebRequest (for example, due to a #ref click),
   // we don't want the transition type to persist.  Just clear it.
-  navigation_state->set_transition_type(ui::PAGE_TRANSITION_LINK);
+  navigation_state->common_params().transition = ui::PAGE_TRANSITION_LINK;
 }
 
 void RenderFrameImpl::didStartLoading(bool to_different_document) {
@@ -4027,8 +4030,10 @@ WebNavigationPolicy RenderFrameImpl::DecidePolicyForNavigation(
 
   // A content initiated navigation may have originated from a link-click,
   // script, drag-n-drop operation, etc.
-  bool is_content_initiated = static_cast<DocumentState*>(info.extraData)->
-          navigation_state()->is_content_initiated();
+  DocumentState* document_state = static_cast<DocumentState*>(info.extraData);
+  bool is_content_initiated =
+      static_cast<NavigationStateImpl*>(document_state->navigation_state())

[Charlie Reis, 2015/03/17 03:26:31]

Why do you need this static cast?  IsContentInitiated is on the public
interface.

[clamy, 2015/03/17 16:08:18]

Done (it was not initially).

+          ->IsContentInitiated();
 
   // Experimental:
   // If --enable-strict-site-isolation is enabled, send all top-level
@@ -4208,8 +4213,9 @@ void RenderFrameImpl::OpenURL(WebFrame* frame,
   WebDataSource* ds = frame->provisionalDataSource();
   if (ds) {
     DocumentState* document_state = DocumentState::FromDataSource(ds);
-    NavigationState* navigation_state = document_state->navigation_state();
-    if (navigation_state->is_content_initiated()) {
+    NavigationStateImpl* navigation_state =
+        static_cast<NavigationStateImpl*>(document_state->navigation_state());
+    if (navigation_state->IsContentInitiated()) {
       params.should_replace_current_entry =
           ds->replacesCurrentHistoryItem() &&
           render_view_->history_list_length_;
@@ -4220,7 +4226,7 @@ void RenderFrameImpl::OpenURL(WebFrame* frame,
       // TODO(davidben): Avoid this awkward duplication of state. See comment on
       // NavigationState::should_replace_current_entry().
       params.should_replace_current_entry =
-          navigation_state->should_replace_current_entry();
+          navigation_state->start_params().should_replace_current_entry;
     }
   } else {
     params.should_replace_current_entry = false;