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();
+  // Set the redirect chain to the navigation's redirects, unless we are
+  // returning to a completed navigation (whose previous redirects don't apply).
+  bool is_transition_reload = PageTransitionCoreTypeIs(
+      params->transition, content::PAGE_TRANSITION_RELOAD);
+  bool is_transition_forward_back = PageTransitionIsForwardBack(

[Charlie Reis, 2014/03/31 22:06:08]

Does PageTransitionIsRedirect not suffice here?

[donnd, 2014/04/01 04:36:26]

No, returning to any previously committed navigation will pick up those
redirects, but we don't want to send them to sync as if we just did the redirect
all over again.

[brettw, 2014/04/01 06:28:52]

Ah, so this answer seems to be different than we were talking about on Friday. I
had asked you why you needed to clear here and you thought the list was getting
appended to, but weren't sure where.

In this comment, it seems like you're saying the redirects are preserved from
the original navigation "properly" when doing back-forward, but you don't want
them?

Is my summary correct?

If so, why don't we want these in sync? I don't think the contents of what's in
the navigation entry should be driven by what's desired by sync (although in
this case I question the desire) bur rather what makes sense to have in a
navigation entry. And I would expect the redirects to be there.

[donnd, 2014/04/01 18:31:09]

No, they need to be preserved in the original navigation but should not be
passed on to the new navigation on back-forward.
Sorry I have not been clear in communicating what happens in these cases.  When
I said "appended to" on Friday I meant that the previous redirects would be
preserved instead of cleared so subsequent navigations append to them.  

I think an example will help: When navigating to A, which redirects to B and
then C, we get a redirect chain ABC.  If the user does a reload or forward or
back, the redirect chain should just be C.  However, in this CL we're preserving
the redirect chain, so it will be appended to, and we'll see ABCC, which is
wrong.  Starting with an empty chain here fixes those cases.  

BTW, when I said we wouldn't want to send that to sync I just meant that as an
example of sending the wrong data to any part of the system.  We're just trying
to send a snapshot of the redirect chain to sync.

+      params->transition);
+  if (is_transition_forward_back || is_transition_reload) {
+    params->redirects = std::vector<GURL>();
+  } 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