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

content/browser/frame_host/navigator_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/navigator_impl.h"
 
 #include "base/command_line.h"
 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
@@ skipping 87 matching lines @@
   // Avoid downloading when in view-source mode.
   params->allow_download = !entry.IsViewSourceMode();
   params->is_post = entry.GetHasPostData();
   if (entry.GetBrowserInitiatedPostData()) {
     params->browser_initiated_post_data.assign(
         entry.GetBrowserInitiatedPostData()->front(),
         entry.GetBrowserInitiatedPostData()->front() +
             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();
 }
 
 }  // namespace
 
 
 NavigatorImpl::NavigatorImpl(
     NavigationControllerImpl* navigation_controller,
@@ skipping 42 matching lines @@
                                              controller_->GetBrowserContext()));
       entry->set_site_instance(
           static_cast<SiteInstanceImpl*>(
               render_frame_host->render_view_host()->GetSiteInstance()));
       // TODO(creis): If there's a pending entry already, find a safe way to
       // update it instead of replacing it and copying over things like this.
       if (pending_entry) {
         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_)
         delegate_->NotifyChangedNavigationState(content::INVALIDATE_TYPE_URL);
     }
   }
 
   if (delegate_) {
     // Notify the observer about the start of the provisional load.
     delegate_->DidStartProvisionalLoad(
@@ skipping 345 matching lines @@
   // still be used for a normal web site.
   if (url == GURL(kAboutBlankURL))
     return false;
 
   // The embedder will then have the opportunity to determine if the URL
   // should "use up" the SiteInstance.
   return GetContentClient()->browser()->ShouldAssignSiteForURL(url);
 }
 
 }  // namespace content