Add the navigation redirect-chain to Sync sessions proto for offline analysis.

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 57a92f55a4ee341032d2dc8492133b699d0db368..25318cdec0398c1d1cabe49d4e79044f5aa03216 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -705,7 +705,7 @@ void NavigationControllerImpl::LoadURLWithParams(const LoadURLParams& params) {
   if (params.frame_tree_node_id != -1)
     entry->set_frame_tree_node_id(params.frame_tree_node_id);
   if (params.redirect_chain.size() > 0)
-    entry->set_redirect_chain(params.redirect_chain);
+    entry->SetRedirectChain(params.redirect_chain);
   if (params.should_replace_current_entry)
     entry->set_should_replace_entry(true);
   entry->set_should_clear_history_list(params.should_clear_history_list);
@@ -826,6 +826,8 @@ bool NavigationControllerImpl::RendererDidNavigate(
   active_entry->SetTimestamp(timestamp);
   active_entry->SetHttpStatusCode(params.http_status_code);
   active_entry->SetPageState(params.page_state);
+  active_entry->SetRedirectChain(
+      GetMergedRedirectChain(params, active_entry->GetRedirectChain()));
 
   // Once it is committed, we no longer need to track several pieces of state on
   // the entry.
@@ -852,6 +854,35 @@ bool NavigationControllerImpl::RendererDidNavigate(
   return true;
 }
 
+// static
+std::vector<GURL> NavigationControllerImpl::GetMergedRedirectChain(

[sky, 2014/02/25 22:42:15]

Seems like this should be only called in the switch statement when appropriate.
Brett will have to offer his advice on this though.

[brettw, 2014/03/11 22:40:01]

I'd prefer a function UpdateRedirectChain(params, active_entry) {
  if (PageTransitionCoreTypeIs(params.transition,
content::PAGE_TRANSITION_RELOAD))
    return;
  if (...FORWARDBACK))
     return;
  ...
}

In addition to that, I don't really understand the merging thing. It seems like
we'd always want to replace if we think we have a new list, and if we don't have
a new list, we always want to leave it as-is. If there is a need for this, there
should be a detailed comment here giving an example.

[Donn Denman, 2014/03/12 20:13:36]

Done.

[Donn Denman, 2014/03/12 20:13:36]

Applied Brett's suggestion.  Now that this function immediately returns when
appropriate, does that address your concern? 

+    const FrameHostMsg_DidCommitProvisionalLoad_Params& params,
+    const std::vector<GURL>& active_entry_redirects) const {
+  // TODO(donnd): are there other transition cases we need to handle specially?
+  bool is_transition_reload = PageTransitionCoreTypeIs(
+      params.transition, content::PAGE_TRANSITION_RELOAD);
+  bool is_transition_forward_back = PageTransitionIsForwardBack(
+      params.transition);
+  if (is_transition_forward_back || is_transition_reload) {
+    // Returning to an existing page so the active entry is complete.
+    return active_entry_redirects;
+  }
+  if (active_entry_redirects.size() == 0)
+    return params.redirects;
+  // Merge new redirects into the active entry.
+  std::vector<GURL> merged_redirects = active_entry_redirects;
+  // Remove entries that duplicate the last URL from the active entry.
+  std::vector<GURL>::const_iterator first_different_param =
+      params.redirects.begin();
+  while (first_different_param != params.redirects.end() &&
+      *first_different_param == merged_redirects.back()) {
+    first_different_param++;
+  }
+  merged_redirects.insert(
+      merged_redirects.end(), first_different_param, params.redirects.end());
+  return merged_redirects;
+}
+
 NavigationType NavigationControllerImpl::ClassifyNavigation(
     RenderViewHost* rvh,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params) const {