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

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index f01dc531fca5073272ebf8e64a56d9a65e265bc1..8e6428955b9cc63eaeb631c41eea4d20207d5ce1 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -105,7 +105,18 @@ void MakeNavigateParams(const NavigationEntryImpl& entry,
             entry.GetBrowserInitiatedPostData()->size());
   }
 
-  params->redirects = entry.redirect_chain();
+  // We no longer clear redirects in the navigation entry upon commit, so

[brettw, 2014/03/27 22:16:22]

This comment only makes sense in the context of this change. i.e. if somebody
reads this later the "we no longer" part makes no sense because they don't care
how it used to be. It should just say what happens now and why we need to do
this.

Also, the way you wrote "clear" them is confusing. You're not clearing anything
in this code, you're just not copying it in some case. I would expect from this
comment you're editing the navigation entry.

[donnd, 2014/03/28 03:11:38]

I should have known better than to write a non-durable comment - shame on me,
and thanks for flagging that.

Does this look OK now?

+  // we need to clear them when returning to a previously committed navigation.
+  // Those cases are reload and forward/back.
+  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) {
+    params->redirects = std::vector<GURL>();

[brettw, 2014/03/27 22:16:22]

Actually, I'm curious why you need to not send the redirects? Were you just
trying to match the old behavior? If so, are we sure that was correct/best?

Its difficult for me to follow through what this is used for. It looks like the
renderer doesn't use this and just forwards it back to the browser in the
commit. And then it looks like maybe it just comes back to your new code in the
navigation controller where you do
active_entry->SetRedirectChain(params.redirects);

If that's true, it seems like we would want to preserve the redirects? I think
you should definitely trace through what happens with this.

[donnd, 2014/03/28 03:11:38]

No, the old behavior was to preserve the redirects, and that's the default case
now too.

However, we need to prevent the chain from growing endlessly when returning to
previously committed navigations (e.g. when reloading the page).  The old
behavior cut off the chain at commit time, so that wasn't a problem that had to
be handled here.
That all sounds right to me, and we do preserve the redirects except in those
committed cases.

+  } else {
+    params->redirects = entry.GetRedirectChain();
+  }
 
   params->can_load_local_resources = entry.GetCanLoadLocalResources();
   params->frame_to_navigate = entry.GetFrameToNavigate();
@@ -168,7 +179,7 @@ void NavigatorImpl::DidStartProvisionalLoad(
         entry->set_transferred_global_request_id(
             pending_entry->transferred_global_request_id());
         entry->set_should_replace_entry(pending_entry->should_replace_entry());
-        entry->set_redirect_chain(pending_entry->redirect_chain());
+        entry->SetRedirectChain(pending_entry->GetRedirectChain());
       }
       controller_->SetPendingEntry(entry);
       if (delegate_)